Skip to content

fix: Set Substrait output_type on window functions and LIKE - #25367

Open
namanjain24-sudo wants to merge 1 commit into
apache:mainfrom
namanjain24-sudo:fix-substrait-window-like-output-type
Open

namanjain24-sudo wants to merge 1 commit into
apache:mainfrom
namanjain24-sudo:fix-substrait-window-like-output-type

Conversation

@namanjain24-sudo

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

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.

Substrait documents output_type on both ScalarFunction and WindowFunction
as the return type of the function, and a consumer that reads it rejects
a call that leaves it unset: substrait-java refuses a window function
plan and a LIKE plan with "Type is not set".

Both types are derived from the expression itself, so they match what
DataFusion derives: from_window_function from Expr::WindowFunction, and
make_substrait_like_expr from Expr::Like, which also covers the not()
wrapper a negated LIKE emits.

make_substrait_window_function had a single caller and its body now sits
in from_window_function, and make_substrait_like_expr takes the Like
rather than its five fields, so neither grows an extra parameter.

A DataFusion round trip cannot catch this, because the consumer reads
output_type only when it converts a cast.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

substrait Changes to the substrait crate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Substrait producer omits output_type on window functions and LIKE expressions

1 participant