Skip to content

Commit f868d60

Browse files
fix: Emit LIKE the way the Substrait extension defines it (#25442)
## Which issue does this PR close? - Closes #25368. ## Rationale for this change `functions_string.yaml` defines one function for this, with two arguments and an option: ```yaml name: like impls: - args: - value: "varchar<L1>" name: "input" - value: "varchar<L2>" name: "match" options: case_sensitivity: values: [ CASE_SENSITIVE, CASE_INSENSITIVE, CASE_INSENSITIVE_ASCII ] return: "boolean" ``` The producer emitted three arguments, registered `ilike` for the case insensitive form, and never set the option. Neither `ilike` nor an escape argument appears in any extension file, in the pinned crate or upstream. ## What changes are included in this PR? - `make_substrait_like_expr` registers `like`, emits the two arguments the definition takes, and sets `case_sensitivity` to `CASE_INSENSITIVE` for `ILIKE`. A plain `LIKE` sets no option, which leaves the default. - `LIKE ... ESCAPE` is now `not_impl_err`. There is no escape in the definition, so the third argument only produced a call a consumer cannot bind. - The consumer reads `case_sensitivity`, taking the first value it supports and refusing the ones it does not, as Substrait requires. It still accepts the `ilike` name and the three argument form, so plans written by an older DataFusion still load. ## What is the testing strategy for this PR? - New producer tests: `LIKE` emits `like` with two arguments and no option, `ILIKE` emits the `case_sensitivity` option, and an escape character is rejected. - `roundtrip_like` and `roundtrip_ilike` still pass, which is what covers the consumer: with the option reading removed, `roundtrip_ilike` fails because `ILIKE` comes back as `LIKE`. Removing the producer half likewise fails the new producer test. - `cargo test -p datafusion-substrait` passes and `./ci/scripts/rust_clippy.sh` is clean. Measured against substrait-java 0.103.0 and Spark 3.5.4, over `t(s)` holding `abc` and `ABC`, with the #11545 URN patched and `output_type` filled (#25049): | plan | before | after | | --- | --- | --- | | `s LIKE 'a%'` | `AnalysisException: WRONG_NUM_ARGS ... requires 2 parameters but the actual number is 3` | runs, returns `abc` | | `s ILIKE 'a%'` | `IllegalArgumentException: Unexpected scalar function with key ilike:str_str` | runs, returns `abc` | **The second row is a trade-off worth stating.** `ILIKE` used to fail loudly there, and now it is accepted but answered case sensitively, because substrait-spark does not read the option: `case_sensitivity` appears nowhere in its sources. Substrait says a consumer that does not recognise an option must reject the call, so that looks like a gap on that side, and I am happy to report it there. If you would rather not emit something a known consumer mishandles, the alternative is to reject `ILIKE` as well until a consumer honours the option, and I will change it. ## Are there any user-facing changes? `LIKE` and `ILIKE` are emitted in the form the extension defines, so consumers that bind arguments by the declaration can now read them. `LIKE ... ESCAPE` is rejected instead of being emitted in a shape no consumer can bind; DataFusion round trips it today only because its own consumer reads the same private convention. This touches `make_substrait_like_expr`, which #25367 folds into `from_like`, so whichever lands second needs a small rebase. --------- Co-authored-by: namanjain24-sudo <180642416+namanjain24-sudo@users.noreply.github.com>
1 parent 048fc19 commit f868d60

2 files changed

Lines changed: 222 additions & 22 deletions

File tree

‎datafusion/substrait/src/logical_plan/consumer/expr/scalar_function.rs‎

Lines changed: 110 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -187,6 +187,32 @@ fn arg_list_to_binary_op_tree_inner(
187187
}))
188188
}
189189

