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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
