Skip to content

Conversation

@Jefffrey
Copy link
Contributor

@Jefffrey Jefffrey commented Nov 2, 2025

Which issue does this PR close?

Rationale for this change

Setting a default of true is too permissive; we can see it allows specifying it on median for example even though that function doesn't take the config into account. It seems only array_agg, first_value and last_value actually respect the config this setting handles so it makes more sense to make this false by default.

What changes are included in this PR?

Change default of AggregateUDFImpl::supports_null_handling_clause to false (from true). Adjust array_agg, first_value and last_value to implement it as true.

Are these changes tested?

Existing tests, also added a negative case for median (expect SQL parsing to fail if providing null handling clause).

Are there any user-facing changes?

Behaviour change as default of a trait method is changing. Added section to upgrade guide.

@github-actions github-actions bot added logical-expr Logical plan and expressions sqllogictest SQL Logic Tests (.slt) functions Changes to functions implementation labels Nov 2, 2025
@github-actions github-actions bot added the documentation Improvements or additions to documentation label Nov 2, 2025
@Jefffrey Jefffrey added the api change Changes the API exposed to users of the crate label Nov 2, 2025
@Jefffrey Jefffrey marked this pull request as ready for review November 2, 2025 06:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api change Changes the API exposed to users of the crate documentation Improvements or additions to documentation functions Changes to functions implementation logical-expr Logical plan and expressions sqllogictest SQL Logic Tests (.slt)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant