Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make Snowflake keywords unreserved: account, organization, pivot #2172

Merged
merged 7 commits into from Dec 26, 2021
Merged

Make Snowflake keywords unreserved: account, organization, pivot #2172

merged 7 commits into from Dec 26, 2021

Conversation

jpy-git
Copy link
Contributor

@jpy-git jpy-git commented Dec 23, 2021

Brief summary of the change made

Fixes #2165 and fixes #2164. A couple snowflake keywords were being conservatively included in reserved when they shouldn't be. Moving them to non_reserved accordingly.

Are there any other side effects of this change that we should be aware of?

No

Pull Request checklist

  • Please confirm you have completed any of the necessary steps below.

  • Included test cases to demonstrate any code changes, which may be one or more of the following:

    • .yml rule test cases in test/fixtures/rules/std_rule_cases.
    • .sql/.yml parser test cases in test/fixtures/dialects (note YML files can be auto generated with tox -e generate-fixture-yml).
    • Full autofix test cases in test/fixtures/linter/autofix.
    • Other.
  • Added appropriate documentation for the change.

  • Created GitHub issues for any relevant followup/future enhancements if appropriate.

@jpy-git
Copy link
Contributor Author

jpy-git commented Dec 23, 2021

After moving pivot to unreserved_keywords this query now won't parse:

SELECT * FROM my_tbl
PIVOT (min(f_val) FOR f_id IN (1, 2)) AS f (a, b);

Reason being the parser sees the PIVOT as an alias i.e.

SELECT * FROM my_tbl PIVOT
-- Rest is not unparsable
(min(f_val) FOR f_id IN (1, 2)) AS f (a, b);

Any ideas to get around this?

@BorePlusPlus
Copy link

Is there a way to differentiate between PIVOT and PIVOT(? The latter is a function invocation.

@jpy-git
Copy link
Contributor Author

jpy-git commented Dec 23, 2021

@BorePlusPlus yeah it's a bit of a weird one as when this gets parsed we have tokens like: 'SELECT', '*', 'FROM', 'my_tbl', 'PIVOT', '(', 'min', '(', ... and then the parser works forwards through the tokens to find the first grammar match at the current point to build the AST.

Since a token like FROM is a reserved keyword we know it can't be an alias to the preceding select clause element (in this case the *) and must be the start of a FROM clause, which then allows us to parse the rest of that clause (otherwise we might think it's something like SELECT * AS FROM <some unparsable stuff>.

However, now we've made PIVOT non-reserved how do we know the difference between:

  1. SELECT * FROM my_tbl PIVOT <some_unparseable_stuff> (because PIVOT is a table alias alias)
  2. SELECT * FROM my_tbl PIVOT

I think there is might be a way to terminate the FROM match grammar on the pivot clause so I'll double check that now 😄

I don't currently have access to Snowflake atm, but if you do I'm curious if this is fine in snowflake:

SELECT * FROM my_tbl PIVOT

i.e. can we use pivot as a table alias if the pivot isn't being used as a function?

@codecov
Copy link

codecov bot commented Dec 23, 2021

Codecov Report

Merging #2172 (81127be) into main (2823301) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##              main     #2172   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          148       148           
  Lines        10777     10777           
=========================================
  Hits         10777     10777           
Impacted Files Coverage Δ
src/sqlfluff/dialects/dialect_ansi.py 100.00% <ø> (ø)
...rc/sqlfluff/dialects/dialect_snowflake_keywords.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2823301...81127be. Read the comment docs.

@@ -621,6 +621,7 @@ class ObjectReferenceSegment(BaseSegment):
Ref("BinaryOperatorGrammar"),
Ref("ColonSegment"),
Ref("DelimiterSegment"),
Ref("JoinLikeClauseGrammar"),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@BorePlusPlus btw this is what I needed to add to stop the parser getting confused about whether PIVOT is an alias or a function.
Basically for an ObjectReferenceSegment, which is inherited by TableReferenceSegment, it will stop matching when the subsequent token is part of JoinLikeClauseGrammar, which contains the FromPivotExpressionSegment in Snowflake.

@jpy-git jpy-git merged commit a0e07f5 into sqlfluff:main Dec 26, 2021
@jpy-git jpy-git deleted the move_snowflake_keywords branch December 26, 2021 13:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants