perf(proto): avoid re-normalizing logical plans - #25297
AnuragRaut08 wants to merge 1 commit into
Conversation
Codecov Report❌ Patch coverage is
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. 🚀 New features to boost your workflow:
|
|
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 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. |
Which issue does this PR close?
Closes #24777
Rationale for this change
Deserializing wide logical plans with
datafusion-protocan 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, andSortlogical plan nodes directly through their constructors instead of rebuilding them throughLogicalPlanBuilder.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-protocargo test -p datafusion-proto --test proto_integration roundtrip_logical_plancargo test -p datafusion-proto --test proto_integration roundtrip_logical_plan_aggregationcargo test -p datafusion-proto --test proto_integration roundtrip_logical_plan_sortcargo test -p datafusion-proto --test proto_integration roundtrip_windowNo new tests were added because the existing protobuf logical-plan roundtrip coverage exercises the affected decode paths.
The full
proto_integrationtest suite was also run. It had 254 passing tests and 7 failures caused by missing Parquet test data from theparquet-testingsubmodule; these failures are unrelated to this change.Are there any user-facing changes?
No.