Skip to content

Commit d2d0357

Browse files
jx2leeadriangbclaude
authored
Port LikeExpr to use try_to_proto / try_from_proto (apache#22471)
## Which issue does this PR close? - Closes apache#22431. ## Rationale for this change `LikeExpr` still serialized through the central physical proto conversion chain. This change moves serialization and deserialization responsibility into the expression type itself, following the established migration pattern used for `Column` and `BinaryExpr`. ## What changes are included in this PR? - Add `LikeExpr::try_to_proto` - Add `LikeExpr::try_from_proto` - Route physical proto decode through `LikeExpr::try_from_proto` - Remove the central `LikeExpr` encode and decode branches - Keep behavior and wire shape unchanged - Rely on existing physical proto roundtrip coverage instead of an extra expression-local roundtrip test ## Are these changes tested? Yes. Existing physical proto roundtrip coverage already covers `LikeExpr`. Verification run: - `cargo fmt --all` - `cargo test -p datafusion-physical-expr --features proto` - `cargo check -p datafusion-proto --all-features` - `cargo clippy -p datafusion-physical-expr -p datafusion-proto --all-targets --all-features -- -D warnings` ## Are there any user-facing changes? No. --------- Co-authored-by: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 525e01a commit d2d0357

5 files changed

Lines changed: 393 additions & 35 deletions

File tree

datafusion/physical-expr/src/expressions/like.rs

Lines changed: 247 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -145,6 +145,69 @@ impl PhysicalExpr for LikeExpr {
145145
write!(f, " {} ", self.op_name())?;
146146
self.pattern.fmt_sql(f)
147147
}
148+
149+
#[cfg(feature = "proto")]
150+
fn try_to_proto(
151+
&self,
152+
ctx: &datafusion_physical_expr_common::physical_expr::proto_encode::PhysicalExprEncodeCtx<'_>,
153+
) -> Result<Option<datafusion_proto_models::protobuf::PhysicalExprNode>> {
154+
use datafusion_proto_models::protobuf;
155+
156+
Ok(Some(protobuf::PhysicalExprNode {
157+
expr_id: None,
158+
expr_type: Some(protobuf::physical_expr_node::ExprType::LikeExpr(Box::new(
159+
protobuf::PhysicalLikeExprNode {
160+
negated: self.negated,
161+
case_insensitive: self.case_insensitive,
162+
expr: Some(Box::new(ctx.encode_child(&self.expr)?)),
163+
pattern: Some(Box::new(ctx.encode_child(&self.pattern)?)),
164+
},
165+
))),
166+
}))
167+
}
168+
}
169+
170+
#[cfg(feature = "proto")]
171+
impl LikeExpr {
172+
/// Reconstruct a [`LikeExpr`] from its protobuf representation.
173+
///
174+
/// Takes the whole [`PhysicalExprNode`] so the decode signature matches
175+
/// other migrated expressions and can inspect outer-node metadata if
176+
/// needed in the future.
177+
///
178+
/// [`PhysicalExprNode`]: datafusion_proto_models::protobuf::PhysicalExprNode
179+
pub fn try_from_proto(
180+
node: &datafusion_proto_models::protobuf::PhysicalExprNode,
181+
ctx: &datafusion_physical_expr_common::physical_expr::proto_decode::PhysicalExprDecodeCtx<'_>,
182+
) -> Result<Arc<dyn PhysicalExpr>> {
183+
use datafusion_common::internal_err;
184+
use datafusion_proto_models::protobuf;
185+
186+
let like_expr = match &node.expr_type {
187+
Some(protobuf::physical_expr_node::ExprType::LikeExpr(like_expr)) => {
188+
like_expr.as_ref()
189+
}
190+
_ => return internal_err!("PhysicalExprNode is not a LikeExpr"),
191+
};
192+
193+
let expr = like_expr.expr.as_deref().ok_or_else(|| {
194+
datafusion_common::DataFusionError::Internal(
195+
"LikeExpr is missing required field 'expr'".to_string(),
196+
)
197+
})?;
198+
let pattern = like_expr.pattern.as_deref().ok_or_else(|| {
199+
datafusion_common::DataFusionError::Internal(
200+
"LikeExpr is missing required field 'pattern'".to_string(),
201+
)
202+
})?;
203+
204+
Ok(Arc::new(LikeExpr::new(
205+
like_expr.negated,
206+
like_expr.case_insensitive,
207+
ctx.decode(expr)?,
208+
ctx.decode(pattern)?,
209+
)))
210+
}
148211
}
149212

