Skip to content

Commit c0bf17f

Browse files
adriangbclaude
andcommitted
refactor(physical-expr): adopt proto ctx expr helpers in InList and Like
Replace the hand-rolled list maps and missing-required-field checks in `InListExpr` and `LikeExpr`'s `try_to_proto`/`try_from_proto` with the new `encode_children_expressions`, `decode_children_expressions`, and `decode_required_expression` ctx methods. Behavior note: `decode_required_expression` couples the presence check with the decode, so `LikeExpr` now decodes children left-to-right rather than validating both required fields up front. The end result is unchanged (a missing required field still errors), but a present sibling is decoded before a later missing field is reported; the `try_from_proto_rejects_missing_pattern` test is updated accordingly. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent bc9f439 commit c0bf17f

2 files changed

Lines changed: 11 additions & 33 deletions

File tree

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

Lines changed: 4 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -263,17 +263,8 @@ impl InListExpr {
263263
}
264264
};
265265

266-
let expr = ctx.decode(node.expr.as_deref().ok_or_else(|| {
267-
datafusion_common::DataFusionError::Internal(
268-
"InList is missing required field 'expr'".to_string(),
269-
)
270-
})?)?;
271-
272-
let list = node
273-
.list
274-
.iter()
275-
.map(|e| ctx.decode(e))
276-
.collect::<Result<Vec<_>>>()?;
266+
let expr = ctx.decode_required_expression(node.expr.as_deref(), "expr")?;
267+
let list = ctx.decode_children_expressions(&node.list)?;
277268

278269
Ok(Arc::new(InListExpr::try_new(
279270
expr,
@@ -491,11 +482,7 @@ impl PhysicalExpr for InListExpr {
491482
expr_type: Some(protobuf::physical_expr_node::ExprType::InList(Box::new(
492483
protobuf::PhysicalInListNode {
493484
expr: Some(Box::new(ctx.encode_child(&self.expr)?)),
494-
list: self
495-
.list
496-
.iter()
497-
.map(|e| ctx.encode_child(e))
498-
.collect::<Result<Vec<_>>>()?,
485+
list: ctx.encode_children_expressions(&self.list)?,
499486
negated: self.negated,
500487
},
501488
))),
@@ -4007,7 +3994,7 @@ mod proto_tests {
40073994
let err = InListExpr::try_from_proto(&node, &ctx).unwrap_err();
40083995
assert!(matches!(
40093996
err,
4010-
DataFusionError::Internal(msg) if msg.contains("InList is missing required field 'expr'")
3997+
DataFusionError::Internal(msg) if msg.contains(r#"Missing required field "expr""#)
40113998
));
40123999
}
40134000

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

Lines changed: 7 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -190,22 +190,11 @@ impl LikeExpr {
190190
_ => return internal_err!("PhysicalExprNode is not a LikeExpr"),
191191
};
192192

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-
204193
Ok(Arc::new(LikeExpr::new(
205194
like_expr.negated,
206195
like_expr.case_insensitive,
207-
ctx.decode(expr)?,
208-
ctx.decode(pattern)?,
196+
ctx.decode_required_expression(like_expr.expr.as_deref(), "expr")?,
197+
ctx.decode_required_expression(like_expr.pattern.as_deref(), "pattern")?,
209198
)))
210199
}
211200
}
@@ -483,20 +472,22 @@ mod proto_tests {
483472
let err = LikeExpr::try_from_proto(&node, &ctx).unwrap_err();
484473
assert!(matches!(
485474
err,
486-
DataFusionError::Internal(msg) if msg.contains("LikeExpr is missing required field 'expr'")
475+
DataFusionError::Internal(msg) if msg.contains(r#"Missing required field "expr""#)
487476
));
488477
}
489478

490479
#[test]
491480
fn try_from_proto_rejects_missing_pattern() {
492481
let node = like_node(false, false, Some(Box::new(column_node("a"))), None);
493482
let schema = Schema::empty();
494-
let decoder = UnreachableDecoder;
483+
// `expr` is present, so it is decoded before the missing-`pattern`
484+
// check fires; use a decoder that succeeds for that first child.
485+
let decoder = StubDecoder::ok();
495486
let ctx = PhysicalExprDecodeCtx::new(&schema, &decoder);
496487
let err = LikeExpr::try_from_proto(&node, &ctx).unwrap_err();
497488
assert!(matches!(
498489
err,
499-
DataFusionError::Internal(msg) if msg.contains("LikeExpr is missing required field 'pattern'")
490+
DataFusionError::Internal(msg) if msg.contains(r#"Missing required field "pattern""#)
500491
));
501492
}
502493

0 commit comments

Comments
 (0)