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 4 commits into
base: main
Choose a base branch
from
+168 −35
Open

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

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
24e5a4e
fix: support Utf8/LargeUtf8/Utf8View in native RLike without panickin…
sam-1112 Aug 2, 2026
00f6ca0
style: cargo fmt
sam-1112 Aug 2, 2026
4874f48
fix: handle any dictionary key type in RLike and tighten review follo…
sam-1112 Aug 3, 2026
c799e5b
test: cover nulls in RLike dictionary values and drop stray doc comment
sam-1112 Aug 4, 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
203 changes: 168 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
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,31 @@ 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,
};
use arrow::datatypes::{Field, Int32Type, Int8Type};
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 +245,117 @@ 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("Rose"),
None,
Some("Daisy"),
])),
),
];

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();
assert_bool_results(expr.evaluate(&batch).unwrap(), &[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!["Rose", "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 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),
)),
),
(
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,
)),
),
];

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());
}
}