Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
sam-1112 wants to merge 7 commits into
base: main
Choose a base branch
from
Open

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

Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
471d526
fix: support Utf8/LargeUtf8/Utf8View in native RLike without panickin…
sam-1112 Aug 2, 2026
c41ccfc
style: cargo fmt
sam-1112 Aug 2, 2026
22b7071
fix: handle any dictionary key type in RLike and tighten review follo…
sam-1112 Aug 3, 2026
d2e61a2
test: cover nulls in RLike dictionary values and drop stray doc comment
sam-1112 Aug 4, 2026
fa4dcf2
test: assert RLike all-valid output has no null buffer
Aug 28, 2026
d855849
test: cover long Utf8View, UInt64 keys, and sliced dictionaries in RLike
Sep 9, 2026
b845444
Merge branch 'apache:main' into fix-rlike-string-layouts-5102
sam-1112 Sep 17, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
227 changes: 192 additions & 35 deletions native/spark-expr/src/predicate_funcs/rlike.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,10 @@
// under the License.

use crate::SparkError;
use arrow::array::builder::BooleanBuilder;
use arrow::array::types::Int32Type;
use arrow::array::{Array, BooleanArray, DictionaryArray, RecordBatch, StringArray};
use arrow::array::{Array, ArrayRef, AsArray, BooleanArray, RecordBatch, StringArrayType};
use arrow::compute::take;
use arrow::datatypes::{DataType, Schema};
use datafusion::common::cast::{as_large_string_array, as_string_array, as_string_view_array};
use datafusion::common::{internal_err, Result, ScalarValue};
use datafusion::physical_expr::PhysicalExpr;
use datafusion::physical_plan::ColumnarValue;
Expand Down Expand Up @@ -71,22 +70,29 @@ impl RLike {
})
}

fn is_match(&self, inputs: &StringArray) -> BooleanArray {
let mut builder = BooleanBuilder::with_capacity(inputs.len());
if inputs.is_nullable() {
for i in 0..inputs.len() {
if inputs.is_null(i) {
builder.append_null();
} else {
builder.append_value(self.pattern.is_match(inputs.value(i)));
}
}
} else {
for i in 0..inputs.len() {
builder.append_value(self.pattern.is_match(inputs.value(i)));
/// Match the pre-compiled pattern against a string array of any Arrow string layout.
///
/// Keeps the plan-time compiled [`Regex`] rather than calling Arrow's
/// `regexp_is_match(_scalar)`, which recompiles the pattern on every batch.
fn is_match<'a, S>(&self, inputs: &'a S) -> BooleanArray
where
&'a S: StringArrayType<'a>,
{

@0lai0 0lai0 Aug 2, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 sam-1112 Aug 3, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

inputs
.iter()
.map(|v| v.map(|s| self.pattern.is_match(s)))
.collect()
}

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 => {
internal_err!("RLike requires string type for input, got {other:?}")
}
}
builder.finish()
}
}

