refactor(proto): migrate aggregate exec serde - #23779
Conversation
|
Thank you for opening this pull request! Reviewer note: cargo-semver-checks reported the current version number is not SemVer-compatible with the changes in this pull request (compared against the base branch). Details |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #23779 +/- ##
==========================================
- Coverage 80.65% 80.63% -0.03%
==========================================
Files 1091 1091
Lines 370692 370720 +28
Branches 370692 370720 +28
==========================================
- Hits 298972 298916 -56
- Misses 53885 53961 +76
- Partials 17835 17843 +8 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
1366315 to
fa4209e
Compare
|
Please resolve conflicts 🙏🏻 |
|
Reviewed this against the reference implementation in #23496 and against the code it deletes
Also note |
fa4209e to
73c6075
Compare
Aggregate protobuf conversion lived in the central proto crate, which prevented AggregateExec from owning its function-codec serialization. Move encoding and decoding into AggregateExec and keep the deprecated helpers as delegates so existing callers retain wire-compatible behavior. CLOSES apache#23512 Signed-off-by: Jiawei Zhao <Phoenix500526@163.com>
AggregateExec now owns its protobuf conversion, so edge-case tests and serialization rationale belong beside the implementation. Keeping these constraints local makes the self-serialization migration safer while the deprecated central serializer remains available. Refs apache#23512 Signed-off-by: Jiawei Zhao <Phoenix500526@163.com>
73c6075 to
0bb42c4
Compare
Done |
Done |
## Which issue does this PR close? <!-- We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes apache#123` indicates that this PR will close issue apache#123. --> - Closes apache#23512. ## Rationale for this change <!-- Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed. Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes. --> Aggregate protobuf conversion lived in the central proto crate, which prevented AggregateExec from owning its function-codec serialization. Move encoding and decoding into AggregateExec and keep the deprecated helpers as delegates so existing callers retain wire-compatible behavior. ## What changes are included in this PR? <!-- There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR. --> ## Are these changes tested? <!-- We typically require tests for all PRs in order to: 1. Prevent the code from being accidentally broken by subsequent changes 2. Serve as another way to document the expected behavior of the code If tests are not included in your PR, please explain why (for example, are they covered by existing tests)? --> Yes ## Are there any user-facing changes? <!-- If there are user-facing changes then we may require documentation to be updated before approving the PR. --> <!-- If there are any breaking changes to public APIs, please add the `api change` label. --> --------- Signed-off-by: Jiawei Zhao <Phoenix500526@163.com>
Which issue does this PR close?
Rationale for this change
Aggregate protobuf conversion lived in the central proto crate, which prevented AggregateExec from owning its function-codec serialization.
Move encoding and decoding into AggregateExec and keep the deprecated helpers as delegates so existing callers retain wire-compatible behavior.
What changes are included in this PR?
Are these changes tested?
Yes
Are there any user-facing changes?