Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Draft
KKould wants to merge5 commits intodatabendlabs:main
base:main
Choose a base branch
Loading
fromKKould:perf/physical_paln_serde_deep_stack

Conversation

@KKould
Copy link
Member

@KKouldKKould commentedDec 17, 2025•
edited
Loading

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_serde results in a deep call stack. This pr uses an enum to convert dynamic dispatching to static dispatching, reducing the call stack depth duringserde 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: 37, delta from baseline: 10

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

@KKouldKKould self-assigned thisDec 17, 2025
@KKouldKKould changed the titleperf: 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-actionsgithub-actionsbot added the pr-featurethis PR introduces a new feature to the codebase labelDec 17, 2025
Copy link
Contributor

CopilotAI 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:

  • IntroducedPhysicalPlanImpl 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
FileDescription
src/query/service/src/physical_plans/physical_plan.rsCore refactoring: added PhysicalPlanImpl enum, dispatch macros, backward compatibility layer, and tests
src/query/service/src/servers/flight/v1/packets/packet_fragment.rsMade SerializedPhysicalPlanRef public for inclusion in PhysicalPlanImpl enum
src/query/service/src/servers/flight/v1/packets/mod.rsMade packet_fragment module public to expose SerializedPhysicalPlanRef
src/query/service/src/physical_plans/physical_limit.rsRemoved #[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
MemberAuthor

@codex review

@chatgpt-codex-connector

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

@github-actions
Copy link
Contributor

github-actionsbot commentedDec 17, 2025•
edited
Loading

🤖 CI Job Analysis

Workflow:20297127831

📊 Summary

  • Total Jobs: 83
  • Failed Jobs: 23
  • Retryable: 0
  • Code Issues: 23

❌NO RETRY NEEDED

All failures appear to be code/test issues requiring manual fixes.

🔍 Job Details

  • ❌linux / test_private_tasks: Not retryable (Code/Test)
  • ❌linux / test_logs: Not retryable (Code/Test)
  • ❌linux / test_compat_client_cluster: Not retryable (Code/Test)
  • ❌linux / test_stateful_cluster: Not retryable (Code/Test)
  • ❌linux / test_stateless_cluster: Not retryable (Code/Test)
  • ❌linux / sqllogic / cluster_with_minio_and_nginx (http_handler, ttc-rust): Not retryable (Code/Test)
  • ❌linux / sqllogic / cluster_with_minio_and_nginx (http_handler, ttc-go): Not retryable (Code/Test)
  • ❌linux / sqllogic / cluster (query, 4c16g, http): Not retryable (Code/Test)
  • ❌linux / sqllogic / cluster (ydb, 2c8g, http): Not retryable (Code/Test)
  • ❌linux / sqllogic / cluster (duckdb, 4c16g, http): Not retryable (Code/Test)
  • ❌linux / sqllogic / cluster (crdb, 2c8g, 2, http): Not retryable (Code/Test)
  • ❌linux / sqllogic / cluster (crdb, 2c8g, 2, hybrid): Not retryable (Code/Test)
  • ❌linux / sqllogic / cluster (cluster, 2c8g, hybrid): Not retryable (Code/Test)
  • ❌linux / sqllogic / cluster (query, 4c16g, hybrid): Not retryable (Code/Test)
  • ❌linux / sqllogic / cluster (cluster, 2c8g, http): Not retryable (Code/Test)
  • ❌linux / sqllogic / cluster (duckdb, 4c16g, hybrid): Not retryable (Code/Test)
  • ❌linux / sqllogic / cluster (ydb, 2c8g, hybrid): Not retryable (Code/Test)
  • ❌linux / sqllogic / cluster (tpcds, 2c8g, http): Not retryable (Code/Test)
  • ❌linux / sqllogic / cluster (tpch, 2c8g, http): Not retryable (Code/Test)
  • ❌linux / sqllogic / cluster (base, 2c8g, 2, hybrid): Not retryable (Code/Test)
  • ❌linux / sqllogic / cluster (base, 2c8g, 2, http): Not retryable (Code/Test)
  • ❌linux / sqllogic / cluster (tpcds, 2c8g, hybrid): Not retryable (Code/Test)
  • ❌linux / sqllogic / cluster (tpch, 2c8g, hybrid): Not retryable (Code/Test)
🤖 About

Automated analysis using job annotations to distinguish infrastructure issues (auto-retried) from code/test issues (manual fixes needed).

@KKouldKKould changed the titlefeat: 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
…are then aggregated into an enumeration at compile time to implement static dispatch of Serde without requiring centralized implementation.
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

Copilot code reviewCopilotCopilot left review comments

@sundy-lisundy-liAwaiting requested review from sundy-li

@zhang2014zhang2014Awaiting requested review from zhang2014

At least 1 approving review is required to merge this pull request.

Assignees

@KKouldKKould

Labels

pr-featurethis PR introduces a new feature to the codebase

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@KKould
[8]ページ先頭
©2009-2025 Movatter.jp