-
Notifications
You must be signed in to change notification settings - Fork 1.7k
docs: Window::try_new_with_schema
with a descriptive error message
#17926
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
/// Create a new window function using the provided schema to avoid the overhead of | ||
/// building the schema again when the schema is already known. | ||
/// | ||
/// This method should only be called when you are absolutely sure that the schema being | ||
/// provided is correct for the window function. If in doubt, call [try_new](Self::try_new) instead. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reuse the docs from Aggregate::try_new_with_schema
datafusion/datafusion/expr/src/logical_plan/plan.rs
Lines 3451 to 3456 in 9e8ec54
/// Create a new aggregate operator using the provided schema to avoid the overhead of | |
/// building the schema again when the schema is already known. | |
/// | |
/// This method should only be called when you are absolutely sure that the schema being | |
/// provided is correct for the aggregate. If in doubt, call [try_new](Self::try_new) instead. | |
pub fn try_new_with_schema( |
if window_expr.len() != schema.fields().len() - input.schema().fields().len() { | ||
let input_fields_count = input.schema().fields().len(); | ||
if schema.fields().len() != input_fields_count + window_expr.len() { | ||
return plan_err!( | ||
"Window has mismatch between number of expressions ({}) and number of fields in schema ({})", | ||
window_expr.len(), | ||
schema.fields().len() - input.schema().fields().len() | ||
"Window schema has wrong number of fields. Expected {} got {}", | ||
input_fields_count + window_expr.len(), | ||
schema.fields().len() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I follow the error message from Aggregate::try_new_with_schema
.
datafusion/datafusion/expr/src/logical_plan/plan.rs
Lines 3470 to 3477 in 9e8ec54
let group_expr_count = grouping_set_expr_count(&group_expr)?; | |
if schema.fields().len() != group_expr_count + aggr_expr.len() { | |
return plan_err!( | |
"Aggregate schema has wrong number of fields. Expected {} got {}", | |
group_expr_count + aggr_expr.len(), | |
schema.fields().len() | |
); | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @dqkqd what concerns me is no tests failed on this change
Yes, we currently have no test covering the failure case of |
Maybe it is worth filing a ticket to track However, I think this particular error message is most likely to show up if someone creates an incorrect plan programatically rather than directly by a higher API so maybe it isn't critical |
Which issue does this PR close?
Window::try_new_with_schema
#17912.Rationale for this change
Window::try_new_with_schema
is missing doc string with unclear error message.What changes are included in this PR?
Window::try_new_with_schema
.Are these changes tested?
No. I think this is a just a small docs change.
Are there any user-facing changes?
No.