Expand All @@ -111,29 +117,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 = array.as_any_dictionary();
// evaluate the regexp pattern against the dictionary values
let new_values = self.is_match(dict_values);
let new_values = self.is_match_array(dict_array.values())?;
// convert to conventional (not dictionary-encoded) array
let result = take(&new_values, dict_array.keys(), None)?;
Ok(ColumnarValue::Array(result))
}
ColumnarValue::Array(array) => {
let inputs = array
.as_any()
.downcast_ref::<StringArray>()
.expect("string array");
let array = self.is_match(inputs);
Ok(ColumnarValue::Array(Arc::new(array)))
let result = self.is_match_array(&array)?;
Ok(ColumnarValue::Array(Arc::new(result)))
}
ColumnarValue::Scalar(scalar) => {
if scalar.is_null() {
Expand Down Expand Up @@ -180,7 +176,32 @@ impl PhysicalExpr for RLike {
#[cfg(test)]
mod tests {
use super::*;
use datafusion::physical_expr::expressions::Literal;
use arrow::array::{
DictionaryArray, Int32Array, Int8Array, LargeStringArray, StringArray, StringViewArray,
UInt64Array,
};
use arrow::datatypes::{Field, Int32Type, Int8Type, UInt64Type};
use datafusion::physical_expr::expressions::{Column, Literal};

fn assert_bool_results(result: ColumnarValue, expected: &[Option<bool>]) {
let ColumnarValue::Array(arr) = result else {
panic!("expected array result");
};
let bools = arr
.as_any()
.downcast_ref::<BooleanArray>()
.expect("boolean array");
assert_eq!(bools.len(), expected.len());
for (i, exp) in expected.iter().enumerate() {
match exp {
Some(v) => {
assert!(!bools.is_null(i), "row {i} should not be null");
assert_eq!(bools.value(i), *v, "row {i}");
}
None => assert!(bools.is_null(i), "row {i} should be null"),
}
}
}

#[test]
fn test_rlike_scalar_string_variants() {
Expand Down Expand Up @@ -225,4 +246,140 @@ mod tests {
let result = expr.evaluate(&RecordBatch::new_empty(Arc::new(Schema::empty())));
assert!(result.is_err());
}

#[test]
fn test_rlike_string_array_layouts() {
let pattern = "R[a-z]+";
let cases: Vec<(DataType, ArrayRef)> = vec![
(
DataType::Utf8,
Arc::new(StringArray::from(vec![Some("Rose"), None, Some("Daisy")])),
),
(
DataType::LargeUtf8,
Arc::new(LargeStringArray::from(vec![
Some("Rose"),
None,
Some("Daisy"),
])),
),
(
DataType::Utf8View,
Arc::new(StringViewArray::from(vec![
Some("Rhododendrons"),
None,
Some("Daisy"),
])),
),
Comment on lines +267 to +273

@andygrove andygrove Sep 8, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All the Utf8View values in these tests are "Rose" and "Daisy", which are both 12 bytes or fewer, so they live inline in the view struct. The representation that actually differs from Utf8 is the one for strings longer than 12 bytes, where the view holds a (len, prefix, buffer_index, offset) pointer into a separate data buffer, and that path is never constructed here.

Since Utf8View support is the headline of this PR and there is no end-to-end coverage behind it, could you make one of these values longer than 12 bytes? I checked that a 42-byte value passes on your branch and panics on main, so it is in scope. The Dictionary(Int32, Utf8View) case below would benefit from the same thing.

@sam-1112 sam-1112 Sep 9, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated both the direct and dictionary Utf8View cases to use "Rhododendrons" (13 bytes), so they exercise the out-of-line view representation rather than the inline representation used for values of 12 bytes or fewer.

];

for (data_type, array) in cases {
let schema = Arc::new(Schema::new(vec![Field::new("s", data_type, true)]));
let batch = RecordBatch::try_new(Arc::clone(&schema), vec![array]).unwrap();
let expr = RLike::try_new(Arc::new(Column::new("s", 0)), pattern).unwrap();
assert_bool_results(
expr.evaluate(&batch).unwrap(),
&[Some(true), None, Some(false)],
);
}
}

#[test]
fn test_rlike_string_array_no_nulls() {
let schema = Arc::new(Schema::new(vec![Field::new("s", DataType::Utf8, false)]));
let batch = RecordBatch::try_new(
Arc::clone(&schema),
vec![Arc::new(StringArray::from(vec!["Rose", "Daisy"]))],
)
.unwrap();

let expr = RLike::try_new(Arc::new(Column::new("s", 0)), "R[a-z]+").unwrap();
let ColumnarValue::Array(arr) = expr.evaluate(&batch).unwrap() else {
panic!("expected array result");
};
// Preserve a null-buffer-free output shape for all-valid input, avoiding an
// unnecessary len / 8-byte validity allocation.
assert!(arr.nulls().is_none());
assert_bool_results(ColumnarValue::Array(arr), &[Some(true), Some(false)]);
}

#[test]
fn test_rlike_dictionary_arrays() {
let pattern = "R[a-z]+";
let expected = [Some(true), None, Some(false)];

let utf8_values: ArrayRef = Arc::new(StringArray::from(vec!["Rose", "Daisy"]));
let utf8_view_values: ArrayRef =
Arc::new(StringViewArray::from(vec!["Rhododendrons", "Daisy"]));
// Null in dictionary values (keys all valid): is_match emits null, take carries it.
let utf8_values_with_null: ArrayRef =
Arc::new(StringArray::from(vec![Some("Rose"), None, Some("Daisy")]));
let sliced_dictionary = DictionaryArray::<Int32Type>::new(
Int32Array::from(vec![Some(1), Some(0), None, Some(1), Some(0)]),
Arc::clone(&utf8_values),
);
let sliced_dictionary: ArrayRef = Arc::new(sliced_dictionary.slice(1, 3));

let cases: Vec<(DataType, ArrayRef)> = vec![
Comment thread
sam-1112 marked this conversation as resolved.
(
DataType::Dictionary(Box::new(DataType::Int32), Box::new(DataType::Utf8)),
Arc::new(DictionaryArray::<Int32Type>::new(
Int32Array::from(vec![Some(0), None, Some(1)]),
Arc::clone(&utf8_values),
)),
),
(
DataType::Dictionary(Box::new(DataType::Int32), Box::new(DataType::Utf8View)),
Arc::new(DictionaryArray::<Int32Type>::new(
Int32Array::from(vec![Some(0), None, Some(1)]),
Arc::clone(&utf8_view_values),
)),
),
(
DataType::Dictionary(Box::new(DataType::Int8), Box::new(DataType::Utf8)),
Arc::new(DictionaryArray::<Int8Type>::new(
Int8Array::from(vec![Some(0), None, Some(1)]),
Arc::clone(&utf8_values),
)),
),
Comment on lines +338 to +344

@andygrove andygrove Sep 8, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two more rows here that would each be one line. Dictionary(UInt64, Utf8), since as_any_dictionary() covers the unsigned key types too and both of the current cases are signed. And a sliced dictionary, since that is the shape that turns up after a filter or a limit rather than the contiguous one. I confirmed both panic on main and pass on your branch.

@sam-1112 sam-1112 Sep 9, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added coverage for Dictionary(UInt64, Utf8) and a dictionary sliced with a non-zero offset. Both cases use the shared [Some(true), None, Some(false)] expected result.

(
DataType::Dictionary(Box::new(DataType::UInt64), Box::new(DataType::Utf8)),
Arc::new(DictionaryArray::<UInt64Type>::new(
UInt64Array::from(vec![Some(0), None, Some(1)]),
Arc::clone(&utf8_values),
)),
),
(
DataType::Dictionary(Box::new(DataType::Int32), Box::new(DataType::Utf8)),
Arc::new(DictionaryArray::<Int32Type>::new(
Int32Array::from(vec![Some(0), Some(1), Some(2)]),
utf8_values_with_null,
)),
),
(
DataType::Dictionary(Box::new(DataType::Int32), Box::new(DataType::Utf8)),
sliced_dictionary,
),
];

for (data_type, array) in cases {
let schema = Arc::new(Schema::new(vec![Field::new("s", data_type, true)]));
let batch = RecordBatch::try_new(Arc::clone(&schema), vec![array]).unwrap();
let expr = RLike::try_new(Arc::new(Column::new("s", 0)), pattern).unwrap();
assert_bool_results(expr.evaluate(&batch).unwrap(), &expected);
}
}

#[test]
fn test_rlike_array_non_string_error() {
let schema = Arc::new(Schema::new(vec![Field::new("b", DataType::Boolean, true)]));
let batch = RecordBatch::try_new(
Arc::clone(&schema),
vec![Arc::new(BooleanArray::from(vec![Some(true), None]))],
)
.unwrap();

let expr = RLike::try_new(Arc::new(Column::new("b", 0)), "R[a-z]+").unwrap();
assert!(expr.evaluate(&batch).is_err());
}
}