Skip to content

Conversation

dqkqd
Copy link
Contributor

@dqkqd dqkqd commented Oct 5, 2025

Which issue does this PR close?

Rationale for this change

Window::try_new_with_schema is missing doc string with unclear error message.

What changes are included in this PR?

  • Add docs Window::try_new_with_schema.
  • Descriptive error message for invalid output schema.

Are these changes tested?

No. I think this is a just a small docs change.

Are there any user-facing changes?

No.

@github-actions github-actions bot added the logical-expr Logical plan and expressions label Oct 5, 2025
Comment on lines +2546 to +2550
/// 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.
Copy link
Contributor Author

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

/// 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(

Comment on lines -2551 to +2561
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()
Copy link
Contributor Author

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.

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()
);
}

@dqkqd dqkqd marked this pull request as ready for review October 5, 2025 10:54
Copy link
Contributor

@Jefffrey Jefffrey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Copy link
Contributor

@comphead comphead left a 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

@dqkqd
Copy link
Contributor Author

dqkqd commented Oct 6, 2025

Thanks @dqkqd what concerns me is no tests failed on this change

Yes, we currently have no test covering the failure case of try_new_with_schema.
I didn't add one because neither Aggregate::try_new_with_schema nor Projection::try_new_with_schema
has tests for the error path.

@alamb
Copy link
Contributor

alamb commented Oct 7, 2025

Thanks @dqkqd what concerns me is no tests failed on this change

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

@alamb alamb added this pull request to the merge queue Oct 7, 2025
@alamb
Copy link
Contributor

alamb commented Oct 7, 2025

Thanks @dqkqd @Jefffrey and @comphead

Merged via the queue into apache:main with commit 7ce9b6e Oct 7, 2025
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
logical-expr Logical plan and expressions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add docstring for Window::try_new_with_schema
4 participants