190+
/// Reads the `case_sensitivity` option of a `like` call.
191+
///
192+
/// Substrait says a consumer must use the first value it supports, and must
193+
/// reject the call when it supports none of them.
194+
fn case_insensitive_option(f: &ScalarFunction) -> Result<bool> {
195+
let Some(option) = f
196+
.options
197+
.iter()
198+
.find(|option| option.name.eq_ignore_ascii_case("case_sensitivity"))
199+
else {
200+
return Ok(false);
201+
};
202+
for preference in &option.preference {
203+
if preference.eq_ignore_ascii_case("CASE_SENSITIVE") {
204+
return Ok(false);
205+
}
206+
if preference.eq_ignore_ascii_case("CASE_INSENSITIVE") {
207+
return Ok(true);
208+
}
209+
}
210+
not_impl_err!(
211+
"Unsupported case_sensitivity for `like`: {:?}",
212+
option.preference
213+
)
214+
}
215+
190216
/// Build [`Expr`] from its name and required inputs.
191217
struct BuiltinExprBuilder {
192218
expr_name: String,
@@ -213,7 +239,10 @@ impl BuiltinExprBuilder {
213239
args: Vec<Expr>,
214240
) -> Result<Expr> {
215241
match self.expr_name.as_str() {
216-
"like" => Self::build_like_expr(false, false, f, args),
242+
// `like` carries case sensitivity as an option. `ilike` is not a
243+
// Substrait function, but DataFusion used to emit it, so plans
244+
// written by an older version are still read.
245+
"like" => Self::build_like_expr(case_insensitive_option(f)?, false, f, args),
217246
"ilike" => Self::build_like_expr(true, false, f, args),
218247
"like_match" => Self::build_like_expr(false, false, f, args),
219248
"like_imatch" => Self::build_like_expr(true, false, f, args),
@@ -374,7 +403,7 @@ impl BuiltinExprBuilder {
374403

375404
#[cfg(test)]
376405
mod tests {
377-
use super::arg_list_to_binary_op_tree;
406+
use super::{arg_list_to_binary_op_tree, case_insensitive_option};
378407
use crate::extensions::Extensions;
379408
use crate::logical_plan::consumer::tests::TEST_SESSION_STATE;
380409
use crate::logical_plan::consumer::{DefaultSubstraitConsumer, SubstraitConsumer};
@@ -583,4 +612,83 @@ mod tests {
583612

584613
Ok(())
585614
}
615+
616+
fn scalar_function_with_case_sensitivity_preference(
617+
preference: &[&str],
618+
) -> ScalarFunction {
619+
ScalarFunction {
620+
options: vec![substrait::proto::FunctionOption {
621+
name: "case_sensitivity".to_string(),
622+
preference: preference.iter().map(|s| s.to_string()).collect(),
623+
}],
624+
..Default::default()
625+
}
626+
}
627+
628+
/// Substrait says a consumer must use the first value it supports in the
629+
/// `case_sensitivity` option's preference list, and must reject the call
630+
/// when it supports none of them.
631+
#[test]
632+
fn case_insensitive_option_cases() -> Result<()> {
633+
// No `case_sensitivity` option at all defaults to case-sensitive.
634+
assert!(!case_insensitive_option(&ScalarFunction::default())?);
635+
636+
// A single supported preference, in either sensitivity.
637+
assert!(!case_insensitive_option(
638+
&scalar_function_with_case_sensitivity_preference(&["CASE_SENSITIVE"])
639+
)?);
640+
assert!(case_insensitive_option(
641+
&scalar_function_with_case_sensitivity_preference(&["CASE_INSENSITIVE"])
642+
)?);
643+
644+
// The option name and its preference values are matched
645+
// case-insensitively (ASCII).
646+
assert!(case_insensitive_option(&ScalarFunction {
647+
options: vec![substrait::proto::FunctionOption {
648+
name: "Case_Sensitivity".to_string(),
649+
preference: vec!["case_insensitive".to_string()],
650+
}],
651+
..Default::default()
652+
})?);
653+
654+
// Preference ordering: a consumer must use the first value it
655+
// supports, not necessarily the first value in the list.
656+
// `CASE_INSENSITIVE_ASCII` is not a value this consumer supports, so
657+
// it is skipped in favor of the next, supported preference.
658+
assert!(case_insensitive_option(
659+
&scalar_function_with_case_sensitivity_preference(&[
660+
"CASE_INSENSITIVE_ASCII",
661+
"CASE_INSENSITIVE"
662+
])
663+
)?);
664+
assert!(!case_insensitive_option(
665+
&scalar_function_with_case_sensitivity_preference(&[
666+
"CASE_INSENSITIVE_ASCII",
667+
"CASE_SENSITIVE"
668+
])
669+
)?);
670+
671+
// Rejection: every preference is unsupported.
672+
let err =
673+
case_insensitive_option(&scalar_function_with_case_sensitivity_preference(
674+
&["CASE_INSENSITIVE_ASCII"],
675+
))
676+
.unwrap_err();
677+
assert!(
678+
err.to_string().contains("Unsupported case_sensitivity"),
679+
"unexpected error: {err}"
680+
);
681+
682+
// Rejection: an empty preference list supports nothing.
683+
let err = case_insensitive_option(
684+
&scalar_function_with_case_sensitivity_preference(&[]),
685+
)
686+
.unwrap_err();
687+
assert!(
688+
err.to_string().contains("Unsupported case_sensitivity"),
689+
"unexpected error: {err}"
690+
);
691+
692+
Ok(())
693+
}
586694
}

‎datafusion/substrait/src/logical_plan/producer/expr/scalar_function.rs‎

Lines changed: 112 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -16,17 +16,17 @@
1616
// under the License.
1717

1818
use crate::logical_plan::producer::{
19-
SubstraitProducer, to_substrait_literal_expr, to_substrait_type,
20-
to_substrait_type_from_field,
19+
SubstraitProducer, to_substrait_type, to_substrait_type_from_field,
2120
};
2221
use datafusion::arrow::datatypes::DataType;
2322
use datafusion::common::datatype::FieldExt;
2423
use datafusion::common::{
25-
DFSchemaRef, ScalarValue, internal_datafusion_err, not_impl_err, substrait_err,
24+
DFSchemaRef, internal_datafusion_err, not_impl_err, substrait_err,
2625
};
2726
use datafusion::logical_expr::{
2827
Between, BinaryExpr, Expr, ExprSchemable, Like, Operator, expr,
2928
};
29+
use substrait::proto::FunctionOption;
3030
use substrait::proto::expression::{RexType, ScalarFunction};
3131
use substrait::proto::function_argument::ArgType;
3232
use substrait::proto::{Expression, FunctionArgument, Type};
@@ -234,6 +234,11 @@ pub fn from_binary_expr(
234234
))
235235
}
236236

237+
/// The option `like` uses to carry case sensitivity, and the value that asks
238+
/// for the case insensitive form. Defined in `functions_string.yaml`.
239+
pub(crate) const CASE_SENSITIVITY_OPTION: &str = "case_sensitivity";
240+
pub(crate) const CASE_INSENSITIVE: &str = "CASE_INSENSITIVE";
241+
237242
pub fn from_like(
238243
producer: &mut impl SubstraitProducer,
239244
like: &Like,
@@ -246,35 +251,42 @@ pub fn from_like(
246251
escape_char,
247252
case_insensitive,
248253
} = like;
249-
let function_anchor = if *case_insensitive {
250-
producer.register_function("ilike".to_string())
251-
} else {
252-
producer.register_function("like".to_string())
253-
};
254+
// `like` takes two arguments and carries case sensitivity as an option;
255+
// the extensions define no `ilike` and no escape character, so an escape
256+
// has no encoding here and is rejected rather than emitted as a third
257+
// argument that a consumer would bind to a parameter the function does
258+
// not have.
259+
if escape_char.is_some() {
260+
return not_impl_err!("Substrait does not define an escape character for `like`");
261+
}
254262
// Substrait documents `output_type` as "Must be set to the return type of
255263
// the function, exactly as derived using the declaration in the extension",
256264
// and a consumer that reads it rejects the call when it is unset. The type
257265
// comes from the expression itself so that it matches what DataFusion
258266
// derives, rather than being restated here.
259267
let (_, output_field) = Expr::Like(like.clone()).to_field(schema)?;
260268
let output_type = to_substrait_type_from_field(producer, &output_field)?;
261-
let expr = producer.handle_expr(expr, schema)?;
262-
let pattern = producer.handle_expr(pattern, schema)?;
263-
let escape_char = to_substrait_literal_expr(
264-
producer,
265-
&ScalarValue::Utf8(escape_char.map(|c| c.to_string())),
266-
)?;
269+
270+
let function_anchor = producer.register_function("like".to_string());
271+
let substrait_expr = producer.handle_expr(expr, schema)?;
272+
let substrait_pattern = producer.handle_expr(pattern, schema)?;
267273
let arguments = vec![
268274
FunctionArgument {
269-
arg_type: Some(ArgType::Value(expr)),
270-
},
271-
FunctionArgument {
272-
arg_type: Some(ArgType::Value(pattern)),
275+
arg_type: Some(ArgType::Value(substrait_expr)),
273276
},
274277
FunctionArgument {
275-
arg_type: Some(ArgType::Value(escape_char)),
278+
arg_type: Some(ArgType::Value(substrait_pattern)),
276279
},
277280
];
281+
// An unset option leaves the default, which is `CASE_SENSITIVE`.
282+
let options = if *case_insensitive {
283+
vec![FunctionOption {
284+
name: CASE_SENSITIVITY_OPTION.to_string(),
285+
preference: vec![CASE_INSENSITIVE.to_string()],
286+
}]
287+
} else {
288+
vec![]
289+
};
278290

279291
#[expect(deprecated)]
280292
let substrait_like = Expression {
@@ -283,7 +295,7 @@ pub fn from_like(
283295
arguments,
284296
output_type: Some(output_type.clone()),
285297
args: vec![],
286-
options: vec![],
298+
options,
287299
})),
288300
};
289301

@@ -438,6 +450,7 @@ pub fn operator_to_name(op: Operator) -> &'static str {
438450

439451
#[cfg(test)]
440452
mod tests {
453+
use super::{CASE_INSENSITIVE, CASE_SENSITIVITY_OPTION};
441454
use crate::logical_plan::producer::{
442455
DefaultSubstraitProducer, SubstraitProducer, to_substrait_type,
443456
};
@@ -447,9 +460,88 @@ mod tests {
447460
use datafusion::logical_expr::{Expr, Like};
448461
use datafusion::prelude::{col, lit};
449462
use substrait::proto::Expression;
463+
use substrait::proto::FunctionOption;
450464
use substrait::proto::expression::{RexType, ScalarFunction};
451465
use substrait::proto::function_argument::ArgType;
452466

467+
/// `like` takes two arguments and carries case sensitivity as an option,
468+
/// so `ILIKE` is that option rather than a separate function.
469+
#[tokio::test]
470+
async fn like_emits_case_sensitivity_option() -> datafusion::common::Result<()> {
471+
let state = SessionStateBuilder::default().build();
472+
let schema =
473+
DFSchemaRef::new(DFSchema::try_from(Schema::new(vec![Field::new(
474+
"s",
475+
DataType::Utf8,
476+
true,
477+
)]))?);
478+
479+
for (case_insensitive, expected_options) in [
480+
(false, vec![]),
481+
(
482+
true,
483+
vec![FunctionOption {
484+
name: CASE_SENSITIVITY_OPTION.to_string(),
485+
preference: vec![CASE_INSENSITIVE.to_string()],
486+
}],
487+
),
488+
] {
489+
let mut producer = DefaultSubstraitProducer::new(&state);
490+
let like = Like::new(
491+
false,
492+
Box::new(col("s")),
493+
Box::new(lit("a%")),
494+
None,
495+
case_insensitive,
496+
);
497+
let expr = producer.handle_expr(&Expr::Like(like), &schema)?;
498+
499+
let Some(RexType::ScalarFunction(call)) = expr.rex_type else {
500+
panic!("Substrait ScalarFunction expected")
501+
};
502+
assert_eq!(
503+
producer
504+
.get_extensions()
505+
.functions
506+
.get(&call.function_reference),
507+
Some(&"like".to_string()),
508+
"case_insensitive = {case_insensitive}"
509+
);
510+
assert_eq!(call.arguments.len(), 2, "no escape argument is emitted");
511+
assert_eq!(call.options, expected_options);
512+
}
513+
514+
Ok(())
515+
}
516+
517+
/// The extensions define no escape character, so there is nothing to emit.
518+
#[tokio::test]
519+
async fn like_with_escape_is_rejected() -> datafusion::common::Result<()> {
520+
let state = SessionStateBuilder::default().build();
521+
let schema =
522+
DFSchemaRef::new(DFSchema::try_from(Schema::new(vec![Field::new(
523+
"s",
524+
DataType::Utf8,
525+
true,
526+
)]))?);
527+
let mut producer = DefaultSubstraitProducer::new(&state);
528+
529+
let like = Like::new(
530+
false,
531+
Box::new(col("s")),
532+
Box::new(lit("a!%")),
533+
Some('!'),
534+
false,
535+
);
536+
let err = producer
537+
.handle_expr(&Expr::Like(like), &schema)
538+
.expect_err("an escape character must be rejected")
539+
.to_string();
540+
assert!(err.contains("escape character"), "unexpected error: {err}");
541+
542+
Ok(())
543+
}
544+
453545
#[tokio::test]
454546
async fn binary_expr_output_type() -> datafusion::common::Result<()> {
455547
let state = SessionStateBuilder::default().build();

0 commit comments

Comments
 (0)