Use PhysicalExtensionCodec consistently #10075
Conversation
joroKr21
force-pushed
the
physical-extension-codec
branch
from
73c1310 to
a6993eb
Compare
There was a problem hiding this comment.
Makes sense to me -- thank you @joroKr21 . I think some of the backwards compatible API stuff isn't strictly necessary but I also think it is fine to leave
I had some suggestions for additional tests, but otherwise this looks very nice.
| /// * `input_schema` - The Arrow schema for the input, used for determining expression data types | ||
| /// when performing type coercion. | ||
| /// * `codec` - An extension codec used to decode custom UDFs. |
There was a problem hiding this comment.
thank you for improving the comments here as well ❤️
| ) | ||
| } | ||
|
|
||
| // TODO: Make this the public function on next major release. |
There was a problem hiding this comment.
I think what is on main will effectively become the next major release (38.0.0) so there is no reason to avoid API changes here.
If we want to backport this to a stable 37.1.0 release (e.g. #9904 ) then we should avoid API changes
There was a problem hiding this comment.
Ok, I see so if we want the backwards compatible changes it would be against branch-37
There was a problem hiding this comment.
I added the breaking changes here in 1f0040c
| } | ||
|
|
||
| #[test] | ||
| fn roundtrip_distinct_count() -> Result<()> { |
There was a problem hiding this comment.
it might be worth a test for round tripping window functions with a user defined function too
Which issue does this PR close?
Follup to #9436.
Rationale for this change
We need to use the physical extension codec consistently, so that UDFs embedded in more complex expressions can also be serialized and deserialized. Most importantly in
parse_required_physical_exprwhich calls recursively intoparse_physical_exprand should be providing the original codec.What changes are included in this PR?
Add a
PhysicalExtensionCodecparameter to more of the deserialization functions that were using aDefaultPhysicalExtensionCodecpreviously. I have left the the signature of public functions unchanged but they should be changed in the next major release (left a TODO comments):parse_physical_window_exprparse_protobuf_hash_partitioningparse_protobuf_file_scan_configAre these changes tested?
I extended the test in
roundtrip_physical_plan.rsto use a more complex expression.Are there any user-facing changes?
No, but left a TODO for future changes.