150213
/// used for optimize Dictionary like
@@ -283,3 +346,187 @@ mod test {
283346
Ok(())
284347
}
285348
}
349+
350+
/// Tests for the `try_to_proto` / `try_from_proto` hooks.
351+
#[cfg(all(test, feature = "proto"))]
352+
mod proto_tests {
353+
use super::*;
354+
use crate::expressions::{Column, col};
355+
use crate::proto_test_util::{
356+
StubDecoder, StubEncoder, UnreachableDecoder, column_node,
357+
};
358+
use arrow::datatypes::Field;
359+
use datafusion_common::DataFusionError;
360+
use datafusion_physical_expr_common::physical_expr::proto_decode::PhysicalExprDecodeCtx;
361+
use datafusion_physical_expr_common::physical_expr::proto_encode::PhysicalExprEncodeCtx;
362+
use datafusion_proto_models::protobuf::{
363+
PhysicalExprNode, PhysicalLikeExprNode, physical_expr_node,
364+
};
365+
366+
/// Build a `LikeExpr` proto node with the given children.
367+
fn like_node(
368+
negated: bool,
369+
case_insensitive: bool,
370+
expr: Option<Box<PhysicalExprNode>>,
371+
pattern: Option<Box<PhysicalExprNode>>,
372+
) -> PhysicalExprNode {
373+
PhysicalExprNode {
374+
expr_id: None,
375+
expr_type: Some(physical_expr_node::ExprType::LikeExpr(Box::new(
376+
PhysicalLikeExprNode {
377+
negated,
378+
case_insensitive,
379+
expr,
380+
pattern,
381+
},
382+
))),
383+
}
384+
}
385+
386+
/// A `LikeExpr` over two `Utf8` columns with both flags set, so the
387+
/// `negated` / `case_insensitive` wiring is actually exercised.
388+
fn like_fixture() -> LikeExpr {
389+
let schema = Schema::new(vec![
390+
Field::new("a", DataType::Utf8, false),
391+
Field::new("b", DataType::Utf8, false),
392+
]);
393+
LikeExpr::new(
394+
true,
395+
true,
396+
col("a", &schema).unwrap(),
397+
col("b", &schema).unwrap(),
398+
)
399+
}
400+
401+
#[test]
402+
fn try_to_proto_encodes_like_expr() {
403+
let like = like_fixture();
404+
let encoder = StubEncoder::ok();
405+
let ctx = PhysicalExprEncodeCtx::new(&encoder);
406+
407+
let node = like
408+
.try_to_proto(&ctx)
409+
.unwrap()
410+
.expect("LikeExpr should encode to Some(node)");
411+
412+
// Built-in exprs never set expr_id; only dynamic filters do.
413+
assert!(node.expr_id.is_none());
414+
let like_node = match node.expr_type {
415+
Some(physical_expr_node::ExprType::LikeExpr(boxed)) => *boxed,
416+
other => panic!("expected a LikeExpr node, got {other:?}"),
417+
};
418+
assert!(like_node.negated);
419+
assert!(like_node.case_insensitive);
420+
assert!(like_node.expr.is_some());
421+
assert!(like_node.pattern.is_some());
422+
}
423+
424+
#[test]
425+
fn try_to_proto_propagates_expr_encode_error() {
426+
let like = like_fixture();
427+
let encoder = StubEncoder::failing_on(1);
428+
let ctx = PhysicalExprEncodeCtx::new(&encoder);
429+
let err = like.try_to_proto(&ctx).unwrap_err();
430+
assert!(matches!(err, DataFusionError::Internal(msg) if msg.contains("call 1")));
431+
}
432+
433+
#[test]
434+
fn try_to_proto_propagates_pattern_encode_error() {
435+
let like = like_fixture();
436+
let encoder = StubEncoder::failing_on(2);
437+
let ctx = PhysicalExprEncodeCtx::new(&encoder);
438+
let err = like.try_to_proto(&ctx).unwrap_err();
439+
assert!(matches!(err, DataFusionError::Internal(msg) if msg.contains("call 2")));
440+
}
441+
442+
#[test]
443+
fn try_from_proto_decodes_like_expr() {
444+
let node = like_node(
445+
true,
446+
true,
447+
Some(Box::new(column_node("a"))),
448+
Some(Box::new(column_node("b"))),
449+
);
450+
let schema = Schema::empty();
451+
let decoder = StubDecoder::ok();
452+
let ctx = PhysicalExprDecodeCtx::new(&schema, &decoder);
453+
454+
let decoded = LikeExpr::try_from_proto(&node, &ctx).unwrap();
455+
let like = decoded
456+
.downcast_ref::<LikeExpr>()
457+
.expect("decoded expr should be a LikeExpr");
458+
assert!(like.negated());
459+
assert!(like.case_insensitive());
460+
assert!(like.expr().downcast_ref::<Column>().is_some());
461+
assert!(like.pattern().downcast_ref::<Column>().is_some());
462+
}
463+
464+
#[test]
465+
fn try_from_proto_rejects_non_like_node() {
466+
let node = column_node("a");
467+
let schema = Schema::empty();
468+
let decoder = UnreachableDecoder;
469+
let ctx = PhysicalExprDecodeCtx::new(&schema, &decoder);
470+
let err = LikeExpr::try_from_proto(&node, &ctx).unwrap_err();
471+
assert!(matches!(
472+
err,
473+
DataFusionError::Internal(msg) if msg.contains("PhysicalExprNode is not a LikeExpr")
474+
));
475+
}
476+
477+
#[test]
478+
fn try_from_proto_rejects_missing_expr() {
479+
let node = like_node(false, false, None, Some(Box::new(column_node("b"))));
480+
let schema = Schema::empty();
481+
let decoder = UnreachableDecoder;
482+
let ctx = PhysicalExprDecodeCtx::new(&schema, &decoder);
483+
let err = LikeExpr::try_from_proto(&node, &ctx).unwrap_err();
484+
assert!(matches!(
485+
err,
486+
DataFusionError::Internal(msg) if msg.contains("LikeExpr is missing required field 'expr'")
487+
));
488+
}
489+
490+
#[test]
491+
fn try_from_proto_rejects_missing_pattern() {
492+
let node = like_node(false, false, Some(Box::new(column_node("a"))), None);
493+
let schema = Schema::empty();
494+
let decoder = UnreachableDecoder;
495+
let ctx = PhysicalExprDecodeCtx::new(&schema, &decoder);
496+
let err = LikeExpr::try_from_proto(&node, &ctx).unwrap_err();
497+
assert!(matches!(
498+
err,
499+
DataFusionError::Internal(msg) if msg.contains("LikeExpr is missing required field 'pattern'")
500+
));
501+
}
502+
503+
#[test]
504+
fn try_from_proto_propagates_expr_decode_error() {
505+
let node = like_node(
506+
false,
507+
false,
508+
Some(Box::new(column_node("a"))),
509+
Some(Box::new(column_node("b"))),
510+
);
511+
let schema = Schema::empty();
512+
let decoder = StubDecoder::failing_on(1);
513+
let ctx = PhysicalExprDecodeCtx::new(&schema, &decoder);
514+
let err = LikeExpr::try_from_proto(&node, &ctx).unwrap_err();
515+
assert!(matches!(err, DataFusionError::Internal(msg) if msg.contains("call 1")));
516+
}
517+
518+
#[test]
519+
fn try_from_proto_propagates_pattern_decode_error() {
520+
let node = like_node(
521+
false,
522+
false,
523+
Some(Box::new(column_node("a"))),
524+
Some(Box::new(column_node("b"))),
525+
);
526+
let schema = Schema::empty();
527+
let decoder = StubDecoder::failing_on(2);
528+
let ctx = PhysicalExprDecodeCtx::new(&schema, &decoder);
529+
let err = LikeExpr::try_from_proto(&node, &ctx).unwrap_err();
530+
assert!(matches!(err, DataFusionError::Internal(msg) if msg.contains("call 2")));
531+
}
532+
}

datafusion/physical-expr/src/lib.rs

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,9 @@ mod partitioning;
4040
mod physical_expr;
4141
pub mod planner;
4242
pub mod projection;
43+
/// Shared test helpers for the `try_to_proto` / `try_from_proto` unit tests
44+
#[cfg(all(test, feature = "proto"))]
45+
pub(crate) mod proto_test_util;
4346
mod scalar_function;
4447
pub mod scalar_subquery;
4548
pub mod simplifier;

0 commit comments

Comments
 (0)