From 9f78f97e488cd82a4b233ee606f422483255b061 Mon Sep 17 00:00:00 2001 From: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com> Date: Tue, 15 Sep 2026 17:42:48 -0500 Subject: [PATCH 1/8] fix: keep the regex diagnosis in regexp_count and regexp_instr errors `compile_regex` discarded the `regex::Error` and reported only the pattern, so a user could not see why a pattern or a flag was invalid. Report the diagnosis from the regex crate instead. It contains the pattern, so nothing is lost. Co-Authored-By: Claude Opus 5 --- datafusion/functions/src/regex/mod.rs | 4 ++-- .../test_files/regexp/regexp_count.slt | 16 ++++++++++++++++ .../test_files/regexp/regexp_instr.slt | 16 ++++++++++++++++ 3 files changed, 34 insertions(+), 2 deletions(-) diff --git a/datafusion/functions/src/regex/mod.rs b/datafusion/functions/src/regex/mod.rs index 3877251c66a45..5c1b7b1ee2e53 100644 --- a/datafusion/functions/src/regex/mod.rs +++ b/datafusion/functions/src/regex/mod.rs @@ -187,8 +187,8 @@ pub fn compile_regex(regex: &str, flags: Option<&str>) -> Result Date: Tue, 15 Sep 2026 19:40:00 -0500 Subject: [PATCH 2/8] refactor: move the regex compile helpers to physical-expr-common `compile_regex` and `compile_and_cache_regex` move to a new `regex` module in datafusion-physical-expr-common, so that the physical expressions can use them too. They now return a `DataFusionError` instead of an `ArrowError`, and they take the name of the SQL function of the caller, so that an unsupported flag names the function that the user called instead of a fixed pair of names. `datafusion_functions::regex` re-exports both, so the paths that callers use still resolve. regexp_count and regexp_instr propagate the new error type. Their tests are updated, including three that asserted nothing because the expected message was parsed as part of the SQL statement. Co-Authored-By: Claude Opus 5 --- Cargo.lock | 1 + datafusion/functions/src/regex/mod.rs | 48 +---- datafusion/functions/src/regex/regexpcount.rs | 180 ++++++++++-------- datafusion/functions/src/regex/regexpinstr.rs | 83 ++++---- datafusion/physical-expr-common/Cargo.toml | 1 + datafusion/physical-expr-common/src/lib.rs | 1 + datafusion/physical-expr-common/src/regex.rs | 150 +++++++++++++++ .../test_files/regexp/regexp_count.slt | 27 ++- .../test_files/regexp/regexp_instr.slt | 24 ++- 9 files changed, 337 insertions(+), 178 deletions(-) create mode 100644 datafusion/physical-expr-common/src/regex.rs diff --git a/Cargo.lock b/Cargo.lock index 2c4e8d3e7e9d0..f0677c38d85c9 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2462,6 +2462,7 @@ dependencies = [ "parking_lot", "pin-project", "rand 0.9.4", + "regex", ] [[package]] diff --git a/datafusion/functions/src/regex/mod.rs b/datafusion/functions/src/regex/mod.rs index 5c1b7b1ee2e53..55482cd52698d 100644 --- a/datafusion/functions/src/regex/mod.rs +++ b/datafusion/functions/src/regex/mod.rs @@ -19,12 +19,15 @@ use arrow::array::ArrayRef; use arrow::compute::kernels::{cmp::eq, nullif::nullif}; -use arrow::error::ArrowError; use datafusion_common::{Result, ScalarValue}; -use regex::Regex; -use std::collections::HashMap; -use std::collections::hash_map::Entry; use std::sync::Arc; + +// The compilation of a regular expression is shared with the physical +// expressions, so that every caller reports a failure in the same way. These +// re-exports keep the paths that callers of this crate already use. +pub use datafusion_physical_expr_common::regex::{ + compile_and_cache_regex, compile_regex, +}; pub mod regexpcount; pub mod regexpinstr; pub mod regexplike; @@ -139,24 +142,6 @@ pub fn functions() -> Vec> { ] } -pub fn compile_and_cache_regex<'strings, 'cache>( - regex: &'strings str, - flags: Option<&'strings str>, - regex_cache: &'cache mut HashMap<(&'strings str, Option<&'strings str>), Regex>, -) -> Result<&'cache Regex, ArrowError> -where - 'strings: 'cache, -{ - let result = match regex_cache.entry((regex, flags)) { - Entry::Occupied(occupied_entry) => occupied_entry.into_mut(), - Entry::Vacant(vacant_entry) => { - let compiled = compile_regex(regex, flags)?; - vacant_entry.insert(compiled) - } - }; - Ok(result) -} - /// Maps `start`, a 1-based character position, to a byte offset in `value`. /// Positions `1..=n` (for an `n`-character string) map to the corresponding /// character's first byte; position `n + 1`, the end of the string, maps to @@ -173,25 +158,6 @@ pub(crate) fn start_to_byte_offset(value: &str, start: i64) -> Option { .nth(start_index) } -pub fn compile_regex(regex: &str, flags: Option<&str>) -> Result { - let pattern = match flags { - None | Some("") => regex.to_string(), - Some(flags) => { - if flags.contains('g') { - return Err(ArrowError::ComputeError( - "regexp_count()/regexp_instr() does not support the global flag" - .to_string(), - )); - } - format!("(?{flags}){regex}") - } - }; - - Regex::new(&pattern).map_err(|e| { - ArrowError::ComputeError(format!("Regular expression did not compile: {e}")) - }) -} - #[cfg(test)] mod tests { use super::start_to_byte_offset; diff --git a/datafusion/functions/src/regex/regexpcount.rs b/datafusion/functions/src/regex/regexpcount.rs index 2920b687ed33f..c8ada077afd82 100644 --- a/datafusion/functions/src/regex/regexpcount.rs +++ b/datafusion/functions/src/regex/regexpcount.rs @@ -21,7 +21,6 @@ use arrow::datatypes::{DataType, Int64Type}; use arrow::datatypes::{ DataType::Int64, DataType::LargeUtf8, DataType::Utf8, DataType::Utf8View, }; -use arrow::error::ArrowError; use datafusion_common::{Result, ScalarValue, exec_err, internal_err}; use datafusion_expr::{ ColumnarValue, Documentation, ScalarFunctionArgs, ScalarUDFImpl, Signature, @@ -155,7 +154,6 @@ pub fn regexp_count_func(args: &[ArrayRef]) -> Result { if args_len > 2 { Some(&args[2]) } else { None }, if args_len > 3 { Some(&args[3]) } else { None }, ) - .map_err(|e| e.into()) } /// `arrow-rs` style implementation of `regexp_count` function. @@ -178,7 +176,7 @@ fn regexp_count( regex_array: &dyn Datum, start_array: Option<&dyn Datum>, flags_array: Option<&dyn Datum>, -) -> Result { +) -> Result { let (regex_array, is_regex_scalar) = regex_array.get(); let (start_array, is_start_scalar) = start_array.map_or((None, true), |start| { let (start, is_start_scalar) = start.get(); @@ -199,15 +197,17 @@ fn regexp_count( None, is_flags_scalar, ), - (Utf8, Utf8, Some(flags_array)) if *flags_array.data_type() == Utf8 => regexp_count_inner( - &values.as_string::(), - ®ex_array.as_string::(), - is_regex_scalar, - start_array.map(|start| start.as_primitive::()), - is_start_scalar, - Some(&flags_array.as_string::()), - is_flags_scalar, - ), + (Utf8, Utf8, Some(flags_array)) if *flags_array.data_type() == Utf8 => { + regexp_count_inner( + &values.as_string::(), + ®ex_array.as_string::(), + is_regex_scalar, + start_array.map(|start| start.as_primitive::()), + is_start_scalar, + Some(&flags_array.as_string::()), + is_flags_scalar, + ) + } (LargeUtf8, LargeUtf8, None) => regexp_count_inner( &values.as_string::(), ®ex_array.as_string::(), @@ -217,15 +217,19 @@ fn regexp_count( None, is_flags_scalar, ), - (LargeUtf8, LargeUtf8, Some(flags_array)) if *flags_array.data_type() == LargeUtf8 => regexp_count_inner( - &values.as_string::(), - ®ex_array.as_string::(), - is_regex_scalar, - start_array.map(|start| start.as_primitive::()), - is_start_scalar, - Some(&flags_array.as_string::()), - is_flags_scalar, - ), + (LargeUtf8, LargeUtf8, Some(flags_array)) + if *flags_array.data_type() == LargeUtf8 => + { + regexp_count_inner( + &values.as_string::(), + ®ex_array.as_string::(), + is_regex_scalar, + start_array.map(|start| start.as_primitive::()), + is_start_scalar, + Some(&flags_array.as_string::()), + is_flags_scalar, + ) + } (Utf8View, Utf8View, None) => regexp_count_inner( &values.as_string_view(), ®ex_array.as_string_view(), @@ -235,18 +239,22 @@ fn regexp_count( None, is_flags_scalar, ), - (Utf8View, Utf8View, Some(flags_array)) if *flags_array.data_type() == Utf8View => regexp_count_inner( - &values.as_string_view(), - ®ex_array.as_string_view(), - is_regex_scalar, - start_array.map(|start| start.as_primitive::()), - is_start_scalar, - Some(&flags_array.as_string_view()), - is_flags_scalar, + (Utf8View, Utf8View, Some(flags_array)) + if *flags_array.data_type() == Utf8View => + { + regexp_count_inner( + &values.as_string_view(), + ®ex_array.as_string_view(), + is_regex_scalar, + start_array.map(|start| start.as_primitive::()), + is_start_scalar, + Some(&flags_array.as_string_view()), + is_flags_scalar, + ) + } + _ => internal_err!( + "regexp_count() expected the input arrays to be of type Utf8, LargeUtf8, or Utf8View and the data types of the values, regex_array, and flags_array to match" ), - _ => Err(ArrowError::ComputeError( - "regexp_count() expected the input arrays to be of type Utf8, LargeUtf8, or Utf8View and the data types of the values, regex_array, and flags_array to match".to_string(), - )), } } @@ -258,7 +266,7 @@ fn regexp_count_inner<'a, S>( is_start_scalar: bool, flags_array: Option<&S>, is_flags_scalar: bool, -) -> Result +) -> Result where S: StringArrayType<'a>, { @@ -293,23 +301,23 @@ where match (regex_scalar, is_start_scalar, is_flags_scalar) { (Some(regex), true, true) => { - let pattern = compile_regex(regex, flags_scalar)?; + let pattern = compile_regex("regexp_count", regex, flags_scalar)?; Ok(Arc::new( values .iter() .map(|value| count_matches(value, &pattern, start_scalar)) - .collect::>()?, + .collect::>()?, )) } (Some(regex), true, false) => { let flags_array = flags_array.unwrap(); if values.len() != flags_array.len() { - return Err(ArrowError::ComputeError(format!( + return exec_err!( "flags_array must be the same length as values array; got {} and {}", flags_array.len(), - values.len(), - ))); + values.len() + ); } Ok(Arc::new( @@ -322,17 +330,18 @@ where }; let pattern = compile_and_cache_regex( + "regexp_count", regex, Some(flags), &mut regex_cache, )?; count_matches(value, pattern, start_scalar) }) - .collect::>()?, + .collect::>()?, )) } (Some(regex), false, true) => { - let pattern = compile_regex(regex, flags_scalar)?; + let pattern = compile_regex("regexp_count", regex, flags_scalar)?; let start_array = start_array.unwrap(); @@ -341,17 +350,17 @@ where .iter() .zip(start_array.iter()) .map(|(value, start)| count_matches(value, &pattern, start)) - .collect::>()?, + .collect::>()?, )) } (Some(regex), false, false) => { let flags_array = flags_array.unwrap(); if values.len() != flags_array.len() { - return Err(ArrowError::ComputeError(format!( + return exec_err!( "flags_array must be the same length as values array; got {} and {}", flags_array.len(), - values.len(), - ))); + values.len() + ); } Ok(Arc::new( @@ -365,21 +374,25 @@ where return Ok(None); }; - let pattern = - compile_and_cache_regex(regex, Some(flags), &mut regex_cache)?; + let pattern = compile_and_cache_regex( + "regexp_count", + regex, + Some(flags), + &mut regex_cache, + )?; count_matches(value, pattern, start) }) - .collect::>()?, + .collect::>()?, )) } (None, true, true) => { if values.len() != regex_array.len() { - return Err(ArrowError::ComputeError(format!( + return exec_err!( "regex_array must be the same length as values array; got {} and {}", regex_array.len(), - values.len(), - ))); + values.len() + ); } Ok(Arc::new( @@ -392,31 +405,32 @@ where }; let pattern = compile_and_cache_regex( + "regexp_count", regex, flags_scalar, &mut regex_cache, )?; count_matches(value, pattern, start_scalar) }) - .collect::>()?, + .collect::>()?, )) } (None, true, false) => { if values.len() != regex_array.len() { - return Err(ArrowError::ComputeError(format!( + return exec_err!( "regex_array must be the same length as values array; got {} and {}", regex_array.len(), - values.len(), - ))); + values.len() + ); } let flags_array = flags_array.unwrap(); if values.len() != flags_array.len() { - return Err(ArrowError::ComputeError(format!( + return exec_err!( "flags_array must be the same length as values array; got {} and {}", flags_array.len(), - values.len(), - ))); + values.len() + ); } Ok(Arc::new( @@ -427,6 +441,7 @@ where }; let pattern = compile_and_cache_regex( + "regexp_count", regex, Some(flags), &mut regex_cache, @@ -434,25 +449,25 @@ where count_matches(value, pattern, start_scalar) }) - .collect::>()?, + .collect::>()?, )) } (None, false, true) => { if values.len() != regex_array.len() { - return Err(ArrowError::ComputeError(format!( + return exec_err!( "regex_array must be the same length as values array; got {} and {}", regex_array.len(), - values.len(), - ))); + values.len() + ); } let start_array = start_array.unwrap(); if values.len() != start_array.len() { - return Err(ArrowError::ComputeError(format!( + return exec_err!( "start_array must be the same length as values array; got {} and {}", start_array.len(), - values.len(), - ))); + values.len() + ); } Ok(Arc::new( @@ -463,40 +478,41 @@ where }; let pattern = compile_and_cache_regex( + "regexp_count", regex, flags_scalar, &mut regex_cache, )?; count_matches(value, pattern, start) }) - .collect::>()?, + .collect::>()?, )) } (None, false, false) => { if values.len() != regex_array.len() { - return Err(ArrowError::ComputeError(format!( + return exec_err!( "regex_array must be the same length as values array; got {} and {}", regex_array.len(), - values.len(), - ))); + values.len() + ); } let start_array = start_array.unwrap(); if values.len() != start_array.len() { - return Err(ArrowError::ComputeError(format!( + return exec_err!( "start_array must be the same length as values array; got {} and {}", start_array.len(), - values.len(), - ))); + values.len() + ); } let flags_array = flags_array.unwrap(); if values.len() != flags_array.len() { - return Err(ArrowError::ComputeError(format!( + return exec_err!( "flags_array must be the same length as values array; got {} and {}", flags_array.len(), - values.len(), - ))); + values.len() + ); } Ok(Arc::new( @@ -511,11 +527,15 @@ where return Ok(None); }; - let pattern = - compile_and_cache_regex(regex, Some(flags), &mut regex_cache)?; + let pattern = compile_and_cache_regex( + "regexp_count", + regex, + Some(flags), + &mut regex_cache, + )?; count_matches(value, pattern, start) }) - .collect::>()?, + .collect::>()?, )) } } @@ -525,16 +545,14 @@ fn count_matches( value: Option<&str>, pattern: &Regex, start: Option, -) -> Result, ArrowError> { +) -> Result> { // A NULL value or start position produces a NULL result. let (Some(value), Some(start)) = (value, start) else { return Ok(None); }; if start < 1 { - return Err(ArrowError::ComputeError( - "regexp_count() requires start to be 1 based".to_string(), - )); + return exec_err!("regexp_count() requires start to be 1 based"); } let Some(byte_offset) = start_to_byte_offset(value, start) else { diff --git a/datafusion/functions/src/regex/regexpinstr.rs b/datafusion/functions/src/regex/regexpinstr.rs index de460c56f63c5..5e1f5293b4062 100644 --- a/datafusion/functions/src/regex/regexpinstr.rs +++ b/datafusion/functions/src/regex/regexpinstr.rs @@ -23,7 +23,6 @@ use arrow::datatypes::{DataType, Int64Type}; use arrow::datatypes::{ DataType::Int64, DataType::LargeUtf8, DataType::Utf8, DataType::Utf8View, }; -use arrow::error::ArrowError; use datafusion_common::{Result, ScalarValue, exec_err, internal_err}; use datafusion_expr::{ ColumnarValue, Documentation, ScalarFunctionArgs, ScalarUDFImpl, Signature, @@ -175,7 +174,6 @@ pub fn regexp_instr_func(args: &[ArrayRef]) -> Result { if args_len > 4 { Some(&args[4]) } else { None }, if args_len > 5 { Some(&args[5]) } else { None }, ) - .map_err(|e| e.into()) } /// `arrow-rs` style implementation of `regexp_instr` function. @@ -203,7 +201,7 @@ fn regexp_instr( nth_array: Option<&dyn Datum>, flags_array: Option<&dyn Datum>, subexpr_array: Option<&dyn Datum>, -) -> Result { +) -> Result { let (regex_array, _) = regex_array.get(); let start_array = start_array.map(|start| { let (start, _) = start.get(); @@ -231,14 +229,16 @@ fn regexp_instr( None, subexpr_array.map(|subexpr| subexpr.as_primitive::()), ), - (Utf8, Utf8, Some(flags_array)) if *flags_array.data_type() == Utf8 => regexp_instr_inner( - &values.as_string::(), - ®ex_array.as_string::(), - start_array.map(|start| start.as_primitive::()), - nth_array.map(|nth| nth.as_primitive::()), - Some(&flags_array.as_string::()), - subexpr_array.map(|subexpr| subexpr.as_primitive::()), - ), + (Utf8, Utf8, Some(flags_array)) if *flags_array.data_type() == Utf8 => { + regexp_instr_inner( + &values.as_string::(), + ®ex_array.as_string::(), + start_array.map(|start| start.as_primitive::()), + nth_array.map(|nth| nth.as_primitive::()), + Some(&flags_array.as_string::()), + subexpr_array.map(|subexpr| subexpr.as_primitive::()), + ) + } (LargeUtf8, LargeUtf8, None) => regexp_instr_inner( &values.as_string::(), ®ex_array.as_string::(), @@ -247,14 +247,18 @@ fn regexp_instr( None, subexpr_array.map(|subexpr| subexpr.as_primitive::()), ), - (LargeUtf8, LargeUtf8, Some(flags_array)) if *flags_array.data_type() == LargeUtf8 => regexp_instr_inner( - &values.as_string::(), - ®ex_array.as_string::(), - start_array.map(|start| start.as_primitive::()), - nth_array.map(|nth| nth.as_primitive::()), - Some(&flags_array.as_string::()), - subexpr_array.map(|subexpr| subexpr.as_primitive::()), - ), + (LargeUtf8, LargeUtf8, Some(flags_array)) + if *flags_array.data_type() == LargeUtf8 => + { + regexp_instr_inner( + &values.as_string::(), + ®ex_array.as_string::(), + start_array.map(|start| start.as_primitive::()), + nth_array.map(|nth| nth.as_primitive::()), + Some(&flags_array.as_string::()), + subexpr_array.map(|subexpr| subexpr.as_primitive::()), + ) + } (Utf8View, Utf8View, None) => regexp_instr_inner( &values.as_string_view(), ®ex_array.as_string_view(), @@ -263,17 +267,21 @@ fn regexp_instr( None, subexpr_array.map(|subexpr| subexpr.as_primitive::()), ), - (Utf8View, Utf8View, Some(flags_array)) if *flags_array.data_type() == Utf8View => regexp_instr_inner( - &values.as_string_view(), - ®ex_array.as_string_view(), - start_array.map(|start| start.as_primitive::()), - nth_array.map(|nth| nth.as_primitive::()), - Some(&flags_array.as_string_view()), - subexpr_array.map(|subexpr| subexpr.as_primitive::()), + (Utf8View, Utf8View, Some(flags_array)) + if *flags_array.data_type() == Utf8View => + { + regexp_instr_inner( + &values.as_string_view(), + ®ex_array.as_string_view(), + start_array.map(|start| start.as_primitive::()), + nth_array.map(|nth| nth.as_primitive::()), + Some(&flags_array.as_string_view()), + subexpr_array.map(|subexpr| subexpr.as_primitive::()), + ) + } + _ => internal_err!( + "regexp_instr() expected the input arrays to be of type Utf8, LargeUtf8, or Utf8View and the data types of the values, regex_array, and flags_array to match" ), - _ => Err(ArrowError::ComputeError( - "regexp_instr() expected the input arrays to be of type Utf8, LargeUtf8, or Utf8View and the data types of the values, regex_array, and flags_array to match".to_string(), - )), } } @@ -284,7 +292,7 @@ fn regexp_instr_inner<'a, S>( nth_array: Option<&Int64Array>, flags_array: Option<&S>, subexp_array: Option<&Int64Array>, -) -> Result +) -> Result where S: StringArrayType<'a>, { @@ -342,7 +350,7 @@ impl<'a> RegexCache<'a> { &mut self, regex: &'a str, flags: Option<&'a str>, - ) -> Result<&Regex, ArrowError> { + ) -> Result<&Regex> { let key = (regex, flags); let index = match self.last { Some((last_key, index)) if last_key == key => index, @@ -350,7 +358,8 @@ impl<'a> RegexCache<'a> { let index = match self.indices.entry(key) { Entry::Occupied(entry) => *entry.get(), Entry::Vacant(entry) => { - self.compiled.push(compile_regex(regex, flags)?); + self.compiled + .push(compile_regex("regexp_instr", regex, flags)?); *entry.insert(self.compiled.len() - 1) } }; @@ -372,17 +381,13 @@ fn get_index( start: i64, n: i64, subexpr: i64, -) -> Result { +) -> Result { if start < 1 { - return Err(ArrowError::ComputeError( - "regexp_instr() requires start to be 1-based".to_string(), - )); + return exec_err!("regexp_instr() requires start to be 1-based"); } if n < 1 { - return Err(ArrowError::ComputeError( - "N must be 1 or greater".to_string(), - )); + return exec_err!("N must be 1 or greater"); } let Some(byte_start_offset) = start_to_byte_offset(value, start) else { diff --git a/datafusion/physical-expr-common/Cargo.toml b/datafusion/physical-expr-common/Cargo.toml index 4276814fd04da..a4f8ebf91baee 100644 --- a/datafusion/physical-expr-common/Cargo.toml +++ b/datafusion/physical-expr-common/Cargo.toml @@ -60,6 +60,7 @@ indexmap = { workspace = true } itertools = { workspace = true } parking_lot = { workspace = true } pin-project = { workspace = true } +regex = { workspace = true } [dev-dependencies] criterion = { workspace = true } diff --git a/datafusion/physical-expr-common/src/lib.rs b/datafusion/physical-expr-common/src/lib.rs index b6eaacdca2505..131bea24ff275 100644 --- a/datafusion/physical-expr-common/src/lib.rs +++ b/datafusion/physical-expr-common/src/lib.rs @@ -35,6 +35,7 @@ pub mod binary_view_map; pub mod datum; pub mod metrics; pub mod physical_expr; +pub mod regex; pub mod sort_expr; pub mod tree_node; pub mod utils; diff --git a/datafusion/physical-expr-common/src/regex.rs b/datafusion/physical-expr-common/src/regex.rs new file mode 100644 index 0000000000000..3ca678d5d0f17 --- /dev/null +++ b/datafusion/physical-expr-common/src/regex.rs @@ -0,0 +1,150 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +//! Compilation of the regular expressions of SQL. +//! +//! The regexp functions and the `~` family of operators share this module, so +//! that a pattern that does not compile is reported in one way, wherever the +//! pattern came from. + +use arrow::array::{Array, AsArray}; +use arrow::datatypes::DataType; +use arrow::error::ArrowError; +use datafusion_common::{ + DataFusionError, Result, arrow_datafusion_err, exec_datafusion_err, plan_err, +}; +use regex::Regex; +use std::collections::HashMap; +use std::collections::hash_map::Entry; + +/// Compiles `regex` with [`compile_regex`], keeping the compiled pattern in +/// `regex_cache` under the key `(regex, flags)`. +pub fn compile_and_cache_regex<'strings, 'cache>( + function_name: &str, + regex: &'strings str, + flags: Option<&'strings str>, + regex_cache: &'cache mut HashMap<(&'strings str, Option<&'strings str>), Regex>, +) -> Result<&'cache Regex> +where + 'strings: 'cache, +{ + let result = match regex_cache.entry((regex, flags)) { + Entry::Occupied(occupied_entry) => occupied_entry.into_mut(), + Entry::Vacant(vacant_entry) => { + let compiled = compile_regex(function_name, regex, flags)?; + vacant_entry.insert(compiled) + } + }; + Ok(result) +} + +/// Compiles `regex`, applying `flags` as inline regex flags. +/// +/// `function_name` names the SQL function that the user called. It appears in +/// the error that reports an unsupported flag, so that every function reports +/// its own name. +/// +/// A pattern that does not compile is reported as +/// [`DataFusionError::Execution`] carrying the diagnosis of the `regex` crate, +/// which names the position and the reason the pattern was rejected. +pub fn compile_regex( + function_name: &str, + regex: &str, + flags: Option<&str>, +) -> Result { + let pattern = match flags { + None | Some("") => regex.to_string(), + Some(flags) => { + if flags.contains('g') { + return plan_err!( + "{function_name}() does not support the \"global\" option" + ); + } + format!("(?{flags}){regex}") + } + }; + + Regex::new(&pattern) + .map_err(|e| exec_datafusion_err!("Regular expression did not compile: {e}")) +} + +/// Explains a failure reported by one of the arrow regexp kernels. +/// +/// The kernels compile the patterns themselves and report a pattern that does +/// not compile as an opaque [`ArrowError::ComputeError`]. This compiles the +/// patterns that were given to the kernel and reports the first one that does +/// not compile, with the diagnosis of the `regex` crate. The diagnosis of a +/// syntax error quotes the pattern that caused it. +/// +/// This runs only after the kernel has failed, so a query that succeeds never +/// compiles a pattern twice. +/// +/// `patterns` and `flags` are the arrays that the kernel received. An argument +/// that was a scalar is held as an array of one value, which applies to every +/// row. If every pattern compiles, the failure has a different cause and the +/// original error is kept. +pub fn explain_regexp_kernel_error( + function_name: &str, + error: ArrowError, + patterns: &dyn Array, + flags: Option<&dyn Array>, +) -> DataFusionError { + let Some(patterns) = string_values(patterns) else { + return arrow_datafusion_err!(error); + }; + let flags = match flags.map(string_values) { + None => None, + Some(Some(flags)) => Some(flags), + // Flags of some other type are not what made the kernel fail. + Some(None) => return arrow_datafusion_err!(error), + }; + + let rows = patterns.len().max(flags.as_ref().map_or(0, Vec::len)); + for row in 0..rows { + // A NULL pattern or NULL flags produce a NULL result, not an error. + let Some(pattern) = broadcast_value(&patterns, row) else { + continue; + }; + let flags = flags.as_ref().and_then(|flags| broadcast_value(flags, row)); + if let Err(error) = compile_regex(function_name, pattern, flags) { + return error; + } + } + + arrow_datafusion_err!(error) +} + +/// Borrows the values of a string array of any of the three string types. +/// Returns `None` for an array of any other type. +fn string_values(array: &dyn Array) -> Option>> { + match array.data_type() { + DataType::Utf8 => Some(array.as_string::().iter().collect()), + DataType::LargeUtf8 => Some(array.as_string::().iter().collect()), + DataType::Utf8View => Some(array.as_string_view().iter().collect()), + _ => None, + } +} + +/// Reads the value of `row`, treating an array of a single value as a scalar +/// that applies to every row. +fn broadcast_value<'a>(values: &[Option<&'a str>], row: usize) -> Option<&'a str> { + if values.len() == 1 { + values[0] + } else { + values.get(row).copied().flatten() + } +} diff --git a/datafusion/sqllogictest/test_files/regexp/regexp_count.slt b/datafusion/sqllogictest/test_files/regexp/regexp_count.slt index ee8feb3cb18bf..9c06cbbd915b9 100644 --- a/datafusion/sqllogictest/test_files/regexp/regexp_count.slt +++ b/datafusion/sqllogictest/test_files/regexp/regexp_count.slt @@ -91,16 +91,13 @@ SELECT regexp_count('abc', 'x*', 4); ---- 1 -statement error -External error: query failed: DataFusion error: Arrow error: Compute error: regexp_count() requires start to be 1 based +statement error DataFusion error: Execution error: regexp_count\(\) requires start to be 1 based SELECT regexp_count('123123123123', '123', 0); -statement error -External error: query failed: DataFusion error: Arrow error: Compute error: regexp_count() requires start to be 1 based +statement error DataFusion error: Execution error: regexp_count\(\) requires start to be 1 based SELECT regexp_count('123123123123', '123', -3); -statement error -External error: statement failed: DataFusion error: Arrow error: Compute error: regexp_count() does not support global flag +statement error DataFusion error: Error during planning: regexp_count\(\) does not support the "global" option SELECT regexp_count('123123123123', '123', 1, 'g'); query I @@ -398,18 +395,28 @@ NULL # Invalid patterns and flags report the reason from the regex parser # -query error Arrow error: Compute error: Regular expression did not compile: regex parse error: +query error DataFusion error: Execution error: Regular expression did not compile: regex parse error: SELECT regexp_count(str, 'a(b') from empty_table; -query error Arrow error: Compute error: Regular expression did not compile: regex parse error: +query error DataFusion error: Execution error: Regular expression did not compile: regex parse error: SELECT regexp_count('abc', 'a(b'); -query error Arrow error: Compute error: Regular expression did not compile: regex parse error: +query error DataFusion error: Execution error: Regular expression did not compile: regex parse error: SELECT regexp_count('abc', 'a', 1, 'z'); -query error Arrow error: Compute error: regexp_count\(\)/regexp_instr\(\) does not support the global flag +query error DataFusion error: Error during planning: regexp_count\(\) does not support the "global" option SELECT regexp_count('abc', 'a', 1, 'g'); +# A pattern that varies per row is reported in the same way. +statement ok +CREATE TABLE t_invalid_pattern(str varchar, pattern varchar) AS VALUES ('abc', 'a(b'); + +query error DataFusion error: Execution error: Regular expression did not compile: regex parse error: +SELECT regexp_count(str, pattern) FROM t_invalid_pattern; + +statement ok +DROP TABLE t_invalid_pattern; + statement ok drop table t_stringview; diff --git a/datafusion/sqllogictest/test_files/regexp/regexp_instr.slt b/datafusion/sqllogictest/test_files/regexp/regexp_instr.slt index d5ee69cb18203..18a07afaf8d05 100644 --- a/datafusion/sqllogictest/test_files/regexp/regexp_instr.slt +++ b/datafusion/sqllogictest/test_files/regexp/regexp_instr.slt @@ -73,13 +73,13 @@ SELECT ---- 11 -statement error DataFusion error: Arrow error: Compute error: regexp_instr\(\) requires start to be 1-based +statement error DataFusion error: Execution error: regexp_instr\(\) requires start to be 1-based SELECT regexp_instr('123123123123', '123', 0); -statement error DataFusion error: Arrow error: Compute error: regexp_instr\(\) requires start to be 1-based +statement error DataFusion error: Execution error: regexp_instr\(\) requires start to be 1-based SELECT regexp_instr('123123123123', '123', -3); -statement error DataFusion error: Arrow error: Compute error: N must be 1 or greater +statement error DataFusion error: Execution error: N must be 1 or greater SELECT regexp_instr('abcabcabc', 'abc', 1, 0); query I @@ -250,18 +250,28 @@ SELECT regexp_instr(str, pattern) FROM t_alternating_pattern; # Invalid patterns and flags report the reason from the regex parser # -query error Arrow error: Compute error: Regular expression did not compile: regex parse error: +query error DataFusion error: Execution error: Regular expression did not compile: regex parse error: SELECT regexp_instr(str, 'a(b') FROM empty_table; -query error Arrow error: Compute error: Regular expression did not compile: regex parse error: +query error DataFusion error: Execution error: Regular expression did not compile: regex parse error: SELECT regexp_instr('abc', 'a(b'); -query error Arrow error: Compute error: Regular expression did not compile: regex parse error: +query error DataFusion error: Execution error: Regular expression did not compile: regex parse error: SELECT regexp_instr('abc', 'a', 1, 1, 'z'); -query error Arrow error: Compute error: regexp_count\(\)/regexp_instr\(\) does not support the global flag +query error DataFusion error: Error during planning: regexp_instr\(\) does not support the "global" option SELECT regexp_instr('abc', 'a', 1, 1, 'g'); +# A pattern that varies per row is reported in the same way. +statement ok +CREATE TABLE t_invalid_pattern(str varchar, pattern varchar) AS VALUES ('abc', 'a(b'); + +query error DataFusion error: Execution error: Regular expression did not compile: regex parse error: +SELECT regexp_instr(str, pattern) FROM t_invalid_pattern; + +statement ok +DROP TABLE t_invalid_pattern; + statement ok DROP TABLE t_stringview; From bbd0adbf725e5e5e22af4a0fa71de3fc7aac411a Mon Sep 17 00:00:00 2001 From: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com> Date: Tue, 15 Sep 2026 19:42:12 -0500 Subject: [PATCH 3/8] fix: report a DataFusion error for a regex that does not compile regexp_match, regexp_like, regexp_replace and the `~` family of operators hand the pattern to an arrow kernel, which compiles it and reports a failure as an opaque `ArrowError::ComputeError`. A user saw an internal error instead of the reason the pattern was rejected. The kernel keeps compiling the pattern. Only when it fails does `explain_regexp_kernel_error` compile the patterns again, to report the first one that does not compile with the diagnosis of the regex crate. A query that succeeds compiles the pattern exactly as many times as before. The "global" flag check in regexp_match now tests every flags string that contains 'g', so "gi" no longer reaches the kernel. Co-Authored-By: Claude Opus 5 --- .../examples/builtin_functions/regexp.rs | 3 +- datafusion/functions/src/regex/mod.rs | 1 + datafusion/functions/src/regex/regexplike.rs | 74 ++++++++++--------- datafusion/functions/src/regex/regexpmatch.rs | 57 +++++++++++--- .../functions/src/regex/regexpreplace.rs | 37 +++++----- .../src/expressions/binary/kernels.rs | 36 ++++++++- .../test_files/regexp/regexp_like.slt | 54 +++++++++++++- .../test_files/regexp/regexp_match.slt | 41 +++++++++- .../test_files/regexp/regexp_replace.slt | 25 +++++++ 9 files changed, 260 insertions(+), 68 deletions(-) diff --git a/datafusion-examples/examples/builtin_functions/regexp.rs b/datafusion-examples/examples/builtin_functions/regexp.rs index 97dc71b94e934..94f8315018d8c 100644 --- a/datafusion-examples/examples/builtin_functions/regexp.rs +++ b/datafusion-examples/examples/builtin_functions/regexp.rs @@ -162,7 +162,8 @@ pub async fn regexp() -> Result<()> { .collect() .await; - let expected = "Regular expression did not compile: CompiledTooBig"; + let expected = + "Regular expression did not compile: Compiled regex exceeds size limit"; assert_contains!(result.unwrap_err().to_string(), expected); // diff --git a/datafusion/functions/src/regex/mod.rs b/datafusion/functions/src/regex/mod.rs index 55482cd52698d..bf113b9c03622 100644 --- a/datafusion/functions/src/regex/mod.rs +++ b/datafusion/functions/src/regex/mod.rs @@ -22,6 +22,7 @@ use arrow::compute::kernels::{cmp::eq, nullif::nullif}; use datafusion_common::{Result, ScalarValue}; use std::sync::Arc; +pub(crate) use datafusion_physical_expr_common::regex::explain_regexp_kernel_error; // The compilation of a regular expression is shared with the physical // expressions, so that every caller reports a failure in the same way. These // re-exports keep the paths that callers of this crate already use. diff --git a/datafusion/functions/src/regex/regexplike.rs b/datafusion/functions/src/regex/regexplike.rs index 08cd7b06510b9..5e1ea70f3f4ef 100644 --- a/datafusion/functions/src/regex/regexplike.rs +++ b/datafusion/functions/src/regex/regexplike.rs @@ -17,14 +17,14 @@ //! Regex expressions -use arrow::array::{Array, ArrayRef, AsArray, BooleanArray, GenericStringArray}; +use arrow::array::{ + Array, ArrayRef, AsArray, BooleanArray, GenericStringArray, StringArray, +}; use arrow::compute::kernels::regexp; use arrow::datatypes::DataType; use arrow::datatypes::DataType::{LargeUtf8, Utf8, Utf8View}; use datafusion_common::types::logical_string; -use datafusion_common::{ - Result, ScalarValue, arrow_datafusion_err, exec_err, internal_err, plan_err, -}; +use datafusion_common::{Result, ScalarValue, exec_err, internal_err, plan_err}; use datafusion_expr::{ Coercion, ColumnarValue, Documentation, Expr, ScalarFunctionArgs, ScalarUDFImpl, Signature, TypeSignature, TypeSignatureClass, Volatility, binary_expr, cast, @@ -36,7 +36,7 @@ use datafusion_expr::simplify::{ }; use datafusion_expr_common::operator::Operator; use datafusion_expr_common::type_coercion::binary::BinaryTypeCoercer; -use regex::{Error as RegexError, Regex, RegexBuilder}; +use regex::{Error as RegexError, RegexBuilder}; use std::borrow::Cow; use std::sync::Arc; @@ -377,24 +377,32 @@ fn regexp_like_array_scalar( }; let flags = flags.filter(|flags| !flags.is_empty()); let array = match values.data_type() { - Utf8 => { - let array = values.as_string::(); - regexp::regexp_is_match_scalar(array, pattern, flags)? - } + Utf8 => regexp::regexp_is_match_scalar(values.as_string::(), pattern, flags), Utf8View => { - let array = values.as_string_view(); - regexp::regexp_is_match_scalar(array, pattern, flags)? + regexp::regexp_is_match_scalar(values.as_string_view(), pattern, flags) } LargeUtf8 => { - let array = values.as_string::(); - regexp::regexp_is_match_scalar(array, pattern, flags)? + regexp::regexp_is_match_scalar(values.as_string::(), pattern, flags) } other => { return internal_err!( "Unsupported data type {other:?} for function `regexp_like`" ); } - }; + } + // The kernel compiles the pattern itself. Describe the scalar pattern and + // flags as arrays of one value, so that a failure is explained the same + // way as on the paths that pass arrays. + .map_err(|error| { + let patterns = StringArray::from(vec![pattern]); + let flags = flags.map(|flags| StringArray::from(vec![flags])); + super::explain_regexp_kernel_error( + "regexp_like", + error, + &patterns, + flags.as_ref().map(|flags| flags as &dyn Array), + ) + })?; Ok(Arc::new(array)) } @@ -414,20 +422,15 @@ fn regexp_like_scalar( let value = value.unwrap(); let pattern = pattern.unwrap(); - let pattern = match flags.filter(|flags| !flags.is_empty()) { - Some(flagz) => format!("(?{flagz}){pattern}"), - None => pattern.to_string(), - }; + let flags = flags.filter(|flags| !flags.is_empty()); - let result = if pattern.is_empty() { + // An empty pattern matches every value and needs no compilation. Every + // other pattern is compiled exactly once, as on the paths that call a + // kernel. + let result = if pattern.is_empty() && flags.is_none() { true } else { - let re = Regex::new(pattern.as_str()).map_err(|e| { - datafusion_common::DataFusionError::Execution(format!( - "Regular expression did not compile: {e:?}" - )) - })?; - re.is_match(value) + super::compile_regex("regexp_like", pattern, flags)?.is_match(value) }; Ok(ColumnarValue::Scalar(ScalarValue::Boolean(Some(result)))) @@ -444,70 +447,71 @@ fn handle_regexp_like( let pattern = patterns.as_string::(); regexp::regexp_is_match(value, pattern, flags) - .map_err(|e| arrow_datafusion_err!(e))? } (Utf8View, Utf8View) => { let value = values.as_string_view(); let pattern = patterns.as_string_view(); regexp::regexp_is_match(value, pattern, flags) - .map_err(|e| arrow_datafusion_err!(e))? } (Utf8View, LargeUtf8) => { let value = values.as_string_view(); let pattern = patterns.as_string::(); regexp::regexp_is_match(value, pattern, flags) - .map_err(|e| arrow_datafusion_err!(e))? } (Utf8, Utf8) => { let value = values.as_string::(); let pattern = patterns.as_string::(); regexp::regexp_is_match(value, pattern, flags) - .map_err(|e| arrow_datafusion_err!(e))? } (Utf8, Utf8View) => { let value = values.as_string::(); let pattern = patterns.as_string_view(); regexp::regexp_is_match(value, pattern, flags) - .map_err(|e| arrow_datafusion_err!(e))? } (Utf8, LargeUtf8) => { let value = values.as_string::(); let pattern = patterns.as_string::(); regexp::regexp_is_match(value, pattern, flags) - .map_err(|e| arrow_datafusion_err!(e))? } (LargeUtf8, Utf8) => { let value = values.as_string::(); let pattern = patterns.as_string::(); regexp::regexp_is_match(value, pattern, flags) - .map_err(|e| arrow_datafusion_err!(e))? } (LargeUtf8, Utf8View) => { let value = values.as_string::(); let pattern = patterns.as_string_view(); regexp::regexp_is_match(value, pattern, flags) - .map_err(|e| arrow_datafusion_err!(e))? } (LargeUtf8, LargeUtf8) => { let value = values.as_string::(); let pattern = patterns.as_string::(); regexp::regexp_is_match(value, pattern, flags) - .map_err(|e| arrow_datafusion_err!(e))? } other => { return internal_err!( "Unsupported data type {other:?} for function `regexp_like`" ); } - }; + } + // Every arm hands its pattern to the kernel, which compiles it. Explain a + // failure in one place, for every arm. + .map_err(|error| { + super::explain_regexp_kernel_error( + "regexp_like", + error, + patterns.as_ref(), + flags.map(|flags| flags as &dyn Array), + ) + })?; Ok(Arc::new(array) as ArrayRef) } diff --git a/datafusion/functions/src/regex/regexpmatch.rs b/datafusion/functions/src/regex/regexpmatch.rs index 7ca8ba82d5f34..3be75ef5777d3 100644 --- a/datafusion/functions/src/regex/regexpmatch.rs +++ b/datafusion/functions/src/regex/regexpmatch.rs @@ -23,7 +23,7 @@ use arrow::datatypes::Field; use datafusion_common::Result; use datafusion_common::ScalarValue; use datafusion_common::exec_err; -use datafusion_common::{arrow_datafusion_err, plan_err}; +use datafusion_common::plan_err; use datafusion_expr::{ColumnarValue, Documentation, ScalarFunctionArgs, TypeSignature}; use datafusion_expr::{ScalarUDFImpl, Signature, Volatility}; use datafusion_macros::user_doc; @@ -185,7 +185,11 @@ fn regexp_match_scalar_pattern(args: &[ColumnarValue]) -> Result Result Result { match args.len() { - 2 => regexp::regexp_match(&args[0], &args[1], None) - .map_err(|e| arrow_datafusion_err!(e)), + 2 => regexp::regexp_match(&args[0], &args[1], None).map_err(|error| { + super::explain_regexp_kernel_error( + "regexp_match", + error, + args[1].as_ref(), + None, + ) + }), 3 => { match args[2].data_type() { DataType::Utf8View => { - if args[2].as_string_view().iter().any(|s| s == Some("g")) { + if args[2] + .as_string_view() + .iter() + .any(|s| s.is_some_and(|s| s.contains('g'))) + { return plan_err!( "regexp_match() does not support the \"global\" option" ); } } DataType::Utf8 => { - if args[2].as_string::().iter().any(|s| s == Some("g")) { + if args[2] + .as_string::() + .iter() + .any(|s| s.is_some_and(|s| s.contains('g'))) + { return plan_err!( "regexp_match() does not support the \"global\" option" ); } } DataType::LargeUtf8 => { - if args[2].as_string::().iter().any(|s| s == Some("g")) { + if args[2] + .as_string::() + .iter() + .any(|s| s.is_some_and(|s| s.contains('g'))) + { return plan_err!( "regexp_match() does not support the \"global\" option" ); @@ -241,8 +272,14 @@ pub fn regexp_match(args: &[ArrayRef]) -> Result { } let flags = super::normalize_empty_flags(&args[2])?; - regexp::regexp_match(&args[0], &args[1], Some(&flags)) - .map_err(|e| arrow_datafusion_err!(e)) + regexp::regexp_match(&args[0], &args[1], Some(&flags)).map_err(|error| { + super::explain_regexp_kernel_error( + "regexp_match", + error, + args[1].as_ref(), + Some(flags.as_ref()), + ) + }) } other => exec_err!( "regexp_match was called with {other} arguments. It requires at least 2 and at most 3." diff --git a/datafusion/functions/src/regex/regexpreplace.rs b/datafusion/functions/src/regex/regexpreplace.rs index f5bd6f182ac4b..6ad138de15a7f 100644 --- a/datafusion/functions/src/regex/regexpreplace.rs +++ b/datafusion/functions/src/regex/regexpreplace.rs @@ -32,9 +32,7 @@ use datafusion_common::cast::{ }; use datafusion_common::exec_err; use datafusion_common::plan_err; -use datafusion_common::{ - DataFusionError, Result, cast::as_generic_string_array, internal_err, -}; +use datafusion_common::{Result, cast::as_generic_string_array, internal_err}; use datafusion_expr::ColumnarValue; use datafusion_expr::TypeSignature; use datafusion_expr::function::Hint; @@ -43,6 +41,8 @@ use datafusion_expr::{ }; use datafusion_macros::user_doc; use regex::{CaptureLocations, Regex}; + +use super::compile_regex; use std::borrow::Cow; use std::collections::HashMap; use std::sync::{Arc, LazyLock}; @@ -361,15 +361,15 @@ where // if patterns hashmap already has regexp then use else create and return let re = match patterns.get(pattern) { Some(re) => Ok(re), - None => match Regex::new(pattern) { - Ok(re) => { - patterns.insert(pattern.to_string(), re); - Ok(patterns.get(pattern).unwrap()) - } - Err(err) => { - Err(DataFusionError::External(Box::new(err))) + None => { + match compile_regex("regexp_replace", pattern, None) { + Ok(re) => { + patterns.insert(pattern.to_string(), re); + Ok(patterns.get(pattern).unwrap()) + } + Err(err) => Err(err), } - }, + } }; Some(re.map(|re| re.replace(string, replacement.as_str()))) @@ -420,14 +420,16 @@ where // if patterns hashmap already has regexp then use else create and return let re = match patterns.get(&pattern) { Some(re) => Ok(re), - None => match Regex::new(pattern.as_str()) { + None => match compile_regex( + "regexp_replace", + pattern.as_str(), + None, + ) { Ok(re) => { patterns.insert(pattern.clone(), re); Ok(patterns.get(&pattern).unwrap()) } - Err(err) => { - Err(DataFusionError::External(Box::new(err))) - } + Err(err) => Err(err), }, }; @@ -537,8 +539,7 @@ fn regexp_replace_static_pattern_replace( None => (pattern.to_string(), 1), }; - let re = - Regex::new(&pattern).map_err(|err| DataFusionError::External(Box::new(err)))?; + let re = compile_regex("regexp_replace", &pattern, None)?; // Replaces the posix groups in the replacement string // with rust ones. @@ -976,7 +977,7 @@ mod tests { let pattern_err = re.expect_err("broken pattern should have failed"); assert_eq!( pattern_err.strip_backtrace(), - "External error: regex parse error:\n [\n ^\nerror: unclosed character class" + "Execution error: Regular expression did not compile: regex parse error:\n [\n ^\nerror: unclosed character class" ); } diff --git a/datafusion/physical-expr/src/expressions/binary/kernels.rs b/datafusion/physical-expr/src/expressions/binary/kernels.rs index 94612af9f6498..9a2d910ef98bf 100644 --- a/datafusion/physical-expr/src/expressions/binary/kernels.rs +++ b/datafusion/physical-expr/src/expressions/binary/kernels.rs @@ -28,6 +28,7 @@ use arrow::compute::kernels::comparison::{regexp_is_match, regexp_is_match_scala use arrow::datatypes::DataType; use datafusion_common::{Result, ScalarValue}; use datafusion_common::{exec_err, internal_err, plan_err}; +use datafusion_physical_expr_common::regex::explain_regexp_kernel_error; use std::sync::Arc; @@ -159,6 +160,16 @@ create_left_integral_dyn_scalar_kernel!( bitwise_shift_left_scalar ); +/// The SQL spelling of the operator, for error messages. +fn operator_name(not_match: bool, case_insensitive: bool) -> &'static str { + match (not_match, case_insensitive) { + (false, false) => "~", + (false, true) => "~*", + (true, false) => "!~", + (true, true) => "!~*", + } +} + /// Invoke a compute kernel on a pair of binary data arrays with flags macro_rules! regexp_is_match_flag { ($LEFT:expr, $RIGHT:expr, $ARRAYTYPE:ident, $NOT:expr, $FLAG:expr) => {{ @@ -183,7 +194,16 @@ macro_rules! regexp_is_match_flag { } else { None }; - let mut array = regexp_is_match(ll, rr, flag.as_ref())?; + // The kernel compiles the pattern of each row. A pattern that does + // not compile is explained only after the kernel has failed. + let mut array = regexp_is_match(ll, rr, flag.as_ref()).map_err(|error| { + explain_regexp_kernel_error( + operator_name($NOT, $FLAG), + error, + rr, + flag.as_ref().map(|flag| flag as &dyn Array), + ) + })?; if $NOT { array = not(&array).unwrap(); } @@ -233,7 +253,19 @@ macro_rules! regexp_is_match_flag_scalar { } Ok(Arc::new(array)) } - Err(e) => internal_err!("failed to call 'regex_match_dyn_scalar' {}", e), + Err(error) => { + // Describe the scalar pattern and flags as arrays of one + // value, so that the failure is explained the same way as + // on the paths that pass arrays. + let patterns = StringArray::from(vec![string_value]); + let flags = flag.map(|flag| StringArray::from(vec![flag])); + Err(explain_regexp_kernel_error( + operator_name($NOT, $FLAG), + error, + &patterns, + flags.as_ref().map(|flags| flags as &dyn Array), + )) + } } } else { internal_err!( diff --git a/datafusion/sqllogictest/test_files/regexp/regexp_like.slt b/datafusion/sqllogictest/test_files/regexp/regexp_like.slt index 4a649a196628c..08ed13c477ae0 100644 --- a/datafusion/sqllogictest/test_files/regexp/regexp_like.slt +++ b/datafusion/sqllogictest/test_files/regexp/regexp_like.slt @@ -148,7 +148,7 @@ SELECT regexp_like('bb-1', '.*-(\d)', 'g'); query error Error during planning: regexp_like\(\) does not support the "global" option SELECT regexp_like('bb-1', '.*-(\d)', 'g'); -query error Arrow error: Compute error: Regular expression did not compile: CompiledTooBig\(10485760\) +query error DataFusion error: Execution error: Regular expression did not compile: Compiled regex exceeds size limit of 10485760 bytes SELECT regexp_like('aaaaa', 'a{5}{5}{5}{5}{5}{5}{5}{5}{5}{5}{5}{5}{5}{5}{5}{5}{5}{5}'); # look-around is not supported and will just return false @@ -401,3 +401,55 @@ true true statement ok DROP TABLE regexp_empty_flags; + +# A literal pattern without flags, like every `~` operator, is simplified at +# planning time, so a pattern that does not compile is reported there. +query error DataFusion error: Optimizer rule 'simplify_expressions' failed[\s\S]*Invalid regex +SELECT regexp_like('abc', 'a(b'); + +statement ok +CREATE TABLE t_invalid_pattern(str varchar, pattern varchar) AS VALUES ('abc', 'a(b'), ('abc', NULL); + +query error DataFusion error: Optimizer rule 'simplify_expressions' failed[\s\S]*Invalid regex +SELECT regexp_like(str, 'a(b') FROM t_invalid_pattern; + +query error DataFusion error: Optimizer rule 'simplify_expressions' failed[\s\S]*Invalid regex +SELECT str ~ 'a(b' FROM t_invalid_pattern; + +# Flags that have no operator equivalent keep the call as a function, which +# gives the same diagnosis at execution time. +query error DataFusion error: Execution error: Regular expression did not compile: regex parse error +SELECT regexp_like('abc', 'a(b', 'm'); + +# An unknown flag makes the pattern fail to compile in the same way. +query error DataFusion error: Execution error: Regular expression did not compile: regex parse error +SELECT regexp_like('abc', 'a', 'z'); + +query error DataFusion error: Execution error: Regular expression did not compile: regex parse error +SELECT regexp_like(str, 'a', 'z') FROM t_invalid_pattern; + +# A pattern that varies per row is simplified to the `~` operator, which +# compiles each row's pattern inside the arrow kernel. The kernel failure is +# explained in the same way. +query error DataFusion error: Execution error: Regular expression did not compile: regex parse error +SELECT regexp_like(str, pattern) FROM t_invalid_pattern; + +query error DataFusion error: Execution error: Regular expression did not compile: regex parse error +SELECT str ~ pattern FROM t_invalid_pattern; + +query error DataFusion error: Execution error: Regular expression did not compile: regex parse error +SELECT str ~* pattern FROM t_invalid_pattern; + +# A NULL value or a NULL pattern gives NULL, and compiles nothing. +query B +SELECT regexp_like(NULL, 'a(b'); +---- +NULL + +query B +SELECT regexp_like('abc', NULL); +---- +NULL + +statement ok +DROP TABLE t_invalid_pattern; diff --git a/datafusion/sqllogictest/test_files/regexp/regexp_match.slt b/datafusion/sqllogictest/test_files/regexp/regexp_match.slt index b339812c12e1c..b589afa450235 100644 --- a/datafusion/sqllogictest/test_files/regexp/regexp_match.slt +++ b/datafusion/sqllogictest/test_files/regexp/regexp_match.slt @@ -125,7 +125,7 @@ SELECT regexp_match('bb-1', '.*-(\d)', 'g'); query error Error during planning: regexp_match\(\) does not support the "global" option SELECT regexp_match('bb-1', '.*-(\d)', 'g'); -query error Arrow error: Compute error: Regular expression did not compile: CompiledTooBig\(10485760\) +query error DataFusion error: Execution error: Regular expression did not compile: Compiled regex exceeds size limit of 10485760 bytes SELECT regexp_match('aaaaa', 'a{5}{5}{5}{5}{5}{5}{5}{5}{5}{5}{5}{5}{5}{5}{5}{5}{5}{5}'); # look-around is not supported and will just return null @@ -232,3 +232,42 @@ NULL [bar] statement ok DROP TABLE regexp_empty_flags; + +# A pattern that does not compile is reported with the diagnosis of the `regex` +# crate. The kernel compiles the pattern, and the diagnosis is produced only +# after the kernel has failed, so a query that succeeds compiles it once. +query error DataFusion error: Execution error: Regular expression did not compile: regex parse error +SELECT regexp_match('abc', 'a(b'); + +statement ok +CREATE TABLE t_invalid_pattern(str varchar, pattern varchar) AS VALUES ('abc', 'a(b'), ('abc', NULL); + +query error DataFusion error: Execution error: Regular expression did not compile: regex parse error +SELECT regexp_match(str, 'a(b') FROM t_invalid_pattern; + +# A pattern that varies per row is reported in the same way. +query error DataFusion error: Execution error: Regular expression did not compile: regex parse error +SELECT regexp_match(str, pattern) FROM t_invalid_pattern; + +# An unknown flag makes the pattern fail to compile in the same way. +query error DataFusion error: Execution error: Regular expression did not compile: regex parse error +SELECT regexp_match('abc', 'a', 'z'); + +# A NULL value or a NULL pattern gives NULL, and compiles nothing. +query ? +SELECT regexp_match(NULL, 'a(b'); +---- +NULL + +query ? +SELECT regexp_match('abc', NULL); +---- +NULL + +query ? +SELECT regexp_match(str, pattern) FROM t_invalid_pattern WHERE pattern IS NULL; +---- +NULL + +statement ok +DROP TABLE t_invalid_pattern; diff --git a/datafusion/sqllogictest/test_files/regexp/regexp_replace.slt b/datafusion/sqllogictest/test_files/regexp/regexp_replace.slt index 61f41eb2a563d..937c41dcf5d13 100644 --- a/datafusion/sqllogictest/test_files/regexp/regexp_replace.slt +++ b/datafusion/sqllogictest/test_files/regexp/regexp_replace.slt @@ -274,3 +274,28 @@ not-a-url true # cleanup statement ok DROP TABLE regexp_replace_optimized_cases; + +# A pattern that does not compile is reported with the diagnosis of the `regex` +# crate, in the same shape as the other regexp functions. +query error DataFusion error: Execution error: Regular expression did not compile: regex parse error +SELECT regexp_replace('abc', 'a(b', 'x'); + +statement ok +CREATE TABLE t_invalid_pattern(str varchar, pattern varchar) AS VALUES ('abc', 'a(b'); + +query error DataFusion error: Execution error: Regular expression did not compile: regex parse error +SELECT regexp_replace(str, pattern, 'x') FROM t_invalid_pattern; + +statement ok +DROP TABLE t_invalid_pattern; + +# An unknown flag makes the pattern fail to compile in the same way. +query error DataFusion error: Execution error: Regular expression did not compile: regex parse error +SELECT regexp_replace('abc', 'a', 'x', 'z'); + +# The "global" flag is what regexp_replace uses to replace every match, so it +# stays supported here. +query T +SELECT regexp_replace('abcabc', 'b', 'x', 'g'); +---- +axcaxc From e306835d56c71daaf281d46545933babbfa91df7 Mon Sep 17 00:00:00 2001 From: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com> Date: Tue, 15 Sep 2026 19:44:09 -0500 Subject: [PATCH 4/8] fix: report an invalid literal regex as a plan error `simplify_regex_expr` compiles a literal pattern to rewrite it, and reported a pattern that does not compile as `Invalid regex`, wrapping the diagnosis in an `External` error. A literal pattern that does not compile is an error in the query text, known before execution, so report it as a plan error carrying the diagnosis of the regex_syntax crate. Every two argument regexp_like is simplified to the `~` operator, so this is the error that the most common spelling produces. Its wording now matches the one that the same pattern produces at execution time. Co-Authored-By: Claude Opus 5 --- .../optimizer/src/simplify_expressions/regex.rs | 12 ++++++------ .../sqllogictest/test_files/regexp/regexp_like.slt | 9 +++++---- 2 files changed, 11 insertions(+), 10 deletions(-) diff --git a/datafusion/optimizer/src/simplify_expressions/regex.rs b/datafusion/optimizer/src/simplify_expressions/regex.rs index 97417bc112fbd..05176f722032f 100644 --- a/datafusion/optimizer/src/simplify_expressions/regex.rs +++ b/datafusion/optimizer/src/simplify_expressions/regex.rs @@ -16,7 +16,7 @@ // under the License. use datafusion_common::tree_node::Transformed; -use datafusion_common::{DataFusionError, Result, ScalarValue}; +use datafusion_common::{Result, ScalarValue, plan_err}; use datafusion_expr::{BinaryExpr, Expr, Like, Operator, lit}; use regex_syntax::hir::{Capture, Hir, HirKind, Literal, Look}; @@ -98,11 +98,11 @@ pub fn simplify_regex_expr( } } Err(e) => { - // error out early since the execution may fail anyways - return Err(DataFusionError::Context( - "Invalid regex".to_owned(), - Box::new(DataFusionError::External(Box::new(e))), - )); + // A literal pattern that does not compile is an error in the query + // text, and it is known here, before execution. Report it with the + // diagnosis of the `regex_syntax` crate, in the same shape as the + // error that the regexp functions report at execution time. + return plan_err!("Regular expression did not compile: {e}"); } } diff --git a/datafusion/sqllogictest/test_files/regexp/regexp_like.slt b/datafusion/sqllogictest/test_files/regexp/regexp_like.slt index 08ed13c477ae0..f06dcfedf39e7 100644 --- a/datafusion/sqllogictest/test_files/regexp/regexp_like.slt +++ b/datafusion/sqllogictest/test_files/regexp/regexp_like.slt @@ -403,17 +403,18 @@ statement ok DROP TABLE regexp_empty_flags; # A literal pattern without flags, like every `~` operator, is simplified at -# planning time, so a pattern that does not compile is reported there. -query error DataFusion error: Optimizer rule 'simplify_expressions' failed[\s\S]*Invalid regex +# planning time, so a pattern that does not compile is reported there, with the +# same wording that the function uses at execution time. +query error DataFusion error: Optimizer rule 'simplify_expressions' failed[\s\S]*Error during planning: Regular expression did not compile: regex parse error SELECT regexp_like('abc', 'a(b'); statement ok CREATE TABLE t_invalid_pattern(str varchar, pattern varchar) AS VALUES ('abc', 'a(b'), ('abc', NULL); -query error DataFusion error: Optimizer rule 'simplify_expressions' failed[\s\S]*Invalid regex +query error DataFusion error: Optimizer rule 'simplify_expressions' failed[\s\S]*Error during planning: Regular expression did not compile: regex parse error SELECT regexp_like(str, 'a(b') FROM t_invalid_pattern; -query error DataFusion error: Optimizer rule 'simplify_expressions' failed[\s\S]*Invalid regex +query error DataFusion error: Optimizer rule 'simplify_expressions' failed[\s\S]*Error during planning: Regular expression did not compile: regex parse error SELECT str ~ 'a(b' FROM t_invalid_pattern; # Flags that have no operator equivalent keep the call as a function, which From 807ca6a4a5b4b90456c479a637f2c187a4ab0626 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 16 Sep 2026 19:34:23 +0000 Subject: [PATCH 5/8] perf: explain a regexp kernel error without copying the pattern arrays `explain_regexp_kernel_error` collected the whole `patterns` array (and `flags`) into a `Vec>` before looking for the pattern that did not compile. The collection is proportional to the length of the arrays, so a single invalid pattern in a large batch allocated and copied once per row on the error path. Borrow the arrays instead, through an accessor that holds the typed array and reads a row on demand. Explaining an error now allocates nothing beyond the pattern that `compile_regex` builds, whatever the length of the batch. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01H9fJcUW5cbNf72vbWazRhh --- datafusion/physical-expr-common/src/regex.rs | 70 ++++++++++++++------ 1 file changed, 48 insertions(+), 22 deletions(-) diff --git a/datafusion/physical-expr-common/src/regex.rs b/datafusion/physical-expr-common/src/regex.rs index 3ca678d5d0f17..8f5ab48f2026b 100644 --- a/datafusion/physical-expr-common/src/regex.rs +++ b/datafusion/physical-expr-common/src/regex.rs @@ -21,7 +21,7 @@ //! that a pattern that does not compile is reported in one way, wherever the //! pattern came from. -use arrow::array::{Array, AsArray}; +use arrow::array::{Array, AsArray, LargeStringArray, StringArray, StringViewArray}; use arrow::datatypes::DataType; use arrow::error::ArrowError; use datafusion_common::{ @@ -103,23 +103,25 @@ pub fn explain_regexp_kernel_error( patterns: &dyn Array, flags: Option<&dyn Array>, ) -> DataFusionError { - let Some(patterns) = string_values(patterns) else { + let Some(patterns) = StringValues::new(patterns) else { return arrow_datafusion_err!(error); }; - let flags = match flags.map(string_values) { + let flags = match flags.map(StringValues::new) { None => None, Some(Some(flags)) => Some(flags), // Flags of some other type are not what made the kernel fail. Some(None) => return arrow_datafusion_err!(error), }; - let rows = patterns.len().max(flags.as_ref().map_or(0, Vec::len)); + let rows = patterns + .len() + .max(flags.as_ref().map_or(0, StringValues::len)); for row in 0..rows { // A NULL pattern or NULL flags produce a NULL result, not an error. - let Some(pattern) = broadcast_value(&patterns, row) else { + let Some(pattern) = patterns.broadcast_value(row) else { continue; }; - let flags = flags.as_ref().and_then(|flags| broadcast_value(flags, row)); + let flags = flags.as_ref().and_then(|flags| flags.broadcast_value(row)); if let Err(error) = compile_regex(function_name, pattern, flags) { return error; } @@ -128,23 +130,47 @@ pub fn explain_regexp_kernel_error( arrow_datafusion_err!(error) } -/// Borrows the values of a string array of any of the three string types. -/// Returns `None` for an array of any other type. -fn string_values(array: &dyn Array) -> Option>> { - match array.data_type() { - DataType::Utf8 => Some(array.as_string::().iter().collect()), - DataType::LargeUtf8 => Some(array.as_string::().iter().collect()), - DataType::Utf8View => Some(array.as_string_view().iter().collect()), - _ => None, - } +/// A string array of any of the three string types, read by row. +/// +/// This borrows the array that the kernel received, so that explaining an +/// error reads the rows it needs and allocates nothing, however long the +/// array is. +enum StringValues<'a> { + Utf8(&'a StringArray), + LargeUtf8(&'a LargeStringArray), + Utf8View(&'a StringViewArray), } -/// Reads the value of `row`, treating an array of a single value as a scalar -/// that applies to every row. -fn broadcast_value<'a>(values: &[Option<&'a str>], row: usize) -> Option<&'a str> { - if values.len() == 1 { - values[0] - } else { - values.get(row).copied().flatten() +impl<'a> StringValues<'a> { + /// Borrows `array`, or returns `None` for an array of any other type. + fn new(array: &'a dyn Array) -> Option { + match array.data_type() { + DataType::Utf8 => Some(Self::Utf8(array.as_string::())), + DataType::LargeUtf8 => Some(Self::LargeUtf8(array.as_string::())), + DataType::Utf8View => Some(Self::Utf8View(array.as_string_view())), + _ => None, + } + } + + fn len(&self) -> usize { + match self { + Self::Utf8(array) => array.len(), + Self::LargeUtf8(array) => array.len(), + Self::Utf8View(array) => array.len(), + } + } + + /// Reads the value of `row`, treating an array of a single value as a + /// scalar that applies to every row. + fn broadcast_value(&self, row: usize) -> Option<&'a str> { + let row = if self.len() == 1 { 0 } else { row }; + if row >= self.len() { + return None; + } + match *self { + Self::Utf8(array) => (!array.is_null(row)).then(|| array.value(row)), + Self::LargeUtf8(array) => (!array.is_null(row)).then(|| array.value(row)), + Self::Utf8View(array) => (!array.is_null(row)).then(|| array.value(row)), + } } } From 95c47ef2d9f0b55671576a7a8f202c0030fab365 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 16 Sep 2026 19:50:51 +0000 Subject: [PATCH 6/8] refactor: hide explain_regexp_kernel_error from the public API `explain_regexp_kernel_error` is `pub` because `datafusion-physical-expr` and `datafusion-functions` call it from their own crates, not because it is meant for callers outside the workspace. Mark it `#[doc(hidden)]`, as the rest of the workspace marks the items that are public only to cross a crate boundary. `compile_regex` and `compile_and_cache_regex` keep their documentation: they were already public API before this branch moved them here. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01H9fJcUW5cbNf72vbWazRhh --- datafusion/physical-expr-common/src/regex.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/datafusion/physical-expr-common/src/regex.rs b/datafusion/physical-expr-common/src/regex.rs index 8f5ab48f2026b..b88f18dcd2178 100644 --- a/datafusion/physical-expr-common/src/regex.rs +++ b/datafusion/physical-expr-common/src/regex.rs @@ -97,6 +97,10 @@ pub fn compile_regex( /// that was a scalar is held as an array of one value, which applies to every /// row. If every pattern compiles, the failure has a different cause and the /// original error is kept. +/// +/// This is `pub` only so that the crates that call the kernels can reach it. +// Not public API. +#[doc(hidden)] pub fn explain_regexp_kernel_error( function_name: &str, error: ArrowError, From 3f8ef4cd44910a63e07aacfbd83544a2517c72f7 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 16 Sep 2026 21:16:37 +0000 Subject: [PATCH 7/8] fix: skip NULL rows when explaining a regexp kernel failure The arrow regexp kernels produce NULL for a row whose value is NULL and never compile that row's pattern. `explain_regexp_kernel_error` compiled every pattern, so with two invalid patterns on two rows, one of them a NULL row, it reported the pattern the kernel had skipped rather than the one that actually made it fail. Pass the values array to the explanation on the call sites whose kernel compiles a pattern per row, and skip a row whose value is NULL. The kernels that compile one pattern up front, before reading any value, pass `None` and keep explaining that pattern whatever the values are. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01MiunrdKMjyE9ZRe5KoVtHf --- datafusion/functions/src/regex/regexplike.rs | 6 ++++++ datafusion/functions/src/regex/regexpmatch.rs | 5 +++++ datafusion/physical-expr-common/src/regex.rs | 17 ++++++++++++++++- .../src/expressions/binary/kernels.rs | 6 ++++++ .../test_files/regexp/regexp_like.slt | 18 ++++++++++++++++++ .../test_files/regexp/regexp_match.slt | 15 +++++++++++++++ 6 files changed, 66 insertions(+), 1 deletion(-) diff --git a/datafusion/functions/src/regex/regexplike.rs b/datafusion/functions/src/regex/regexplike.rs index 5e1ea70f3f4ef..546e1dcfb5137 100644 --- a/datafusion/functions/src/regex/regexplike.rs +++ b/datafusion/functions/src/regex/regexplike.rs @@ -399,6 +399,9 @@ fn regexp_like_array_scalar( super::explain_regexp_kernel_error( "regexp_like", error, + // The kernel compiles the one pattern up front, whatever the + // values are. + None, &patterns, flags.as_ref().map(|flags| flags as &dyn Array), ) @@ -508,6 +511,9 @@ fn handle_regexp_like( super::explain_regexp_kernel_error( "regexp_like", error, + // The kernel compiles the pattern of a row only if that row has a + // value. + Some(values.as_ref()), patterns.as_ref(), flags.map(|flags| flags as &dyn Array), ) diff --git a/datafusion/functions/src/regex/regexpmatch.rs b/datafusion/functions/src/regex/regexpmatch.rs index 3be75ef5777d3..e6ffe04c27b95 100644 --- a/datafusion/functions/src/regex/regexpmatch.rs +++ b/datafusion/functions/src/regex/regexpmatch.rs @@ -213,6 +213,9 @@ fn regexp_match_scalar_pattern(args: &[ColumnarValue]) -> Result Result { super::explain_regexp_kernel_error( "regexp_match", error, + Some(args[0].as_ref()), args[1].as_ref(), None, ) @@ -276,6 +280,7 @@ pub fn regexp_match(args: &[ArrayRef]) -> Result { super::explain_regexp_kernel_error( "regexp_match", error, + Some(args[0].as_ref()), args[1].as_ref(), Some(flags.as_ref()), ) diff --git a/datafusion/physical-expr-common/src/regex.rs b/datafusion/physical-expr-common/src/regex.rs index b88f18dcd2178..e0c32f04737be 100644 --- a/datafusion/physical-expr-common/src/regex.rs +++ b/datafusion/physical-expr-common/src/regex.rs @@ -98,12 +98,21 @@ pub fn compile_regex( /// row. If every pattern compiles, the failure has a different cause and the /// original error is kept. /// +/// `values` is the array of strings that the kernel matched against, given +/// only by the callers of a kernel that compiles the pattern of a row as it +/// reaches that row. Such a kernel produces NULL for a row whose value is +/// NULL without compiling that row's pattern, so a pattern that the kernel +/// never reached must not be reported as the one that failed. A kernel that +/// compiles a single pattern up front, before it reads any value, takes +/// `None`: it compiles that pattern whatever the values are. +/// /// This is `pub` only so that the crates that call the kernels can reach it. // Not public API. #[doc(hidden)] pub fn explain_regexp_kernel_error( function_name: &str, error: ArrowError, + values: Option<&dyn Array>, patterns: &dyn Array, flags: Option<&dyn Array>, ) -> DataFusionError { @@ -119,8 +128,14 @@ pub fn explain_regexp_kernel_error( let rows = patterns .len() - .max(flags.as_ref().map_or(0, StringValues::len)); + .max(flags.as_ref().map_or(0, StringValues::len)) + .max(values.map_or(0, Array::len)); for row in 0..rows { + // The kernel skips a row whose value is NULL, so it never compiled + // the pattern of that row. + if values.is_some_and(|values| row < values.len() && values.is_null(row)) { + continue; + } // A NULL pattern or NULL flags produce a NULL result, not an error. let Some(pattern) = patterns.broadcast_value(row) else { continue; diff --git a/datafusion/physical-expr/src/expressions/binary/kernels.rs b/datafusion/physical-expr/src/expressions/binary/kernels.rs index 9a2d910ef98bf..a0e7ac3fc8d69 100644 --- a/datafusion/physical-expr/src/expressions/binary/kernels.rs +++ b/datafusion/physical-expr/src/expressions/binary/kernels.rs @@ -200,6 +200,9 @@ macro_rules! regexp_is_match_flag { explain_regexp_kernel_error( operator_name($NOT, $FLAG), error, + // The kernel compiles the pattern of a row only if that row + // has a value. + Some(ll as &dyn Array), rr, flag.as_ref().map(|flag| flag as &dyn Array), ) @@ -262,6 +265,9 @@ macro_rules! regexp_is_match_flag_scalar { Err(explain_regexp_kernel_error( operator_name($NOT, $FLAG), error, + // The kernel compiles the one pattern up front, + // whatever the values are. + None, &patterns, flags.as_ref().map(|flags| flags as &dyn Array), )) diff --git a/datafusion/sqllogictest/test_files/regexp/regexp_like.slt b/datafusion/sqllogictest/test_files/regexp/regexp_like.slt index f06dcfedf39e7..3657a7122ced3 100644 --- a/datafusion/sqllogictest/test_files/regexp/regexp_like.slt +++ b/datafusion/sqllogictest/test_files/regexp/regexp_like.slt @@ -454,3 +454,21 @@ NULL statement ok DROP TABLE t_invalid_pattern; + +# The kernel gives NULL for a row whose value is NULL and never compiles that +# row's pattern, so the diagnosis names the pattern that made the kernel fail, +# not the one of a row the kernel skipped. +statement ok +CREATE TABLE t_null_value(str varchar, pattern varchar) AS VALUES (NULL, 'a(b'), ('abc', 'c[d'); + +query error DataFusion error: Execution error: Regular expression did not compile: regex parse error[\s\S]*unclosed character class +SELECT str ~ pattern FROM t_null_value; + +query error DataFusion error: Execution error: Regular expression did not compile: regex parse error[\s\S]*unclosed character class +SELECT regexp_like(str, pattern) FROM t_null_value; + +query error DataFusion error: Execution error: Regular expression did not compile: regex parse error[\s\S]*unclosed character class +SELECT regexp_like(str, pattern, 'm') FROM t_null_value; + +statement ok +DROP TABLE t_null_value; diff --git a/datafusion/sqllogictest/test_files/regexp/regexp_match.slt b/datafusion/sqllogictest/test_files/regexp/regexp_match.slt index b589afa450235..51c5d6276a98b 100644 --- a/datafusion/sqllogictest/test_files/regexp/regexp_match.slt +++ b/datafusion/sqllogictest/test_files/regexp/regexp_match.slt @@ -271,3 +271,18 @@ NULL statement ok DROP TABLE t_invalid_pattern; + +# The kernel gives NULL for a row whose value is NULL and never compiles that +# row's pattern, so the diagnosis names the pattern that made the kernel fail, +# not the one of a row the kernel skipped. +statement ok +CREATE TABLE t_null_value(str varchar, pattern varchar) AS VALUES (NULL, 'a(b'), ('abc', 'c[d'); + +query error DataFusion error: Execution error: Regular expression did not compile: regex parse error[\s\S]*unclosed character class +SELECT regexp_match(str, pattern) FROM t_null_value; + +query error DataFusion error: Execution error: Regular expression did not compile: regex parse error[\s\S]*unclosed character class +SELECT regexp_match(str, pattern, 'm') FROM t_null_value; + +statement ok +DROP TABLE t_null_value; From 270299c8fec32880df2868d304aaeb2e1739d9ff Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 16 Sep 2026 22:55:14 +0000 Subject: [PATCH 8/8] test: control the NULL rows that a regexp kernel skips Add the control that the rows the kernel skips give NULL on their own, so that the error of the other row is what the surrounding tests assert, and cover a NULL pattern beside a NULL value: the kernel skips a row of either kind, and neither row's pattern may be reported as the one that failed to compile. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01MiunrdKMjyE9ZRe5KoVtHf --- .../test_files/regexp/regexp_like.slt | 30 ++++++++++++++++--- .../test_files/regexp/regexp_match.slt | 30 ++++++++++++++++--- 2 files changed, 52 insertions(+), 8 deletions(-) diff --git a/datafusion/sqllogictest/test_files/regexp/regexp_like.slt b/datafusion/sqllogictest/test_files/regexp/regexp_like.slt index 3657a7122ced3..225fdb22158a9 100644 --- a/datafusion/sqllogictest/test_files/regexp/regexp_like.slt +++ b/datafusion/sqllogictest/test_files/regexp/regexp_like.slt @@ -455,11 +455,11 @@ NULL statement ok DROP TABLE t_invalid_pattern; -# The kernel gives NULL for a row whose value is NULL and never compiles that -# row's pattern, so the diagnosis names the pattern that made the kernel fail, -# not the one of a row the kernel skipped. +# A row whose value or whose pattern is NULL gives NULL, and the kernel never +# compiles that row's pattern. The diagnosis therefore names the pattern that +# made the kernel fail, not the one of a row the kernel skipped. statement ok -CREATE TABLE t_null_value(str varchar, pattern varchar) AS VALUES (NULL, 'a(b'), ('abc', 'c[d'); +CREATE TABLE t_null_value(str varchar, pattern varchar) AS VALUES (NULL, 'a(b'), ('abc', NULL), ('abc', 'c[d'); query error DataFusion error: Execution error: Regular expression did not compile: regex parse error[\s\S]*unclosed character class SELECT str ~ pattern FROM t_null_value; @@ -470,5 +470,27 @@ SELECT regexp_like(str, pattern) FROM t_null_value; query error DataFusion error: Execution error: Regular expression did not compile: regex parse error[\s\S]*unclosed character class SELECT regexp_like(str, pattern, 'm') FROM t_null_value; +# The control: on their own, the rows that the kernel skips give NULL, and the +# pattern that does not compile is never reported. +query B +SELECT str ~ pattern FROM t_null_value WHERE str IS NULL; +---- +NULL + +query B +SELECT str ~ pattern FROM t_null_value WHERE pattern IS NULL; +---- +NULL + +query B +SELECT regexp_like(str, pattern, 'm') FROM t_null_value WHERE str IS NULL; +---- +NULL + +query B +SELECT regexp_like(str, pattern, 'm') FROM t_null_value WHERE pattern IS NULL; +---- +NULL + statement ok DROP TABLE t_null_value; diff --git a/datafusion/sqllogictest/test_files/regexp/regexp_match.slt b/datafusion/sqllogictest/test_files/regexp/regexp_match.slt index 51c5d6276a98b..a62477d6bab50 100644 --- a/datafusion/sqllogictest/test_files/regexp/regexp_match.slt +++ b/datafusion/sqllogictest/test_files/regexp/regexp_match.slt @@ -272,11 +272,11 @@ NULL statement ok DROP TABLE t_invalid_pattern; -# The kernel gives NULL for a row whose value is NULL and never compiles that -# row's pattern, so the diagnosis names the pattern that made the kernel fail, -# not the one of a row the kernel skipped. +# A row whose value or whose pattern is NULL gives NULL, and the kernel never +# compiles that row's pattern. The diagnosis therefore names the pattern that +# made the kernel fail, not the one of a row the kernel skipped. statement ok -CREATE TABLE t_null_value(str varchar, pattern varchar) AS VALUES (NULL, 'a(b'), ('abc', 'c[d'); +CREATE TABLE t_null_value(str varchar, pattern varchar) AS VALUES (NULL, 'a(b'), ('abc', NULL), ('abc', 'c[d'); query error DataFusion error: Execution error: Regular expression did not compile: regex parse error[\s\S]*unclosed character class SELECT regexp_match(str, pattern) FROM t_null_value; @@ -284,5 +284,27 @@ SELECT regexp_match(str, pattern) FROM t_null_value; query error DataFusion error: Execution error: Regular expression did not compile: regex parse error[\s\S]*unclosed character class SELECT regexp_match(str, pattern, 'm') FROM t_null_value; +# The control: on their own, the rows that the kernel skips give NULL, and the +# pattern that does not compile is never reported. +query ? +SELECT regexp_match(str, pattern) FROM t_null_value WHERE str IS NULL; +---- +NULL + +query ? +SELECT regexp_match(str, pattern) FROM t_null_value WHERE pattern IS NULL; +---- +NULL + +query ? +SELECT regexp_match(str, pattern, 'm') FROM t_null_value WHERE str IS NULL; +---- +NULL + +query ? +SELECT regexp_match(str, pattern, 'm') FROM t_null_value WHERE pattern IS NULL; +---- +NULL + statement ok DROP TABLE t_null_value;