[SPARK-41092][SQL] Do not use identifier to match interval units #38583
Conversation
This file contains 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
cloud-fan
changed the title
MaxGekk
reviewed
Nov 10, 2022
| @@ -407,6 +407,7 @@ Below is a list of all the keywords in Spark SQL. | |||
| |DATEADD|non-reserved|non-reserved|non-reserved| | |||
| |DATEDIFF|non-reserved|non-reserved|non-reserved| | |||
| |DAY|non-reserved|non-reserved|non-reserved| | |||
| |DAYS|non-reserved|non-reserved|non-reserved| | |||
There was a problem hiding this comment.
As far as I remember we don't support such plural units in other functions of interval parsing. How about to not mix this new feature (new units) with making the parser stricter?
There was a problem hiding this comment.
it's not a new feature. InternalUtils.stringToInterval allows ending s in unit names.
viirya
reviewed
Nov 10, 2022
This is an existing test and it fails in #38404 . After the changes here, it can pass. |
viirya
approved these changes
Nov 10, 2022
MaxGekk
reviewed
Nov 10, 2022
| "messageParameters" : { | ||
| "msg" : "Error parsing ' 1 fake_unit' to interval, invalid unit 'fake_unit'" | ||
| }, | ||
| "queryContext" : [ { |
There was a problem hiding this comment.
It is not good that we lost the context here but this is another issue.
MaxGekk
approved these changes
Nov 10, 2022
|
+1, LGTM. Merging to master. |
SandishKumarHN
pushed a commit
to SandishKumarHN/spark
that referenced
this pull request
Dec 12, 2022
### What changes were proposed in this pull request? The current antlr-based SQL parser is pretty fragile due to the fact that we make the antlr parser rule pretty flexible and push more parsing logic to the Scala side (`AstBuilder`). A tiny change to the antlr parser rule may break the parser unexpectedly. As an example, in apache#38404 , we added a new parser rule to extend the INSERT syntax, and it breaks interval literal. `select b + interval '1 month' from values (1, 1)` can't be parsed after apache#38404 This PR makes the interval literal parser rule stricter. Now it lists all the allowed interval units instead of matching an identifier. This fixes the issue we hit in apache#38404 . In the future, we can revisit other parser rules and try to rely on antlr more to do the parsing work. ### Why are the changes needed? fix parser issues we hit in apache#38404 ### Does this PR introduce _any_ user-facing change? The error message is changed a little bit for `SELECT INTERVAL 1 wrong_unit`. ### How was this patch tested? existing tests Closes apache#38583 from cloud-fan/parser. Authored-by: Wenchen Fan <wenchen@databricks.com> Signed-off-by: Max Gekk <max.gekk@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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?
The current antlr-based SQL parser is pretty fragile due to the fact that we make the antlr parser rule pretty flexible and push more parsing logic to the Scala side (
AstBuilder). A tiny change to the antlr parser rule may break the parser unexpectedly. As an example, in #38404 , we added a new parser rule to extend the INSERT syntax, and it breaks interval literal.select b + interval '1 month' from values (1, 1)can't be parsed after #38404This PR makes the interval literal parser rule stricter. Now it lists all the allowed interval units instead of matching an identifier. This fixes the issue we hit in #38404 .
In the future, we can revisit other parser rules and try to rely on antlr more to do the parsing work.
Why are the changes needed?
fix parser issues we hit in #38404
Does this PR introduce any user-facing change?
The error message is changed a little bit for
SELECT INTERVAL 1 wrong_unit.How was this patch tested?
existing tests