andygrove commented on code in PR #5215:
URL: https://github.com/apache/datafusion-comet/pull/5215#discussion_r3699497586
##########
native/spark-expr/src/predicate_funcs/rlike.rs:
##########
@@ -111,29 +135,19 @@ impl PhysicalExpr for RLike {
fn evaluate(&self, batch: &RecordBatch) -> Result<ColumnarValue> {
match self.child.evaluate(batch)? {
- ColumnarValue::Array(array) if
array.as_any().is::<DictionaryArray<Int32Type>>() => {
- let dict_array = array
- .as_any()
- .downcast_ref::<DictionaryArray<Int32Type>>()
- .expect("dict array");
- let dict_values = dict_array
- .values()
- .as_any()
- .downcast_ref::<StringArray>()
- .expect("strings");
+ ColumnarValue::Array(array)
+ if matches!(array.data_type(), DataType::Dictionary(_, _)) =>
+ {
+ let dict_array = as_dictionary_array::<Int32Type>(&array)?;
Review Comment:
The guard above now matches `DataType::Dictionary(_, _)` for any key type,
but the body still calls `as_dictionary_array::<Int32Type>`, so a
non-`Int32`-keyed dictionary trades the old panic for an `Internal` error
rather than actually being handled. I checked this on your branch and a
`Dictionary(Int8, Utf8)` input gives:
```
Err(Internal("could not cast array of type Dictionary(Int8, Utf8) to ...
DictionaryArray<Int32Type>"))
```
I tried `array.as_any_dictionary()` instead and it handles every key type.
`arrow::compute::take` already accepts `indices: &dyn Array` in arrow 58, so
`take(&new_values, dict_array.keys(), None)` works unchanged, and the
`Int32Type` and `as_dictionary_array` imports become unnecessary. With that
swap the `Int8`-keyed case returns the correct `[true, null, false]`. Would
that work for you here?
Worth noting the good news too: `Dictionary(Int32, Utf8View)` and
`Dictionary(Int32, LargeUtf8)` do work correctly with your change.
##########
native/spark-expr/src/predicate_funcs/rlike.rs:
##########
@@ -88,6 +103,15 @@ impl RLike {
}
builder.finish()
}
+
+ fn is_match_array(&self, array: &ArrayRef) -> Result<BooleanArray> {
+ match array.data_type() {
+ DataType::Utf8 => Ok(self.is_match(as_string_array(array)?)),
+ DataType::LargeUtf8 =>
Ok(self.is_match(as_large_string_array(array)?)),
+ DataType::Utf8View =>
Ok(self.is_match(as_string_view_array(array)?)),
+ other => exec_err!("RLike requires string type for input, got
{other:?}"),
Review Comment:
Small consistency question. This uses `exec_err!` while the scalar branch
below uses `internal_err!` for the same "not a string" condition. Since a
non-string child on `RLike` means the planner produced something invalid rather
than the user doing something wrong, would `internal_err!` be the better fit
for both?
##########
native/spark-expr/src/predicate_funcs/rlike.rs:
##########
@@ -37,6 +40,11 @@ use std::sync::Arc;
/// regular expression engine, which are documented at:
///
/// https://docs.oracle.com/javase/8/docs/api/java/util/regex/Pattern.html
+///
+/// Array matching keeps the plan-time compiled [`Regex`] and loops over Utf8 /
Review Comment:
This mentions criterion benches showing regressions on patterns like
character classes and points to #5102, but the issue only says per-batch
compilation "is a measurable regression" without any numbers, and I do not see
a bench added under `native/spark-expr/benches/`. Do you have the numbers
handy? Posting them in the PR description, or adding the bench alongside the
existing ones like `regexp_extract.rs`, would make this a much stronger
reference for the next person who wonders why the kernel was not used.
The same rationale also appears twice, here and again on `is_match`. This
struct-level rustdoc is where the Spark-compatibility caveat lives, so an
explanation of a rejected implementation alternative reads a little out of
place. Keeping the single copy on `is_match` would be cleaner.
##########
native/spark-expr/src/predicate_funcs/rlike.rs:
##########
@@ -225,4 +254,60 @@ mod tests {
let result =
expr.evaluate(&RecordBatch::new_empty(Arc::new(Schema::empty())));
assert!(result.is_err());
}
+
+ #[test]
+ fn test_rlike_utf8_array() {
Review Comment:
Thanks for adding these. A few gaps worth closing.
The dictionary path is the biggest change in the diff and it does not have a
test yet. Could you add coverage there? A `Dictionary(Int32, Utf8)` case plus a
`Dictionary(Int32, Utf8View)` case would exercise both the layout dispatch and
the `take`, and a non-`Int32` key case would pin down whatever behavior you
settle on for the comment above.
`Array::is_nullable()` is `logical_null_count() != 0` in arrow 58, so the
`else` branch of `is_match` only runs when the array has no nulls. All three of
these tests include a null, so that branch never gets hit. Adding one
all-non-null array would close it.
These three tests are also near-identical.
`test_rlike_scalar_string_variants` right above loops over the layouts, so it
might be nice to follow that shape here too.
--
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]
