Repository navigation
fix: delegate the remaining codec methods in ComposedPhysicalExtensionCodec - #24973
sainad2222 wants to merge 9 commits into
Conversation
ryux1
left a comment
There was a problem hiding this comment.
The by-name helper handles empty-success delegation carefully. The newly added expression path still inherits a separate composition bug.
ryux1
left a comment
There was a problem hiding this comment.
The new buffer reset isolates each codec attempt, and the regression covers a codec that writes before rejecting followed by one that succeeds. This addresses my concern. Thanks!
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #24973 +/- ##
==========================================
+ Coverage 81.97% 82.50% +0.53%
==========================================
Files 1137 1140 +3
Lines 429922 438917 +8995
Branches 429922 438917 +8995
==========================================
+ Hits 352419 362132 +9713
+ Misses 56453 54882 -1571
- Partials 21050 21903 +853 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
timsaucer
left a comment
There was a problem hiding this comment.
I think the core change makes a lot of sense, but I am worried about code drift. I think we can tighten it up a bit. My agent also had a few comments.
🤖 note:
Compatibility. Worth a line in the PR description: for a composed codec where the accepting codec sat at position > 0 and wrote no bytes, the emitted bytes change (tuple → nothing). Old bytes still decode fine under the new code, and those cases were undecodable before an
…legation' into fix/24829-composed-codec-full-delegation
timsaucer
left a comment
There was a problem hiding this comment.
I think we should fix up the decode by name issue in the previous thread while we're working on this. It's looking good!
f9aedea to
94f2ca8
Compare
94f2ca8 to
2ade206
Compare
Which issue does this PR close?
ComposedPhysicalExtensionCodecimplements only 6 of the 12PhysicalExtensionCodecmethods #24829.Rationale for this change
See #24829.
What changes are included in this PR?
Delegates the six missing methods to the child codecs.
One wrinkle: the four by-name hooks (
udf,udaf,udwf,higher_order_function) default toOk(())meaning "no payload, encode byname", and decode only consults the registry when no payload is present. Naive
delegation wraps an empty blob and strands those functions, invisible at
position 0, where the tuple prost-encodes to zero bytes. Those four now use a
new
encode_protobuf_by_name_aware, which emits a payload only when a codecactually wrote bytes.
try_encode_exprkeeps the plain path; its default is anerror rather than
Ok(()).What is the testing strategy for this PR?
Four tests in
physical_plan::tests::function_serde; two of them fail againstnaive delegation.
Are there any user-facing changes?
try_encode_udf/try_encode_udafwere already delegated but carried the sameempty-payload bug, so they move to the new helper too. Happy to split that out.
No public API signatures changed.