Skip to content

Conversation

@KKould
Copy link
Member

@KKould KKould commented Dec 17, 2025

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 via erased_serde results in a deep call stack. This pr uses an enum to convert dynamic dispatching to static dispatching, reducing the call stack depth during serde serialization.

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

  • Unit Test
  • Logic Test
  • Benchmark Test
  • No Test - Explain why

Type of change

  • Bug Fix (non-breaking change which fixes an issue)
  • New Feature (non-breaking change which adds functionality)
  • Breaking Change (fix or feature that could cause existing functionality not to work as expected)
  • Documentation Update
  • Refactoring
  • Performance Improvement
  • Other (please describe):

This change is Reviewable

@KKould KKould self-assigned this Dec 17, 2025
@KKould KKould changed the title perf: remove #[typetag::serde], Static dispatch implementation of serde using enum. feat: remove #[typetag::serde], Static dispatch implementation of serde using enum. Dec 17, 2025
@github-actions github-actions bot added the pr-feature this PR introduces a new feature to the codebase label Dec 17, 2025
Copy link
Contributor

Copilot AI left a 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 PhysicalPlanImpl enum 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.

@KKould
Copy link
Member Author

KKould commented Dec 17, 2025

@codex review

@chatgpt-codex-connector
Copy link

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.
Repo admins can enable using credits for code reviews in their settings.

@KKould KKould changed the title feat: remove #[typetag::serde], Static dispatch implementation of serde using enum. feat: remove #[typetag::serde] on PhysicalPlan, Static dispatch implementation of serde using enum. Dec 17, 2025
@KKould KKould force-pushed the perf/physical_paln_serde_deep_stack branch from 4f989cc to 42fc1fb Compare December 18, 2025 06:25
@KKould KKould marked this pull request as ready for review December 18, 2025 07:53
@KKould
Copy link
Member Author

KKould commented Dec 18, 2025

@codex review

@chatgpt-codex-connector
Copy link

Codex Review: Didn't find any major issues. 🎉

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

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".

@KKould KKould force-pushed the perf/physical_paln_serde_deep_stack branch 2 times, most recently from 0390af6 to 47b2605 Compare December 22, 2025 08:40
PhysicalPlanSerdeSerialize and PhysicalPlanDeserialize are only used in Serialize and Deserialize
@KKould KKould force-pushed the perf/physical_paln_serde_deep_stack branch from 47b2605 to 6435057 Compare December 23, 2025 01:45
@KKould KKould requested a review from zhang2014 December 23, 2025 16:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr-feature this PR introduces a new feature to the codebase

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants