sunchao commented on code in PR #5322:
URL: https://github.com/apache/datafusion-comet/pull/5322#discussion_r3816933541
##########
native/spark-expr/src/string_funcs/contains.rs:
##########
@@ -134,6 +138,21 @@ fn contains_with_arrow_scalar(
Ok(Arc::new(result))
}
+fn contains_scalar_array(
+ haystack_scalar: &ScalarValue,
+ needle_array: &ArrayRef,
+) -> Result<ArrayRef> {
+ if haystack_scalar.is_null() {
+ return Ok(Arc::new(BooleanArray::new_null(needle_array.len())));
+ }
+
+ let haystack_str = get_string_scalar_value(haystack_scalar, "haystack")?;
+ let haystack_scalar_array = StringArray::new_scalar(haystack_str);
+
+ let result = arrow_contains(&haystack_scalar_array, needle_array)?;
Review Comment:
[P2] Preserve the haystack's Arrow type
Could this use `Scalar::new(haystack_scalar.to_array()?)` instead of always
constructing a `StringArray`? Arrow 58.4 requires matching string
representations. For example, `LargeUtf8("abc")` with a `LargeStringArray`
containing `["a", "bc", null, "", "d"]` returns `[true, true, null, true,
false]` before this change, but now fails with `Utf8 CONTAINS LargeUtf8`. The
equivalent `Utf8View` inputs also fail. A non-null dictionary-string scalar
previously worked too, but `get_string_scalar_value` now rejects it.
I reproduced these against the exact before/after UDFs. I have not
established an ordinary Spark query that produces those scalar representations,
so this is a native-UDF compatibility finding. Keeping the scalar's type
preserves the allocation benefit. Could you add regression tests for these
representations?
--
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]
