-
Notifications
You must be signed in to change notification settings - Fork 843
feat: remove #[typetag::serde] on PhysicalPlan, Static dispatch implementation of serde using enum. #19114
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: remove #[typetag::serde] on PhysicalPlan, Static dispatch implementation of serde using enum. #19114
Conversation
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.
Pull request overview
This PR replaces the #[typetag::serde] trait object-based polymorphic serialization with an enum-based static dispatch approach to improve performance by reducing call stack depth during serialization.
Key Changes:
- Introduced
PhysicalPlanImplenum containing all 57 physical plan variants - Implemented backward compatibility for deserializing legacy typetag-formatted data
- Removed
#[typetag::serde]attributes from all physical plan implementations
Reviewed changes
Copilot reviewed 49 out of 49 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/query/service/src/physical_plans/physical_plan.rs | Core refactoring: added PhysicalPlanImpl enum, dispatch macros, backward compatibility layer, and tests |
| src/query/service/src/servers/flight/v1/packets/packet_fragment.rs | Made SerializedPhysicalPlanRef public for inclusion in PhysicalPlanImpl enum |
| src/query/service/src/servers/flight/v1/packets/mod.rs | Made packet_fragment module public to expose SerializedPhysicalPlanRef |
| src/query/service/src/physical_plans/physical_limit.rs | Removed #[typetag::serde], made meta field pub(crate) |
| src/query/service/src/physical_plans/physical_*.rs (40+ files) | Removed #[typetag::serde] from IPhysicalPlan implementations |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@codex review |
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
4f989cc to
42fc1fb
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. 🎉 ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
0390af6 to
47b2605
Compare
…are then aggregated into an enumeration at compile time to implement static dispatch of Serde without requiring centralized implementation.
47b2605 to
6435057
Compare
I hereby agree to the terms of the CLA available at: https://docs.databend.com/dev/policies/cla/
Summary
When using
#[typetag::serde]for polymorphic serialization, dynamic dispatching viaerased_serderesults in a deep call stack. This pr uses an enum to convert dynamic dispatching to static dispatching, reducing the call stack depth duringserdeserialization.PhysicalPlan after switching to enum serialization, it still supports deserialization of previous versions.
Call stack depth comparison
test from: typetag_serialize_stack_depth_is_measured
main:
typetag serialize stack depth: 41, delta from baseline: 14
pr:
typetag serialize stack depth: 36, delta from baseline: 9
Tests
Type of change
This change is