adriangb opened a new pull request, #22596:
URL: https://github.com/apache/datafusion/pull/22596
## Which issue does this PR close?
Part of #22418 / follow-up to #22513.
## Rationale for this change
Every PR migrating a `PhysicalExpr` to `try_to_proto` / `try_from_proto`
under #22418 re-implements the same three shapes that don't fit the existing
helpers from #22513:
1. The outer `match &node.expr_type { ... }` that opens every
`try_from_proto`:
```rust
let try_cast = match &node.expr_type {
Some(protobuf::physical_expr_node::ExprType::TryCast(x)) =>
x.as_ref(),
_ => return internal_err!(\"PhysicalExprNode is not a TryCastExpr\"),
};
```
2. The `PhysicalExprNode { expr_id: None, expr_type: Some(_) }` wrapper
every `try_to_proto` returns.
3. The hand-rolled \"missing required field 'X'\" error for non-expression
fields like `arrow_type` on `CastExpr` / `TryCastExpr`.
Each shape leaks across the 7+ remaining open migration PRs. Adding small
helpers in `physical-expr-common` keeps the per-expression diff minimal and the
error messages consistent.
## What changes are included in this PR?
**Commit 1 — `feat(physical-expr-common): add proto helpers ...`**
Three new helpers in `datafusion-physical-expr-common`, all gated on
`feature = \"proto\"`:
- `expect_expr_variant!` macro (re-exported at crate root) — matches
`Option<ExprType>`, returns inner payload, errors with `\"PhysicalExprNode is
not a {variant}\"`.
- `proto_encode::expr_node(expr_type)` — builds a `PhysicalExprNode` with
`expr_id: None` from an `ExprType`. `expr_id` is a dedup tag used only by
`DynamicFilterPhysicalExpr` (#22434); every other expression's `try_to_proto`
reduces to `expr_node(ExprType::Foo(_))`.
- `proto_decode::require_proto_field<T>(opt, expr_name, field)` — mirrors
`decode_required_expression` for non-`PhysicalExprNode` fields.
Six unit tests cover the helpers (success + the two reject paths for the
macro).
**Commit 2 — `refactor(physical-expr): adopt new proto helpers in
already-migrated expressions`**
Ports every expression already on the new hooks:
- `Column`, `BinaryExpr` (originally #21929)
- `LikeExpr` (#22471)
- `InListExpr` (#22503)
- `NegativeExpr` (#22483)
- `HashTableLookupExpr` (#22451)
`BinaryExpr` additionally adopts `decode_required_expression` for its legacy
`l`/`r` arms and `encode_children_expressions` / `decode_children_expressions`
for the linearized `operands` path, removing two more hand-rolled \"missing
required field\" strings.
One existing test changes assertion text — `InListExpr`'s rejected-variant
message was the only one using the article \"an\" instead of \"a\"; the macro
emits article-free \"a {Variant}\" uniformly.
The two commits are stacked for review: commit 1 is the helper addition
only; commit 2 is the adoption. Either can be reviewed in isolation.
## Are these changes tested?
Yes:
- `cargo test -p datafusion-physical-expr-common --features proto` — new
helper unit tests pass.
- `cargo test -p datafusion-physical-expr --features proto proto_tests` — 23
/ 23 per-expression proto tests pass (1 assertion-string update in InList).
- `cargo test -p datafusion-proto --test proto_integration` — 173 / 173
pass; no wire-format change.
- `cargo clippy --all-targets --all-features -- -D warnings` clean on the
touched crates.
## Are there any user-facing changes?
No. New API surface in `datafusion-physical-expr-common` (helpers gated on
`feature = \"proto\"`); no change to serialized output. The macro
`expect_expr_variant!` is exported at the crate root.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
