fix: support Utf8/LargeUtf8/Utf8View in native RLike without panicking

[sam-1112]

Which issue does this PR close?

Closes #5102.

Rationale for this change

Native RLike array evaluation hard-downcasted to StringArray and panicked on other Arrow string layouts (LargeUtf8 / Utf8View). This PR keeps the plan-time compiled Regex (instead of Arrow's per-batch regexp_is_match) and hardens layout / dictionary handling so those inputs return correct results or a typed error instead of panicking.

Today's common Spark string / Parquet path still shows up as Utf8, and the planner currently casts some Utf8View UDF results back to Utf8, so LargeUtf8 / Utf8View may not be easy to hit end-to-end yet. The change is still useful hardening for the layouts called out in #5102, plus correct dictionary key-type handling.

What changes are included in this PR?

• Generalize array matching over Utf8 / LargeUtf8 / Utf8View via StringArrayType.
• Use as_any_dictionary() on the dictionary path so any key type works (match dictionary values, then take); scalar input still uses the precompiled Regex.
• Return internal_err! for non-string inputs on both the array and scalar paths (invalid plan rather than a user-facing execution error).
• Simplify is_match to iter / map / collect, matching the process_parse_url style.
• Keep the Spark-compatibility caveat on the struct rustdoc; keep the "why not Arrow kernel" note only on is_match.

How are these changes tested?

• Unit tests for string layouts (Utf8 / LargeUtf8 / Utf8View), an all-non-null array case, and dictionary cases: Dictionary(Int32, Utf8), Dictionary(Int32, Utf8View), Dictionary(Int8, Utf8).
• cargo test -p datafusion-comet-spark-expr --lib rlike
• cargo clippy -p datafusion-comet-spark-expr --lib --tests -- -D warnings

[andygrove]

Thanks for taking this on. Keeping the plan-time compiled Regex and just fixing the downcast robustness is the right call out of the two options in #5102, and the layout dispatch reads well.

I ran the native checks locally on this branch. cargo test -p datafusion-comet-spark-expr --lib rlike passes and cargo clippy -p datafusion-comet-spark-expr --lib --tests -- -D warnings is clean. CI itself has not run yet, all three workflows are sitting at the first-time-contributor approval gate, so I will get those approved.

I have left some comments inline. The main one is that the dictionary arm is still restricted to Int32 keys, so that shape trades a panic for an error rather than being handled.

One thing that would help in the PR description: which of these layouts is reachable today? planner.rs:3387 casts Utf8View results back to Utf8 with the comment "Comet does not yet support view types", and I could not find a plan that feeds a view array into RLike right now. The hardening is still worth doing and the issue asked for it, but if there is a known reachable case it would be good to name it. If one of these layouts is reachable through a config, an end-to-end query added to spark/src/test/resources/sql-tests/expressions/string/rlike_rust.sql would be a nice complement to the Rust unit tests.

[andygrove]

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.

[sam-1112]

Good catch — thanks for checking the non-Int32 key case.

You're right: widening the guard to Dictionary(_, _) while still casting with as_dictionary_array::<Int32Type> only turned the old panic into an Internal error for other key types. I've switched to array.as_any_dictionary() and take(&new_values, dict_array.keys(), None) as you suggested, and added a Dictionary(Int8, Utf8) test that expects [true, null, false].

Glad to hear Dictionary(Int32, Utf8View) / LargeUtf8 already looked good — those stay covered as well.

[andygrove]

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?

[sam-1112]

Good point — agreed. A non-string child here means an invalid plan rather than a user-facing execution error, so I've switched the array path to internal_err! as well to match the scalar branch.

[andygrove]

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.

[sam-1112]

Agreed on the docs cleanup. I don't have the criterion numbers saved anymore — I'll remove the strong "benches showed regressions" claim from the struct docs (or soften it) and keep the compile-per-batch rationale only on is_match. Happy to add a proper bench in a follow-up if useful.

[andygrove]

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.

[sam-1112]

Thanks for the review — agreed on all three points.

I've added dictionary coverage for Dictionary(Int32, Utf8), Dictionary(Int32, Utf8View), and a non-Int32 key case (Dictionary(Int8, Utf8)) so the layout dispatch, take, and key-type handling are all exercised. I also collapsed the three near-identical layout tests into a single looped test (same shape as test_rlike_scalar_string_variants), and added an all-non-null array case.

One related note: is_match no longer has the is_nullable() if/else — it now uses iter().map(...).collect() — so the no-nulls test is mainly a regression check rather than covering a separate branch. Happy to adjust further if you'd like anything else pinned down.

[0lai0]

Optional and non-blocking
This follows on from the comment by @andygrove below about is_nullable()

Since is_nullable() is logical_null_count() != 0, the else branch only runs on an array with no nulls, and StringArrayType gives us iter(). BooleanArray implements FromIterator<Option<bool>>, so both branches collapse into:

fn is_match<'a, S>(&self, inputs: &'a S) -> BooleanArray
where
    &'a S: StringArrayType<'a>,
{
    inputs.iter().map(|v| v.map(|s| self.pattern.is_match(s))).collect()
}

The uncovered branch stops existing rather than needing a test, and null handling no longer depends on is_nullable().
process_parse_url in url_funcs/parse_url.rs already uses the same StringArrayType bound and the same iter/collect shape.

Worth noting ArrayIter's docs call interleaved null-mask handling suboptimal, but relative to Regex::is_match I would expect that to be noise
ref: https://docs.rs/arrow/latest/arrow/array/struct.ArrayIter.html

Also, &'a self ties the borrow of self to the input lifetime and nothing is borrowed out of it, so plain &self would do

[sam-1112]

Good suggestion — I've collapsed is_match to the iter/map/collect form (and switched to &self), matching process_parse_url. That removes the is_nullable() branch entirely, so the uncovered else path no longer exists.

[0lai0]

Thanks @sam-1112 for picking this up! Congrats on your first PR.

[sam-1112]

Thanks for the review and for kicking CI past the first-time-contributor gate.

On the dictionary arm: agreed — I've switched to as_any_dictionary() so non-Int32 keys are handled, and added unit coverage for Dictionary(Int32, Utf8), Dictionary(Int32, Utf8View), and Dictionary(Int8, Utf8).

On reachability: I don't currently have a known plan that feeds Utf8View (or LargeUtf8) into native RLike. As you noted, planner.rs still casts Utf8View results back to Utf8 because Comet does not yet support view types, and the existing rlike_rust.sql path uses plain Spark string / Parquet, which shows up as Utf8. So this change is mainly hardening for the layouts #5102 called out, plus dictionary key-type correctness, rather than fixing a query I can reproduce end-to-end today. If we later find a reachable config/path, I'm happy to add an e2e case to rlike_rust.sql in a follow-up.

I've also updated the PR for the other inline comments (internal_err!, docs cleanup, test consolidation, and the iter/collect simplification).