[SPARK-44044][SS] Improve Error message for Window functions with streaming #41578
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
There was a problem hiding this comment.
nit: Streaming (upper case)
Contributor
There was a problem hiding this comment.
Interesting...should the error message include OVER spec? 🤔 It might not necessarily be related to this change but an issue with window aggregation in Spark SQL in general.
Contributor
Author
There was a problem hiding this comment.
If we add OVER, it looks like this:
Unsupported window function found in column 'row_number() OVER (PARTITION BY col1 ORDER BY col2 ASC NULLS FIRST ROWS BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW) as column rn_col'. Structured streaming only supports time-window aggregation using the `window` function.
I don't have a strong opinion on this but I didn't include it because I personally feel that I'm a little bit overwhelmed by less relevant information. What's your opinion on this?
Contributor
Author
There was a problem hiding this comment.
Updated the PR. Now the message looks like following:
Unsupported window function in 'row_number()' as column 'rn_col'. Structured Streaming only supports time-window aggregation using the `window` function. (window specification: '(PARTITION BY col1 ORDER BY col2 ASC NULLS FIRST ROWS BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW)')
MaxGekk
reviewed
Jun 19, 2023
Comment on lines
525
to
527
Member
There was a problem hiding this comment.
Could you introduce an error class with this error format, and put it to error-classes.json, please. Also cc @HeartSaVioR for checking semantic.
Contributor
There was a problem hiding this comment.
@siying
The change itself seems to be OK.
We are going to apply error class framework on new attempt of touching exception. Although we haven't made any progress on UnsupportedOperationChecker, we could consider this as a starter.
Please look into the recent PR on error class migration in SS, especially error-classes.json and QueryExecutionErrors.scala.
#41205
Here is a guide doc for error class framework.
https://github.com/apache/spark/blob/master/core/src/main/resources/error/README.md
Please go through and apply it. Thanks!
HeartSaVioR
reviewed
Jun 19, 2023
Contributor
There was a problem hiding this comment.
case w @ Window(_, _, _, child) if child.isStreaming =>
w would be same as subPlan.asInstanceOf[Window]
Contributor
There was a problem hiding this comment.
Looks like it doesn't even need to... we can just extract the value out from the pattern.
case Window(windowExpressions, _, _, child) if child.isStreaming =>
and then remove L512 and use windowExpressions directly.
Comment on lines
525
to
527
Contributor
There was a problem hiding this comment.
@siying
The change itself seems to be OK.
We are going to apply error class framework on new attempt of touching exception. Although we haven't made any progress on UnsupportedOperationChecker, we could consider this as a starter.
Please look into the recent PR on error class migration in SS, especially error-classes.json and QueryExecutionErrors.scala.
#41205
Here is a guide doc for error class framework.
https://github.com/apache/spark/blob/master/core/src/main/resources/error/README.md
Please go through and apply it. Thanks!
MaxGekk
requested changes
Jun 21, 2023
Member
There was a problem hiding this comment.
Could you use checkError to check the error class and parameters. In that way, tech editors can change the error message and don't depend on internal Spark tests.
Member
There was a problem hiding this comment.
Is it possible to avoid the dependency from the error message format?
Member
There was a problem hiding this comment.
Can the argument contain multiple names? If not, please, change it to windowFunc
Member
There was a problem hiding this comment.
Could you wrap windowFuncs by toSQLId, please.
Member
There was a problem hiding this comment.
Please, remove '' around windowSpec
Contributor
Author
|
@MaxGekk I addressed the comments. The CI failure is on python link. I don't know why it happens but I don't see how it is related. |
Contributor
Author
|
@MaxGekk here are the only error messages in CI and I assume it is not related: |
Contributor
|
@MaxGekk Would you mind having an another round of review? Given that you've been providing review comments, I'd like to make sure we get sign-off from you. Thanks in advance! |
MaxGekk
reviewed
Jun 27, 2023
Member
There was a problem hiding this comment.
Please, restore indentation here.
Member
Yep, it seems not. I re-ran it locally. Could you rebase on the recent master. |
Contributor
Author
|
@MaxGekk I removed the unintended indenting and rebase. now all tests passed. |
MaxGekk
requested changes
Jun 28, 2023
Member
There was a problem hiding this comment.
Could you change quoting of the expression and the id using toSQLExpr and toSQLId
Member
There was a problem hiding this comment.
and just pass both to nonTimeWindowNotSupportedInStreamingError, and put as column to error-classes.json.
sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryExecutionErrors.scala
Outdated
Show resolved
Hide resolved
Contributor
Author
|
@MaxGekk I addressed the comments. |
MaxGekk
reviewed
Jun 29, 2023
Member
|
@siying Could you resolve conflicts. |
MaxGekk
requested changes
Jun 30, 2023
Comment on lines
27
to
29
Member
There was a problem hiding this comment.
Could you pass sequences of (windowFunc, columnName, windowSpec), and do quoting inside of QueryExecutionErrors, please.
Comment on lines
515
to
529
Member
There was a problem hiding this comment.
Let's merge all of them and return (we.windowFunction, e.toAttribute, we.windowSpec)
Comment on lines
735
to
737
Member
There was a problem hiding this comment.
Better to perform quoting here.
Co-authored-by: Maxim Gekk <[email protected]>
…xecutionErrors.scala Co-authored-by: Maxim Gekk <[email protected]>
Co-authored-by: Maxim Gekk <[email protected]>
Contributor
Author
|
@MaxGekk I address the comments. |
MaxGekk
approved these changes
Jul 1, 2023
Member
|
+1, LGTM. Merging to master. |
Contributor
Author
|
@MaxGekk thank you for your review! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
Replace existing error message when non-time window function is used with streaming to include aggregation function and column. The error message looks like following now:
org.apache.spark.sql.AnalysisException: Window function is not supported in 'row_number()' as column 'rn_col' on streaming DataFrames/Datasets. Structured Streaming only supports time-window aggregation using the
windowunction. (window specification: '(PARTITION BY col1 ORDER BY col2 ASC NULLS FIRST ROWS BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW)')Note that the message is a little bit unnatural as the existing unit test requires the exception follows the pattern that it includes "not supported", "streaming" "DataFrames" and "Dataset".
Why are the changes needed?
The exiting error message is vague and a full logical plan is included. A user reports that they aren't able to identify what the problem is.
Does this PR introduce any user-facing change?
No.
How was this patch tested?
Added a unit test