Repository navigation
fix: Emit LIKE the way the Substrait extension defines it - #25442
namanjain24-sudo wants to merge 2 commits into
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #25442 +/- ##
==========================================
+ Coverage 82.66% 82.72% +0.06%
==========================================
Files 1147 1147
Lines 446357 448218 +1861
Branches 446357 448218 +1861
==========================================
+ Hits 368971 370795 +1824
+ Misses 54997 54906 -91
- Partials 22389 22517 +128 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Bumping this — still open for review whenever someone has bandwidth. |
…5367) ## Which issue does this PR close? - Closes apache#25366. ## Rationale for this change The producer left `output_type` unset on window function calls and on `LIKE` / `ILIKE`, including the `not` that wraps a negated one. Substrait documents both `Expression.WindowFunction.output_type` and `Expression.ScalarFunction.output_type` as: > Must be set to the return type of the function, exactly as derived using the declaration in the extension. A consumer that reads the field rejects such a call. substrait-java 0.103.0 refuses both plans, from `ProtoTypeConverter.from:125`: | plan produced for | error before this PR | | --- | --- | | `SELECT sum(i) OVER (ORDER BY i) FROM t` | `UnsupportedOperationException: Type is not set` at `ProtoExpressionConverter.fromWindowFunction:406` | | `SELECT i FROM t WHERE CAST(i AS VARCHAR) LIKE '1%'` | `UnsupportedOperationException: Type is not set` at `ProtoExpressionConverter:201`, via `ProtoRelConverter.newFilter` | A DataFusion round trip cannot catch this: the consumer reads `output_type` only in `consumer/expr/cast.rs`, so producer and consumer agree on the omission. apache#15831 and apache#20597 set this field for binary and unary expressions, `from_function` and higher order functions, and apache#25049 and apache#25090 cover `AggregateFunction`. These were the remaining expression kinds. ## What changes are included in this PR? Both types come from the expression itself, so they match what DataFusion derives rather than being restated in the producer: - `from_window_function` derives the type from `Expr::WindowFunction(..).to_field(schema)` and writes it on the call it builds. `make_substrait_window_function` had one caller and eight parameters once the type was added, so its body now sits in `from_window_function` and the helper is gone. - `from_like` derives the type from `Expr::Like(..).to_field(schema)` and sets it on the `like` call and on the `not` wrapper of a negated one, which yields that same type. `make_substrait_like_expr` had `from_like` as its only caller, so, as with the window helper, its body now sits in `from_like`. ## What is the testing strategy for this PR? - Two unit tests next to the existing `binary_expr_output_type`: `window_function_output_type` asserts `sum(i)` over a nullable `i64` carries a nullable `i64`, and `like_output_type` asserts a `LIKE` over a nullable input carries a nullable boolean, on the `like` call, on the `not` of a negated one, and on the `like` nested inside that `not`. - Each half was reverted on its own to check the tests pin it: without the window change only `window_function_output_type` fails, without the `LIKE` change only `like_output_type` fails. - `cargo test -p datafusion-substrait` passes (60 unit, 210 integration with the 6 that were already ignored, 3 doc tests), and `./ci/scripts/rust_clippy.sh`, the workspace clippy CI runs, is clean. - Emitted protobuf before and after, read directly rather than through a consumer: | plan | before | after | | --- | --- | --- | | window `sum` | `output_type` unset | set | | `like` | unset | set | | `not` around a negated `like` | unset | set | - With the same plans, substrait-java 0.103.0 no longer raises `Type is not set`. Both now get through `ProtoPlanConverter`, while the plans built from `main` still fail there. Each then stops further along for reasons that have nothing to do with this field: substrait-spark has no visitor for a window function that appears in a project expression, and Spark's `like` takes two arguments while the call we emit passes three, the third being the escape character. That second one is apache#25442, which emits `LIKE` in the two-argument form the extension defines. ## Are there any user-facing changes? No API changes. Plans produced for window functions and `LIKE` now carry the expression's type, which consumers that require it will accept.
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.
e003924 to
b04cc1d
Compare
kosiew
left a comment
There was a problem hiding this comment.
Thanks for working on this. The Substrait LIKE/ILIKE encoding now matches the extension contract. I left two non-blocking suggestions for additional consumer coverage and a small implementation simplification.
| return Ok(false); | ||
| }; | ||
| for preference in &option.preference { | ||
| if preference.eq_ignore_ascii_case("CASE_SENSITIVE") { |
There was a problem hiding this 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.
There was a problem hiding this 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.
| fn make_substrait_like_expr( | ||
| producer: &mut impl SubstraitProducer, | ||
| ignore_case: bool, | ||
| negated: bool, |
There was a problem hiding this 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.
There was a problem hiding this comment.
Done — inlined it back into from_like (single caller), so the too_many_arguments suppression is gone.
…tion 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.
Which issue does this PR close?
Rationale for this change
functions_string.yamldefines one function for this, with two arguments and an option:The producer emitted three arguments, registered
ilikefor the case insensitive form, and never set the option. Neitherilikenor an escape argument appears in any extension file, in the pinned crate or upstream.What changes are included in this PR?
make_substrait_like_exprregisterslike, emits the two arguments the definition takes, and setscase_sensitivitytoCASE_INSENSITIVEforILIKE. A plainLIKEsets no option, which leaves the default.LIKE ... ESCAPEis nownot_impl_err. There is no escape in the definition, so the third argument only produced a call a consumer cannot bind.case_sensitivity, taking the first value it supports and refusing the ones it does not, as Substrait requires. It still accepts theilikename and the three argument form, so plans written by an older DataFusion still load.What is the testing strategy for this PR?
LIKEemitslikewith two arguments and no option,ILIKEemits thecase_sensitivityoption, and an escape character is rejected.roundtrip_likeandroundtrip_ilikestill pass, which is what covers the consumer: with the option reading removed,roundtrip_ilikefails becauseILIKEcomes back asLIKE. Removing the producer half likewise fails the new producer test.cargo test -p datafusion-substraitpasses and./ci/scripts/rust_clippy.shis clean.Measured against substrait-java 0.103.0 and Spark 3.5.4, over
t(s)holdingabcandABC, with the #11545 URN patched andoutput_typefilled (#25049):s LIKE 'a%'AnalysisException: WRONG_NUM_ARGS ... requires 2 parameters but the actual number is 3abcs ILIKE 'a%'IllegalArgumentException: Unexpected scalar function with key ilike:str_strabcThe second row is a trade-off worth stating.
ILIKEused to fail loudly there, and now it is accepted but answered case sensitively, because substrait-spark does not read the option:case_sensitivityappears 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 rejectILIKEas well until a consumer honours the option, and I will change it.Are there any user-facing changes?
LIKEandILIKEare emitted in the form the extension defines, so consumers that bind arguments by the declaration can now read them.LIKE ... ESCAPEis 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 intofrom_like, so whichever lands second needs a small rebase.