-
Notifications
You must be signed in to change notification settings - Fork 1.5k
feat: metadata handling for aggregates and window functions #15911
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
base: main
Are you sure you want to change the base?
feat: metadata handling for aggregates and window functions #15911
Conversation
7bfcb7b
to
8f0dc24
Compare
FYI @paleolimbot @crystalxyz since this impacts both of your work |
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.
Thank you for this! (Particularly for the heroic updates to all the tests 😬 )
I took a look through and I didn't spot anything out of place (although I'm new to datafusion!).
I'm not sure if the tests are intended to be examples, but it may be worth a slightly more intuitive one (e.g., a sum()
that propagates a metadata key unit
from the source to the destination).
I also still find it strange to use a Field
for these since the name is ignored. Using a dedicated type decoupled from arrow-rs might give you more future flexibility if you need to modify this approach in any way (e.g., to reduce the amount of JSON parsing required when invoking a function on an extension type with parameters). Obviously an optional suggestion!
Which issue does this PR close?
Rationale for this change
This change is a follow on to #15646. With this change we can now handle metadata for both window and aggregate functions. It enables the use of extension types via this metadata handling.
What changes are included in this PR?
Instead of passing
DataType
to aggregates and windows we now passField
in their arguments.return_type
has been replaced withreturn_field
so we can get metadata out of these functions as well.Are these changes tested?
All existing unit tests pass. New unit tests are added.
Are there any user-facing changes?
Yes, the migration guide contains information on the updates that the user will need to make for their user defined functions.
TODO before moving to ready to review
fn return_type()
that returns aField
)