Skip to content

refactor(proto): destructure plan and proto structs in join serde hooks - #24164

Open
adriangb wants to merge 6 commits into
apache:mainfrom
pydantic:refactor/proto-destructure-joins
Open

refactor(proto): destructure plan and proto structs in join serde hooks#24164
adriangb wants to merge 6 commits into
apache:mainfrom
pydantic:refactor/proto-destructure-joins

Conversation

@adriangb

@adriangb adriangb commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Rationale for this change

EPIC #23494 moved every built-in ExecutionPlan off the central downcast_ref chain
in datafusion-proto onto per-plan hooks: a try_to_proto override inside
impl ExecutionPlan for FooExec, and an inherent FooExec::try_from_proto.

Those hooks read plan state through getters (self.filter(), self.join_type(), …).
That makes adding a field to a plan struct invisible to serialization — nothing breaks
at compile time, the new field is just silently not serialized, and the bug only shows
up as state quietly disappearing on a round-trip.

This is not hypothetical. HashJoinExec.fetch was dropped on round-trip for exactly
this reason (fixed in #24165) — the field was added to the struct and the encoder,
still calling getters, never learned about it.

Destructuring removes the failure mode. If try_to_proto starts with an exhaustive
let Self { … } = self; with no .., adding a field to the plan struct is a compile
error in the encoder. If try_from_proto destructures the prost-generated node struct the
same way, adding a field to the .proto is a compile error in every decoder. The prost
structs are plain, all-pub, and not #[non_exhaustive], so this compiles today.

What changes are included in this PR?

Applies the pattern to the join plans, one commit per plan:

  • CrossJoinExec
  • NestedLoopJoinExec
  • SortMergeJoinExec
  • SymmetricHashJoinExec
  • HashExpr / HashTableLookupExpr (joins/hash_join/partitioned_hash_eval.rs)
  • HashJoinExec

Fields that genuinely are not serialized bind to _ and carry a short comment saying
why — derived at construction, runtime state, or recomputed by new/try_new/the builder
on decode. For example:

let Self {
    left,
    right,
    on,
    filter,
    join_type,
    sort_options,
    null_equality,
    // derived from the children's schemas by `try_new` on decode
    schema: _,
    // runtime metrics, not part of the plan
    metrics: _,
    // recomputed from `on` and `sort_options` by `try_new` on decode
    left_sort_exprs: _,
    // recomputed from `on` and `sort_options` by `try_new` on decode
    right_sort_exprs: _,
    // recomputed by `try_new` on decode
    cache: _,
} = self;

HashTableLookupExpr::try_to_proto deliberately serializes none of its state — it holds a
runtime Arc<Map> and is replaced with lit(true). It gets a destructure with every field
bound to _, so that adding a field there forces a decision instead of passing unnoticed.

For HashJoinExec, the serialized bindings are left, right, on, filter,
join_type, mode, projection, null_equality, null_aware, dynamic_filter and
fetch. The six ignored fields are join_schema, left_fut, random_state, metrics,
column_indices and cacherandom_state is the fixed HASH_JOIN_SEED constant that
the builder sets identically on decode, and the rest are runtime state or recomputed by the
builder.

No additional unserialized fields were found. Every field on the join plans that is
not written to the proto is genuinely derived, recomputed on decode, or runtime-only.

This is a pure refactor. The wire format is byte-for-byte unchanged and no behavior changes.
Every getter that was replaced returns exactly the field it was replaced with. The only
incidental changes are in SymmetricHashJoinExec::try_from_proto and
HashJoinExec::try_from_proto, where four internal_datafusion_err! calls switched to
inlined format args now that the field is bound locally; the message text they produce is
identical.

Are these changes tested?

Covered by the existing round-trip tests — that is the point of the change: if the
destructures had drifted from the encoders, the plans would fail to round-trip.

Run locally:

  • cargo test -p datafusion-proto --test proto_integration — 215 passed, including all
    five hash-join round-trip tests (roundtrip_hash_join, roundtrip_hash_join_fetch,
    roundtrip_hash_join_projection_states, test_hash_join_with_dynamic_filter_roundtrip,
    roundtrip_sym_hash_join)
  • cargo test -p datafusion-physical-plan — 1641 passed
  • cargo clippy --all-targets --workspace --features avro,integration-tests,extended_tests -- -D warnings
  • cargo fmt --all

Are there any user-facing changes?

No.

@codecov-commenter

codecov-commenter commented Aug 7, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 79.10448% with 42 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.05%. Comparing base (fc846dd) to head (f9b5571).

Files with missing lines Patch % Lines
...ion/physical-plan/src/joins/symmetric_hash_join.rs 77.58% 4 Missing and 9 partials ⚠️
datafusion/physical-plan/src/joins/cross_join.rs 0.00% 9 Missing ⚠️
...tafusion/physical-plan/src/joins/hash_join/exec.rs 86.20% 2 Missing and 6 partials ⚠️
...fusion/physical-plan/src/joins/nested_loop_join.rs 80.76% 0 Missing and 5 partials ⚠️
...on/physical-plan/src/joins/sort_merge_join/exec.rs 84.84% 0 Missing and 5 partials ⚠️
...-plan/src/joins/hash_join/partitioned_hash_eval.rs 88.23% 0 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #24164      +/-   ##
==========================================
- Coverage   81.05%   81.05%   -0.01%     
==========================================
  Files        1107     1107              
  Lines      381574   381612      +38     
  Branches   381574   381612      +38     
==========================================
+ Hits       309281   309308      +27     
+ Misses      54034    54029       -5     
- Partials    18259    18275      +16     

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

@adriangb
adriangb force-pushed the refactor/proto-destructure-joins branch 2 times, most recently from 78fe3c2 to 36a7d74 Compare August 7, 2026 16:39
@github-actions github-actions Bot added proto Related to proto crate auto detected api change Auto detected API change labels Aug 7, 2026
adriangb and others added 6 commits August 7, 2026 13:58
…serde

Reading plan state through getters means a newly added struct field is
invisible to serialization: nothing breaks, the field is just silently
not serialized. Destructuring `self` exhaustively (no `..`) in
`try_to_proto`, and the prost-generated node struct exhaustively in
`try_from_proto`, turns that into a compile error in both directions.

Fields that genuinely are not serialized bind to `_` with a comment
saying why. Pure refactor: the wire format is unchanged.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…Exec serde

Same treatment as CrossJoinExec: exhaustive `let Self { .. }` (no `..`)
in `try_to_proto` and exhaustive destructure of
`protobuf::NestedLoopJoinExecNode` in `try_from_proto`, so adding a
field on either side is a compile error rather than a silent omission.

Pure refactor: the wire format is unchanged.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…xec serde

Exhaustive `let Self { .. }` (no `..`) in `try_to_proto` and exhaustive
destructure of `protobuf::SortMergeJoinExecNode` in `try_from_proto`.
`left_sort_exprs` / `right_sort_exprs` are recomputed by `try_new` from
`on` and `sort_options`, so they bind to `_` with a comment.

Pure refactor: the wire format is unchanged.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…oinExec serde

Exhaustive `let Self { .. }` (no `..`) in `try_to_proto` and exhaustive
destructure of `protobuf::SymmetricHashJoinExecNode` in
`try_from_proto`. The error messages now use inlined format args since
the field is bound locally; the text they produce is identical.

Pure refactor: the wire format is unchanged.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… serde

`HashExpr` gets the same treatment as the join plans: exhaustive
`let Self { .. }` in `try_to_proto` and exhaustive destructure of
`protobuf::PhysicalHashExprNode` in `try_from_proto`.

`HashTableLookupExpr::try_to_proto` deliberately serializes none of its
state -- it holds a runtime `Arc<Map>` and is replaced with `lit(true)`.
Destructuring `self` there with every field bound to `_` documents that
and forces a decision if a field is ever added.

Pure refactor: the wire format is unchanged.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…erde

Completes the pass over the join plans. `HashJoinExec` was held back to
avoid conflicting with the `fetch` fix; now that this branch is based on
it, `fetch` is a real serialized field and gets bound like the rest.

Exhaustive `let Self { .. }` (no `..`) in `try_to_proto` and exhaustive
destructure of `protobuf::HashJoinExecNode` in `try_from_proto`.
`dynamic_filter` is now read off the bound field instead of through
`dynamic_filter_expr()`. The six ignored fields are `join_schema`,
`left_fut`, `random_state`, `metrics`, `column_indices` and `cache`;
each binds to `_` with a comment saying why. Every getter that was
replaced returns exactly the field it was replaced with, so the wire
format is unchanged.

One incidental change: the `unknown PartitionMode` error switched to an
inlined format arg now that the field is bound locally; the message text
it produces is identical.

Pure refactor: the wire format is unchanged.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@adriangb
adriangb force-pushed the refactor/proto-destructure-joins branch from dedb5cf to f9b5571 Compare August 7, 2026 19:05
@github-actions github-actions Bot removed proto Related to proto crate auto detected api change Auto detected API change labels Aug 7, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

physical-plan Changes to the physical-plan crate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants