Skip to content

refactor: port HashExpr proto hooks#22502

Open
nanookclaw wants to merge 1 commit into
apache:mainfrom
nanookclaw:fix/hash-expr-proto-hooks
Open

refactor: port HashExpr proto hooks#22502
nanookclaw wants to merge 1 commit into
apache:mainfrom
nanookclaw:fix/hash-expr-proto-hooks

Conversation

@nanookclaw
Copy link
Copy Markdown

Which issue does this PR close?

Rationale for this change

HashExpr is part of the physical expression proto cleanup tracked by #22418. Its protobuf serialization should live with the expression implementation instead of in the central proto downcast chains, matching the hook pattern added in #21929.

What changes are included in this PR?

This adds HashExpr's PhysicalExpr::try_to_proto override and a feature-gated inherent HashExpr::try_from_proto. The existing PhysicalHashExprNode wire shape is preserved, including expr_id: None, while datafusion-proto now routes ExprType::HashExpr decoding through the hook and no longer has a central HashExpr serialization arm. datafusion-physical-plan now exposes a proto feature so the hook code only compiles when proto support is requested.

Are these changes tested?

Yes. Added focused direct hook tests for encoding, decoding, and rejecting a wrong expr_type.

Commands run:

  • cargo fmt --all -- --check
  • cargo check -p datafusion-physical-plan
  • cargo test -p datafusion-physical-plan --features proto hash_expr_try
  • cargo test -p datafusion-proto --test proto_integration cases::roundtrip_physical_plan::roundtrip_hash_expr
  • cargo check -p datafusion-proto
  • cargo clippy -p datafusion-physical-plan --features proto --all-targets -- -D warnings
  • cargo clippy -p datafusion-proto --all-targets -- -D warnings

Are there any user-facing changes?

No. This preserves the existing protobuf representation.

@github-actions github-actions Bot added proto Related to proto crate physical-plan Changes to the physical-plan crate labels May 24, 2026
@adriangb
Copy link
Copy Markdown
Contributor

@nanookclaw could you rebase and resolve conflicts please?

@nanookclaw nanookclaw force-pushed the fix/hash-expr-proto-hooks branch from 6defc35 to 768f68d Compare May 25, 2026 17:46
@nanookclaw
Copy link
Copy Markdown
Author

Rebased and resolved the conflicts. The new head is 768f68d50.

Validation run after the rebase:

  • cargo fmt --check -- datafusion/proto/src/physical_plan/from_proto.rs datafusion/proto/src/physical_plan/to_proto.rs
  • cargo test -p datafusion-physical-plan hash_expr_try --features proto --lib → 3 passed
  • cargo check -p datafusion-proto
  • git diff --check upstream/main...HEAD

Signed-off-by: Nanook Claw <nanook@agentmail.to>
@nanookclaw nanookclaw force-pushed the fix/hash-expr-proto-hooks branch from 768f68d to 20c2832 Compare May 26, 2026 19:56
@nanookclaw
Copy link
Copy Markdown
Author

Rebased fix/hash-expr-proto-hooks onto current main and force-pushed head 20c2832988511969334e9ec4b189e1952365c3d3.

Conflict resolution kept upstream's HashTableLookupExpr proto hook behavior and kept this PR's HashExpr hook migration, so HashExpr now goes through try_to_proto / try_from_proto rather than the central to_proto downcast branch.

Local validation passed:

  • cargo fmt --check -- datafusion/proto/src/physical_plan/from_proto.rs datafusion/proto/src/physical_plan/to_proto.rs datafusion/physical-plan/src/joins/hash_join/partitioned_hash_eval.rs
  • cargo test -p datafusion-physical-plan hash_expr_try --features proto --lib
  • cargo check -p datafusion-proto
  • git diff --check upstream/main...HEAD

GitHub checks have restarted on the new head.

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 proto Related to proto crate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Port HashExpr to use try_to_proto / try_from_proto

2 participants