Skip to content

Port LikeExpr to use try_to_proto / try_from_proto #22431

@adriangb

Description

@adriangb

Sub-issue of #22418. Port LikeExpr to own its protobuf (de)serialization via the try_to_proto / try_from_proto hooks added in #21929, removing it from the central downcast chain.

Reference: copy the Column migration in #21929. Mind the asymmetry — try_to_proto is a trait override, try_from_proto is an inherent fn (see steps).

Where LikeExpr lives

datafusion/physical-expr/src/expressions/like.rs (crate datafusion-physical-expr).

Steps

  1. Add try_to_proto inside the impl PhysicalExpr for LikeExpr block — it overrides a trait method, so it must live there, feature-gated #[cfg(feature = "proto")].
    ⚠️ Do not put it in an inherent impl LikeExpr block. The serializer calls it through &dyn PhysicalExpr, so an inherent method is silently never reached and serialization keeps using the old chain. This is the most common review bounce on these PRs.
  2. Add LikeExpr::try_from_proto(node, ctx) as an inherent associated fn (#[cfg(feature = "proto")] impl LikeExpr) and wire it into the matching ExprType::… arm in datafusion/proto/src/physical_plan/from_proto.rs.
  3. In this same PR, delete the LikeExpr arm from datafusion/proto/src/physical_plan/to_proto.rs and the old ExprType::… decode arm in from_proto.rs. This is not cleanup-for-later — it is the only thing that proves your hook is actually reached. If the roundtrip tests still pass after deleting the arm, it works; if you leave the arm, the tests pass through dead code.
  4. Keep the wire format byte-for-byte identical. expr_id is None here (only DynamicFilterPhysicalExpr sets it).
  5. Keep use datafusion_proto_models::protobuf; inside the method bodies (matches the Column reference).

Tests

  • Existing roundtrip_physical_plan / roundtrip_physical_expr must still pass: cargo test -p datafusion-proto --test proto_integration.
  • Add a direct hook test in datafusion/physical-expr/src/expressions/like.rs covering both directions and a bad-input case (wrong expr_type variant / missing child → clean error, not panic). Reviewers have asked for this on every reviewed PR.
  • Put #[cfg(test)] mod tests at the end of the file or clippy -D warnings fails (items-after-test-module).

Before you push

  • Fill in every section of the PR template.
  • cargo fmt --all and cargo clippy --all-targets --all-features -- -D warnings must pass.

Checklist

  • try_to_proto in impl PhysicalExpr for LikeExpr (not inherent), #[cfg(feature="proto")]
  • LikeExpr::try_from_proto wired into from_proto.rs
  • central to_proto.rs + from_proto.rs arms deleted in this PR
  • direct hook test incl. bad-input case; test module at end of file
  • roundtrip tests pass; fmt + clippy -D warnings clean; PR template filled in

Metadata

Metadata

Assignees

Labels

Type

No type
No fields configured for issues without a type.

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions