Skip to content

perf(proto): avoid re-normalizing logical plans - #25297

Open
AnuragRaut08 wants to merge 1 commit into
apache:mainfrom
AnuragRaut08:fix/proto-direct-logical-plan-decode
Open

AnuragRaut08 wants to merge 1 commit into
apache:mainfrom
AnuragRaut08:fix/proto-direct-logical-plan-decode

Conversation

@AnuragRaut08

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Closes #24777

Rationale for this change

Deserializing wide logical plans with datafusion-proto can take disproportionately longer as the number of projected expressions increases. This is because deserialization re-runs expression normalization on logical plan nodes that have already been normalized before serialization.

Avoiding this redundant work makes logical plan deserialization more efficient, particularly for wide plans.

What changes are included in this PR?

Decode Projection, Filter, Window, Aggregate, and Sort logical plan nodes directly through their constructors instead of rebuilding them through LogicalPlanBuilder.

This avoids the redundant expression normalization performed by the builder methods while preserving the serialized logical plan structure.

This implements the constructor-based approach described as fix (2) in #24777 and is complementary to #25010, which optimizes the normalization work itself.

What is the testing strategy for this PR?

The change is covered by the existing logical plan protobuf roundtrip tests, which verify that the decoded plans remain equivalent to the serialized plans.

The following tests were run successfully:

  • cargo check -p datafusion-proto
  • cargo test -p datafusion-proto --test proto_integration roundtrip_logical_plan
  • cargo test -p datafusion-proto --test proto_integration roundtrip_logical_plan_aggregation
  • cargo test -p datafusion-proto --test proto_integration roundtrip_logical_plan_sort
  • cargo test -p datafusion-proto --test proto_integration roundtrip_window

No new tests were added because the existing protobuf logical-plan roundtrip coverage exercises the affected decode paths.

The full proto_integration test suite was also run. It had 254 passing tests and 7 failures caused by missing Parquet test data from the parquet-testing submodule; these failures are unrelated to this change.

Are there any user-facing changes?

No.

@codecov-commenter

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 76.47059% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.91%. Comparing base (85d4cbb) to head (fdc6d94).
⚠️ Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
datafusion/proto/src/logical_plan/mod.rs 76.47% 2 Missing and 2 partials ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main   #25297   +/-   ##
=======================================
  Coverage   81.91%   81.91%           
=======================================
  Files        1134     1134           
  Lines      425631   425711   +80     
  Branches   425631   425711   +80     
=======================================
+ Hits       348647   348720   +73     
- Misses      56304    56310    +6     
- Partials    20680    20681    +1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@AnuragRaut08

Copy link
Copy Markdown
Contributor Author

The lower patch coverage here is mainly due to the direct-construction branches not all being exercised by a single focused test path. The existing logical-plan roundtrip coverage does exercise the affected Projection, Filter, Window, Aggregate, and Sort decode paths, and those tests pass with this change.

I also ran the full datafusion-proto integration test target: 254 tests passed. The 7 failures are unrelated Parquet tests caused by the missing parquet-testing test data/submodule.

I don't think adding coverage-only tests would provide meaningful additional validation for this change; the important behavior is already covered by the existing logical-plan roundtrip tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

proto Related to proto crate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

datafusion-proto: logical plan decode re-normalizes already-normalized plans, superlinear on wide plans

2 participants