From b04cc1d1f36933db74322c7ebc48ec6f54eb40da Mon Sep 17 00:00:00 2001 From: naman Date: Fri, 18 Sep 2026 04:22:07 +0530 Subject: [PATCH 1/2] fix: Emit LIKE the way the Substrait extension defines it The extension defines one function, `like`, taking two arguments and carrying case sensitivity as the `case_sensitivity` option. The producer emitted three arguments, registered `ilike` for the case insensitive form, which no extension defines, and never set the option. substrait-java resolves `ilike` to nothing, and Spark rejects the three argument call with WRONG_NUM_ARGS. The producer now registers `like`, emits the two arguments, and sets the option for ILIKE. An escape character has no place in that definition, so LIKE ... ESCAPE is rejected rather than emitted as a third argument. The consumer reads the option, taking the first value it supports, and still accepts the `ilike` name and the three argument form so that plans from an older DataFusion keep loading. --- .../consumer/expr/scalar_function.rs | 31 +++- .../producer/expr/scalar_function.rs | 149 ++++++++++++++++-- 2 files changed, 162 insertions(+), 18 deletions(-) diff --git a/datafusion/substrait/src/logical_plan/consumer/expr/scalar_function.rs b/datafusion/substrait/src/logical_plan/consumer/expr/scalar_function.rs index 13f86997f31ca..0c8fc819eb1eb 100644 --- a/datafusion/substrait/src/logical_plan/consumer/expr/scalar_function.rs +++ b/datafusion/substrait/src/logical_plan/consumer/expr/scalar_function.rs @@ -187,6 +187,32 @@ fn arg_list_to_binary_op_tree_inner( })) } +/// Reads the `case_sensitivity` option of a `like` call. +/// +/// Substrait says a consumer must use the first value it supports, and must +/// reject the call when it supports none of them. +fn case_insensitive_option(f: &ScalarFunction) -> Result { + let Some(option) = f + .options + .iter() + .find(|option| option.name.eq_ignore_ascii_case("case_sensitivity")) + else { + return Ok(false); + }; + for preference in &option.preference { + if preference.eq_ignore_ascii_case("CASE_SENSITIVE") { + return Ok(false); + } + if preference.eq_ignore_ascii_case("CASE_INSENSITIVE") { + return Ok(true); + } + } + not_impl_err!( + "Unsupported case_sensitivity for `like`: {:?}", + option.preference + ) +} + /// Build [`Expr`] from its name and required inputs. struct BuiltinExprBuilder { expr_name: String, @@ -213,7 +239,10 @@ impl BuiltinExprBuilder { args: Vec, ) -> Result { match self.expr_name.as_str() { - "like" => Self::build_like_expr(false, false, f, args), + // `like` carries case sensitivity as an option. `ilike` is not a + // Substrait function, but DataFusion used to emit it, so plans + // written by an older version are still read. + "like" => Self::build_like_expr(case_insensitive_option(f)?, false, f, args), "ilike" => Self::build_like_expr(true, false, f, args), "like_match" => Self::build_like_expr(false, false, f, args), "like_imatch" => Self::build_like_expr(true, false, f, args), diff --git a/datafusion/substrait/src/logical_plan/producer/expr/scalar_function.rs b/datafusion/substrait/src/logical_plan/producer/expr/scalar_function.rs index 8deee8b657284..6f72e7c575cdd 100644 --- a/datafusion/substrait/src/logical_plan/producer/expr/scalar_function.rs +++ b/datafusion/substrait/src/logical_plan/producer/expr/scalar_function.rs @@ -16,17 +16,17 @@ // under the License. use crate::logical_plan::producer::{ - SubstraitProducer, to_substrait_literal_expr, to_substrait_type, - to_substrait_type_from_field, + SubstraitProducer, to_substrait_type, to_substrait_type_from_field, }; use datafusion::arrow::datatypes::DataType; use datafusion::common::datatype::FieldExt; use datafusion::common::{ - DFSchemaRef, ScalarValue, internal_datafusion_err, not_impl_err, substrait_err, + DFSchemaRef, internal_datafusion_err, not_impl_err, substrait_err, }; use datafusion::logical_expr::{ Between, BinaryExpr, Expr, ExprSchemable, Like, Operator, expr, }; +use substrait::proto::FunctionOption; use substrait::proto::expression::{RexType, ScalarFunction}; use substrait::proto::function_argument::ArgType; use substrait::proto::{Expression, FunctionArgument, Type}; @@ -234,6 +234,11 @@ pub fn from_binary_expr( )) } +/// The option `like` uses to carry case sensitivity, and the value that asks +/// for the case insensitive form. Defined in `functions_string.yaml`. +pub(crate) const CASE_SENSITIVITY_OPTION: &str = "case_sensitivity"; +pub(crate) const CASE_INSENSITIVE: &str = "CASE_INSENSITIVE"; + pub fn from_like( producer: &mut impl SubstraitProducer, like: &Like, @@ -246,11 +251,6 @@ pub fn from_like( escape_char, case_insensitive, } = like; - let function_anchor = if *case_insensitive { - producer.register_function("ilike".to_string()) - } else { - producer.register_function("like".to_string()) - }; // Substrait documents `output_type` as "Must be set to the return type of // the function, exactly as derived using the declaration in the extension", // and a consumer that reads it rejects the call when it is unset. The type @@ -258,12 +258,41 @@ pub fn from_like( // derives, rather than being restated here. let (_, output_field) = Expr::Like(like.clone()).to_field(schema)?; let output_type = to_substrait_type_from_field(producer, &output_field)?; + + make_substrait_like_expr( + producer, + *case_insensitive, + *negated, + expr, + pattern, + *escape_char, + schema, + output_type, + ) +} + +#[expect(clippy::too_many_arguments)] +fn make_substrait_like_expr( + producer: &mut impl SubstraitProducer, + ignore_case: bool, + negated: bool, + expr: &Expr, + pattern: &Expr, + escape_char: Option, + schema: &DFSchemaRef, + output_type: Type, +) -> datafusion::common::Result { + // `like` takes two arguments and carries case sensitivity as an option; + // the extensions define no `ilike` and no escape character, so an escape + // has no encoding here and is rejected rather than emitted as a third + // argument that a consumer would bind to a parameter the function does + // not have. + if escape_char.is_some() { + return not_impl_err!("Substrait does not define an escape character for `like`"); + } + let function_anchor = producer.register_function("like".to_string()); let expr = producer.handle_expr(expr, schema)?; let pattern = producer.handle_expr(pattern, schema)?; - let escape_char = to_substrait_literal_expr( - producer, - &ScalarValue::Utf8(escape_char.map(|c| c.to_string())), - )?; let arguments = vec![ FunctionArgument { arg_type: Some(ArgType::Value(expr)), @@ -271,10 +300,16 @@ pub fn from_like( FunctionArgument { arg_type: Some(ArgType::Value(pattern)), }, - FunctionArgument { - arg_type: Some(ArgType::Value(escape_char)), - }, ]; + // An unset option leaves the default, which is `CASE_SENSITIVE`. + let options = if ignore_case { + vec![FunctionOption { + name: CASE_SENSITIVITY_OPTION.to_string(), + preference: vec![CASE_INSENSITIVE.to_string()], + }] + } else { + vec![] + }; #[expect(deprecated)] let substrait_like = Expression { @@ -283,11 +318,11 @@ pub fn from_like( arguments, output_type: Some(output_type.clone()), args: vec![], - options: vec![], + options, })), }; - if *negated { + if negated { let function_anchor = producer.register_function("not".to_string()); #[expect(deprecated)] @@ -438,6 +473,7 @@ pub fn operator_to_name(op: Operator) -> &'static str { #[cfg(test)] mod tests { + use super::{CASE_INSENSITIVE, CASE_SENSITIVITY_OPTION}; use crate::logical_plan::producer::{ DefaultSubstraitProducer, SubstraitProducer, to_substrait_type, }; @@ -447,9 +483,88 @@ mod tests { use datafusion::logical_expr::{Expr, Like}; use datafusion::prelude::{col, lit}; use substrait::proto::Expression; + use substrait::proto::FunctionOption; use substrait::proto::expression::{RexType, ScalarFunction}; use substrait::proto::function_argument::ArgType; + /// `like` takes two arguments and carries case sensitivity as an option, + /// so `ILIKE` is that option rather than a separate function. + #[tokio::test] + async fn like_emits_case_sensitivity_option() -> datafusion::common::Result<()> { + let state = SessionStateBuilder::default().build(); + let schema = + DFSchemaRef::new(DFSchema::try_from(Schema::new(vec![Field::new( + "s", + DataType::Utf8, + true, + )]))?); + + for (case_insensitive, expected_options) in [ + (false, vec![]), + ( + true, + vec![FunctionOption { + name: CASE_SENSITIVITY_OPTION.to_string(), + preference: vec![CASE_INSENSITIVE.to_string()], + }], + ), + ] { + let mut producer = DefaultSubstraitProducer::new(&state); + let like = Like::new( + false, + Box::new(col("s")), + Box::new(lit("a%")), + None, + case_insensitive, + ); + let expr = producer.handle_expr(&Expr::Like(like), &schema)?; + + let Some(RexType::ScalarFunction(call)) = expr.rex_type else { + panic!("Substrait ScalarFunction expected") + }; + assert_eq!( + producer + .get_extensions() + .functions + .get(&call.function_reference), + Some(&"like".to_string()), + "case_insensitive = {case_insensitive}" + ); + assert_eq!(call.arguments.len(), 2, "no escape argument is emitted"); + assert_eq!(call.options, expected_options); + } + + Ok(()) + } + + /// The extensions define no escape character, so there is nothing to emit. + #[tokio::test] + async fn like_with_escape_is_rejected() -> datafusion::common::Result<()> { + let state = SessionStateBuilder::default().build(); + let schema = + DFSchemaRef::new(DFSchema::try_from(Schema::new(vec![Field::new( + "s", + DataType::Utf8, + true, + )]))?); + let mut producer = DefaultSubstraitProducer::new(&state); + + let like = Like::new( + false, + Box::new(col("s")), + Box::new(lit("a!%")), + Some('!'), + false, + ); + let err = producer + .handle_expr(&Expr::Like(like), &schema) + .expect_err("an escape character must be rejected") + .to_string(); + assert!(err.contains("escape character"), "unexpected error: {err}"); + + Ok(()) + } + #[tokio::test] async fn binary_expr_output_type() -> datafusion::common::Result<()> { let state = SessionStateBuilder::default().build(); From 231c2e4ea93a07a9627fae0fc1e6bc0e74549383 Mon Sep 17 00:00:00 2001 From: namanjain24-sudo <180642416+namanjain24-sudo@users.noreply.github.com> Date: Tue, 6 Oct 2026 19:04:23 +0530 Subject: [PATCH 2/2] refactor: inline make_substrait_like_expr and add case_insensitive_option test coverage - from_like now builds the Substrait call directly instead of delegating to a single-caller helper, dropping the too_many_arguments suppression. - Add case_insensitive_option_cases, a table-driven test covering preference ordering (including CASE_INSENSITIVE_ASCII, which this consumer does not support and must skip), mixed-case option/preference names, an empty preference list, and rejection when nothing in the list is supported. --- .../consumer/expr/scalar_function.rs | 81 ++++++++++++++++++- .../producer/expr/scalar_function.rs | 51 ++++-------- 2 files changed, 94 insertions(+), 38 deletions(-) diff --git a/datafusion/substrait/src/logical_plan/consumer/expr/scalar_function.rs b/datafusion/substrait/src/logical_plan/consumer/expr/scalar_function.rs index 0c8fc819eb1eb..c688b25de27c8 100644 --- a/datafusion/substrait/src/logical_plan/consumer/expr/scalar_function.rs +++ b/datafusion/substrait/src/logical_plan/consumer/expr/scalar_function.rs @@ -403,7 +403,7 @@ impl BuiltinExprBuilder { #[cfg(test)] mod tests { - use super::arg_list_to_binary_op_tree; + use super::{arg_list_to_binary_op_tree, case_insensitive_option}; use crate::extensions::Extensions; use crate::logical_plan::consumer::tests::TEST_SESSION_STATE; use crate::logical_plan::consumer::{DefaultSubstraitConsumer, SubstraitConsumer}; @@ -612,4 +612,83 @@ mod tests { Ok(()) } + + fn scalar_function_with_case_sensitivity_preference( + preference: &[&str], + ) -> ScalarFunction { + ScalarFunction { + options: vec![substrait::proto::FunctionOption { + name: "case_sensitivity".to_string(), + preference: preference.iter().map(|s| s.to_string()).collect(), + }], + ..Default::default() + } + } + + /// Substrait says a consumer must use the first value it supports in the + /// `case_sensitivity` option's preference list, and must reject the call + /// when it supports none of them. + #[test] + fn case_insensitive_option_cases() -> Result<()> { + // No `case_sensitivity` option at all defaults to case-sensitive. + assert!(!case_insensitive_option(&ScalarFunction::default())?); + + // A single supported preference, in either sensitivity. + assert!(!case_insensitive_option( + &scalar_function_with_case_sensitivity_preference(&["CASE_SENSITIVE"]) + )?); + assert!(case_insensitive_option( + &scalar_function_with_case_sensitivity_preference(&["CASE_INSENSITIVE"]) + )?); + + // The option name and its preference values are matched + // case-insensitively (ASCII). + assert!(case_insensitive_option(&ScalarFunction { + options: vec![substrait::proto::FunctionOption { + name: "Case_Sensitivity".to_string(), + preference: vec!["case_insensitive".to_string()], + }], + ..Default::default() + })?); + + // Preference ordering: a consumer must use the first value it + // supports, not necessarily the first value in the list. + // `CASE_INSENSITIVE_ASCII` is not a value this consumer supports, so + // it is skipped in favor of the next, supported preference. + assert!(case_insensitive_option( + &scalar_function_with_case_sensitivity_preference(&[ + "CASE_INSENSITIVE_ASCII", + "CASE_INSENSITIVE" + ]) + )?); + assert!(!case_insensitive_option( + &scalar_function_with_case_sensitivity_preference(&[ + "CASE_INSENSITIVE_ASCII", + "CASE_SENSITIVE" + ]) + )?); + + // Rejection: every preference is unsupported. + let err = + case_insensitive_option(&scalar_function_with_case_sensitivity_preference( + &["CASE_INSENSITIVE_ASCII"], + )) + .unwrap_err(); + assert!( + err.to_string().contains("Unsupported case_sensitivity"), + "unexpected error: {err}" + ); + + // Rejection: an empty preference list supports nothing. + let err = case_insensitive_option( + &scalar_function_with_case_sensitivity_preference(&[]), + ) + .unwrap_err(); + assert!( + err.to_string().contains("Unsupported case_sensitivity"), + "unexpected error: {err}" + ); + + Ok(()) + } } diff --git a/datafusion/substrait/src/logical_plan/producer/expr/scalar_function.rs b/datafusion/substrait/src/logical_plan/producer/expr/scalar_function.rs index 6f72e7c575cdd..2f4fb5c1ce864 100644 --- a/datafusion/substrait/src/logical_plan/producer/expr/scalar_function.rs +++ b/datafusion/substrait/src/logical_plan/producer/expr/scalar_function.rs @@ -251,37 +251,6 @@ pub fn from_like( escape_char, case_insensitive, } = like; - // Substrait documents `output_type` as "Must be set to the return type of - // the function, exactly as derived using the declaration in the extension", - // and a consumer that reads it rejects the call when it is unset. The type - // comes from the expression itself so that it matches what DataFusion - // derives, rather than being restated here. - let (_, output_field) = Expr::Like(like.clone()).to_field(schema)?; - let output_type = to_substrait_type_from_field(producer, &output_field)?; - - make_substrait_like_expr( - producer, - *case_insensitive, - *negated, - expr, - pattern, - *escape_char, - schema, - output_type, - ) -} - -#[expect(clippy::too_many_arguments)] -fn make_substrait_like_expr( - producer: &mut impl SubstraitProducer, - ignore_case: bool, - negated: bool, - expr: &Expr, - pattern: &Expr, - escape_char: Option, - schema: &DFSchemaRef, - output_type: Type, -) -> datafusion::common::Result { // `like` takes two arguments and carries case sensitivity as an option; // the extensions define no `ilike` and no escape character, so an escape // has no encoding here and is rejected rather than emitted as a third @@ -290,19 +259,27 @@ fn make_substrait_like_expr( if escape_char.is_some() { return not_impl_err!("Substrait does not define an escape character for `like`"); } + // Substrait documents `output_type` as "Must be set to the return type of + // the function, exactly as derived using the declaration in the extension", + // and a consumer that reads it rejects the call when it is unset. The type + // comes from the expression itself so that it matches what DataFusion + // derives, rather than being restated here. + let (_, output_field) = Expr::Like(like.clone()).to_field(schema)?; + let output_type = to_substrait_type_from_field(producer, &output_field)?; + let function_anchor = producer.register_function("like".to_string()); - let expr = producer.handle_expr(expr, schema)?; - let pattern = producer.handle_expr(pattern, schema)?; + let substrait_expr = producer.handle_expr(expr, schema)?; + let substrait_pattern = producer.handle_expr(pattern, schema)?; let arguments = vec![ FunctionArgument { - arg_type: Some(ArgType::Value(expr)), + arg_type: Some(ArgType::Value(substrait_expr)), }, FunctionArgument { - arg_type: Some(ArgType::Value(pattern)), + arg_type: Some(ArgType::Value(substrait_pattern)), }, ]; // An unset option leaves the default, which is `CASE_SENSITIVE`. - let options = if ignore_case { + let options = if *case_insensitive { vec![FunctionOption { name: CASE_SENSITIVITY_OPTION.to_string(), preference: vec![CASE_INSENSITIVE.to_string()], @@ -322,7 +299,7 @@ fn make_substrait_like_expr( })), }; - if negated { + if *negated { let function_anchor = producer.register_function("not".to_string()); #[expect(deprecated)]