Jefffrey commented on code in PR #17891:
URL: https://github.com/apache/datafusion/pull/17891#discussion_r2412895340
##########
datafusion/functions-nested/src/array_has.rs:
##########
@@ -145,6 +145,7 @@ impl ScalarUDFImpl for ArrayHas {
let list = scalar_values
.into_iter()
.flatten()
+ .flatten()
Review Comment:
For here, it might be more correct to do it like so:
```rust
if let Ok(scalar_values) =
ScalarValue::convert_array_to_scalar_vec(&array)
{
assert_eq!(scalar_values.len(), 1);
match &scalar_values[0] {
// Haystack was a single list element as expected
Some(list) => {
let list = list
.iter()
.map(|v| Expr::Literal(v.clone(), None))
.collect();
return Ok(ExprSimplifyResult::Simplified(in_list(
std::mem::take(needle),
list,
false,
)));
}
// Haystack was a singular null, should be handled elsewhere
None => {
return Ok(ExprSimplifyResult::Original(args));
}
};
}
```
- Instead of flattening `None` into an empty vec, we handle it separately.
Example of an accompanying test:
```rust
#[test]
fn test123() {
let haystack = ListArray::new_null(
Arc::new(Field::new_list_field(DataType::Int32, true)),
1,
);
let haystack = lit(ScalarValue::List(Arc::new(haystack)));
let needle = col("c");
let props = ExecutionProps::new();
let context =
datafusion_expr::simplify::SimplifyContext::new(&props);
let Ok(ExprSimplifyResult::Original(args)) =
ArrayHas::new().simplify(vec![haystack.clone(), needle.clone()],
&context)
else {
panic!("Expected non-simplified expression");
};
assert_eq!(args, vec![haystack, col("c")]);
}
```
We can see it's essentially like doing `array_has(null, 1)` and handling
this case in the simplifier; we don't try to simplify it as I don't think
there's a way to pass the null as the haystack to `in_list()`.
**However**, it's likely some other code path handles this null propagation
already so that's why tests didn't fail. I prefer the robust (albeit more
verbose) approach here (to me to reads easier), but we can stick with what we
have here (preferably calling it out via comments?) either way.
Thoughts?
--
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]
