Skip to content

Commit f899f89

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 83eae7c commit f899f89

2 files changed

Lines changed: 18 additions & 31 deletions

File tree

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

Lines changed: 5 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -263,17 +263,9 @@ 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 =
267+
ctx.decode_required_expression(node.expr.as_deref(), "InListExpr", "expr")?;
268+
let list = ctx.decode_children_expressions(&node.list)?;
277269

278270
Ok(Arc::new(InListExpr::try_new(
279271
expr,
@@ -491,11 +483,7 @@ impl PhysicalExpr for InListExpr {
491483
expr_type: Some(protobuf::physical_expr_node::ExprType::InList(Box::new(
492484
protobuf::PhysicalInListNode {
493485
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<_>>>()?,
486+
list: ctx.encode_children_expressions(&self.list)?,
499487
negated: self.negated,
500488
},
501489
))),
@@ -4007,7 +3995,7 @@ mod proto_tests {
40073995
let err = InListExpr::try_from_proto(&node, &ctx).unwrap_err();
40083996
assert!(matches!(
40093997
err,
4010-
DataFusionError::Internal(msg) if msg.contains("InList is missing required field 'expr'")
3998+
DataFusionError::Internal(msg) if msg.contains("InListExpr is missing required field 'expr'")
40113999
));
40124000
}
40134001

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

Lines changed: 13 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -190,22 +190,19 @@ 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(
197+
like_expr.expr.as_deref(),
198+
"LikeExpr",
199+
"expr",
200+
)?,
201+
ctx.decode_required_expression(
202+
like_expr.pattern.as_deref(),
203+
"LikeExpr",
204+
"pattern",
205+
)?,
209206
)))
210207
}
211208
}
@@ -491,7 +488,9 @@ mod proto_tests {
491488
fn try_from_proto_rejects_missing_pattern() {
492489
let node = like_node(false, false, Some(Box::new(column_node("a"))), None);
493490
let schema = Schema::empty();
494-
let decoder = UnreachableDecoder;
491+
// `expr` is present, so it is decoded before the missing-`pattern`
492+
// check fires; use a decoder that succeeds for that first child.
493+
let decoder = StubDecoder::ok();
495494
let ctx = PhysicalExprDecodeCtx::new(&schema, &decoder);
496495
let err = LikeExpr::try_from_proto(&node, &ctx).unwrap_err();
497496
assert!(matches!(

0 commit comments

Comments
 (0)