You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "namanjain24-sudo (via GitHub)" <gi...@apache.org> on 2026/09/17 22:52:14 UTC
[PR] fix: Emit LIKE the way the Substrait extension defines it [datafusion]
namanjain24-sudo opened a new pull request, #25442:
URL: https://github.com/apache/datafusion/pull/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 also edits, so whichever lands second needs a small rebase.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: Emit LIKE the way the Substrait extension defines it [datafusion]
Posted by "namanjain24-sudo (via GitHub)" <gi...@apache.org>.
namanjain24-sudo commented on PR #25442:
URL: https://github.com/apache/datafusion/pull/25442#issuecomment-5866396979
Bumping this — still open for review whenever someone has bandwidth.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: Emit LIKE the way the Substrait extension defines it [datafusion]
Posted by "kosiew (via GitHub)" <gi...@apache.org>.
kosiew commented on PR #25442:
URL: https://github.com/apache/datafusion/pull/25442#issuecomment-6073159580
🚀
@namanjain24-sudo
Thank you for your contribution.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: Emit LIKE the way the Substrait extension defines it [datafusion]
Posted by "namanjain24-sudo (via GitHub)" <gi...@apache.org>.
namanjain24-sudo commented on code in PR #25442:
URL: https://github.com/apache/datafusion/pull/25442#discussion_r4195910887
##########
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<bool> {
+ 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") {
Review Comment:
Added case_insensitive_option_cases: covers preference ordering (including CASE_INSENSITIVE_ASCII being skipped since it's unsupported), mixed-case option/preference names, an empty preference list, and full rejection.
##########
datafusion/substrait/src/logical_plan/producer/expr/scalar_function.rs:
##########
@@ -246,35 +251,65 @@ 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
// 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,
Review Comment:
Done — inlined it back into from_like (single caller), so the too_many_arguments suppression is gone.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: Emit LIKE the way the Substrait extension defines it [datafusion]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #25442:
URL: https://github.com/apache/datafusion/pull/25442#issuecomment-5722460098
## [Codecov](https://app.codecov.io/gh/apache/datafusion/pull/25442?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `90.90909%` with `8 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 82.33%. Comparing base ([`3ab72e6`](https://app.codecov.io/gh/apache/datafusion/commit/3ab72e61e38259f8c52e1937a9c90e7799d7b221?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`e003924`](https://app.codecov.io/gh/apache/datafusion/commit/e0039248732f3d85d5b99c264c468839bfe59f35?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 2 commits behind head on main.
| [Files with missing lines](https://app.codecov.io/gh/apache/datafusion/pull/25442?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [.../src/logical\_plan/consumer/expr/scalar\_function.rs](https://app.codecov.io/gh/apache/datafusion/pull/25442?src=pr&el=tree&filepath=datafusion%2Fsubstrait%2Fsrc%2Flogical_plan%2Fconsumer%2Fexpr%2Fscalar_function.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9zdWJzdHJhaXQvc3JjL2xvZ2ljYWxfcGxhbi9jb25zdW1lci9leHByL3NjYWxhcl9mdW5jdGlvbi5ycw==) | 75.00% | [3 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/25442?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [.../src/logical\_plan/producer/expr/scalar\_function.rs](https://app.codecov.io/gh/apache/datafusion/pull/25442?src=pr&el=tree&filepath=datafusion%2Fsubstrait%2Fsrc%2Flogical_plan%2Fproducer%2Fexpr%2Fscalar_function.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9zdWJzdHJhaXQvc3JjL2xvZ2ljYWxfcGxhbi9wcm9kdWNlci9leHByL3NjYWxhcl9mdW5jdGlvbi5ycw==) | 94.44% | [2 Missing and 2 partials :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/25442?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #25442 +/- ##
==========================================
- Coverage 82.34% 82.33% -0.01%
==========================================
Files 1137 1137
Lines 432364 432574 +210
Branches 432364 432574 +210
==========================================
+ Hits 356020 356178 +158
- Misses 54830 54853 +23
- Partials 21514 21543 +29
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/datafusion/pull/25442?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: Emit LIKE the way the Substrait extension defines it [datafusion]
Posted by "kosiew (via GitHub)" <gi...@apache.org>.
kosiew commented on code in PR #25442:
URL: https://github.com/apache/datafusion/pull/25442#discussion_r4194298004
##########
datafusion/substrait/src/logical_plan/producer/expr/scalar_function.rs:
##########
@@ -246,35 +251,65 @@ 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
// 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,
Review Comment:
One optional simplification would be to keep this serialization in `from_like`, or pass `&Like` to the helper instead of unpacking it into eight parameters. Since the helper has one caller, that would avoid the `too_many_arguments` suppression without changing behavior.
##########
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<bool> {
+ 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") {
Review Comment:
Could we add table-driven tests for preference ordering and rejection cases, including `CASE_INSENSITIVE_ASCII`, mixed-case values, and empty preference lists? The code handles these cases, but the current roundtrip tests only cover the single preference emitted by DataFusion, so this could also be a follow-up.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: Emit LIKE the way the Substrait extension defines it [datafusion]
Posted by "kosiew (via GitHub)" <gi...@apache.org>.
kosiew merged PR #25442:
URL: https://github.com/apache/datafusion/pull/25442
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org