namanjain24-sudo opened a new pull request, #25367: URL: https://github.com/apache/datafusion/pull/25367
## Which issue does this PR close? - Closes #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. #15831 and #20597 set this field for binary and unary expressions, `from_function` and higher order functions, and #25049 and #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. - `make_substrait_like_expr` takes the `Like` itself rather than its five fields, 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. ## 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. I am looking at that second one separately. ## 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. -- 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]
