-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Add extension hooks for encoding and decoding UDAFs and UDWFs #11417
Conversation
79e3960
to
2811bf5
Compare
0a7a2e5
to
4c38090
Compare
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.
LGTM
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.
nice!
@@ -239,53 +240,52 @@ struct ComposedPhysicalExtensionCodec { | |||
codecs: Vec<Arc<dyn PhysicalExtensionCodec>>, | |||
} | |||
|
|||
impl ComposedPhysicalExtensionCodec { |
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.
FYI @lewiszlw
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 for the reviews @thinkharderdev and @avantgardnerio -- @joroKr21 it appears this branch has conflicts that need to be resolved. Once those are sorted out I think we can merge this PR |
@alamb do you get this issue locally on
|
I got this error before, running |
4c38090
to
6e3afd2
Compare
🤔 looks like we probably need to increase the minimum version of chrono required to one that has |
…#11417) * Add extension hooks for encoding and decoding UDAFs and UDWFs * Add tests for encoding and decoding UDAF
…#11417) * Add extension hooks for encoding and decoding UDAFs and UDWFs * Add tests for encoding and decoding UDAF
…#11417) * Add extension hooks for encoding and decoding UDAFs and UDWFs * Add tests for encoding and decoding UDAF
Which issue does this PR close?
Closes #11422.
Rationale for this change
The rationale for this change is to be able to run parameterized aggregation UDFs in a distributed architecture.
E.g. one use case is implementing an
sample
UDF (akin toarray_agg
but with a limit on the number of elements collected).What changes are included in this PR?
LogicalExtensionCodec
to handle aggregation UDFs and window UDFs.PhysicalExtensionCodec
to handle aggregation UDFs.ignore_nulls
anddistinct
flags better.Are these changes tested?
Yes, added roundtrip tests for encoding and decoding.
Are there any user-facing changes?
Yes, users can now override the new encoding and decoding functions.