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-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 3877251c66a45..bf113b9c03622 100644 --- a/datafusion/functions/src/regex/mod.rs +++ b/datafusion/functions/src/regex/mod.rs @@ -19,12 +19,16 @@ 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; + +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. +pub use datafusion_physical_expr_common::regex::{ + compile_and_cache_regex, compile_regex, +}; pub mod regexpcount; pub mod regexpinstr; pub mod regexplike; @@ -139,24 +143,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 +159,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(|_| { - ArrowError::ComputeError(format!("Regular expression did not compile: {pattern}")) - }) -} - #[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/functions/src/regex/regexplike.rs b/datafusion/functions/src/regex/regexplike.rs index 08cd7b06510b9..546e1dcfb5137 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,35 @@ 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, + // The kernel compiles the one pattern up front, whatever the + // values are. + None, + &patterns, + flags.as_ref().map(|flags| flags as &dyn Array), + ) + })?; Ok(Arc::new(array)) } @@ -414,20 +425,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 +450,74 @@ 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, + // 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), + ) + })?; Ok(Arc::new(array) as ArrayRef) } diff --git a/datafusion/functions/src/regex/regexpmatch.rs b/datafusion/functions/src/regex/regexpmatch.rs index 7ca8ba82d5f34..e6ffe04c27b95 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, + Some(args[0].as_ref()), + 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 +276,15 @@ 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, + Some(args[0].as_ref()), + 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/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/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..e0c32f04737be --- /dev/null +++ b/datafusion/physical-expr-common/src/regex.rs @@ -0,0 +1,195 @@ +// 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, LargeStringArray, StringArray, StringViewArray}; +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. +/// +/// `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 { + let Some(patterns) = StringValues::new(patterns) else { + return arrow_datafusion_err!(error); + }; + 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, 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; + }; + let flags = flags.as_ref().and_then(|flags| flags.broadcast_value(row)); + if let Err(error) = compile_regex(function_name, pattern, flags) { + return error; + } + } + + arrow_datafusion_err!(error) +} + +/// 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), +} + +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)), + } + } +} diff --git a/datafusion/physical-expr/src/expressions/binary/kernels.rs b/datafusion/physical-expr/src/expressions/binary/kernels.rs index 94612af9f6498..a0e7ac3fc8d69 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,19 @@ 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, + // 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), + ) + })?; if $NOT { array = not(&array).unwrap(); } @@ -233,7 +256,22 @@ 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, + // The kernel compiles the one pattern up front, + // whatever the values are. + None, + &patterns, + flags.as_ref().map(|flags| flags as &dyn Array), + )) + } } } else { internal_err!( diff --git a/datafusion/sqllogictest/test_files/regexp/regexp_count.slt b/datafusion/sqllogictest/test_files/regexp/regexp_count.slt index 1f1500a6ca7b1..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 @@ -394,6 +391,32 @@ NULL NULL NULL +# +# Invalid patterns and flags report the reason from the regex parser +# + +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 DataFusion error: Execution error: Regular expression did not compile: regex parse error: +SELECT regexp_count('abc', 'a(b'); + +query error DataFusion error: Execution error: Regular expression did not compile: regex parse error: +SELECT regexp_count('abc', 'a', 1, 'z'); + +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 14e1ffbdf77a3..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 @@ -246,6 +246,32 @@ SELECT regexp_instr(str, pattern) FROM t_alternating_pattern; 4 1 +# +# Invalid patterns and flags report the reason from the regex parser +# + +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 DataFusion error: Execution error: Regular expression did not compile: regex parse error: +SELECT regexp_instr('abc', 'a(b'); + +query error DataFusion error: Execution error: Regular expression did not compile: regex parse error: +SELECT regexp_instr('abc', 'a', 1, 1, 'z'); + +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; diff --git a/datafusion/sqllogictest/test_files/regexp/regexp_like.slt b/datafusion/sqllogictest/test_files/regexp/regexp_like.slt index 4a649a196628c..225fdb22158a9 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,96 @@ 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, 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]*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]*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 +# 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; + +# 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', 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; + +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; + +# 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 b339812c12e1c..a62477d6bab50 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,79 @@ 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; + +# 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', 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; + +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; 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