Skip to content

Improve Snowflake syntax support#5770

Merged
alanmcruickshank merged 10 commits intosqlfluff:mainfrom
kawashiro:snowflake-for-in-loop
Apr 14, 2024
Merged

Improve Snowflake syntax support#5770
alanmcruickshank merged 10 commits intosqlfluff:mainfrom
kawashiro:snowflake-for-in-loop

Conversation

@kawashiro
Copy link
Copy Markdown
Contributor

@kawashiro kawashiro commented Apr 13, 2024 •
edited
Loading

Brief summary of the change made

  • Extend Snowflake syntax with FOR-IN loop support
  • Extend snowflake syntax with bind variables support in IDENTIFIER() statement

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

Not expected

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.

@kawashiro
Improve Snowflake syntax support
0786d7e 
* Extend Snowflake syntax with FOR-IN loop support
* Extend snowflake syntax with bind variables support in IDENTIFIRE() statement
@kawashiro
Fix segment type name
dd4f839
WittierDinosaur
WittierDinosaur reviewed Apr 13, 2024
View reviewed changes
Comment thread src/sqlfluff/dialects/dialect_snowflake.py Outdated

type = "for_in_statement"

match_grammar = OneOf(
Copy link
Copy Markdown
Contributor

@WittierDinosaur WittierDinosaur Apr 13, 2024

Choose a reason for hiding this comment

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

Should this not be a Sequence at the top level?

Copy link
Copy Markdown
Contributor Author

@kawashiro kawashiro Apr 13, 2024

Choose a reason for hiding this comment

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

It looks like there is a mess with terminators in Delimited() statements and FOR statement itself. Unfortunately, could not find more pretty workaround of that.

Copy link
Copy Markdown
Member

@alanmcruickshank alanmcruickshank Apr 13, 2024

Choose a reason for hiding this comment

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

I think it should be possible, but I'll have a go and see if I can make it work that way.

Copy link
Copy Markdown
Member

@alanmcruickshank alanmcruickshank Apr 13, 2024

Choose a reason for hiding this comment

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

Yep - there's something strange going on here. I think it's actually a problem with ScriptingBlockStatementSegment. I'm going to have a go at fixing it.

Copy link
Copy Markdown
Member

@alanmcruickshank alanmcruickshank Apr 14, 2024

Choose a reason for hiding this comment

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

Yeah I think there was a bug in the terminator support. I think I've found an fixed it - it also threw up that there's a problem with one of our existing test cases, which I've also solved. @WittierDinosaur I'd love a review given I've added quite a lot of code to this PR.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 13, 2024 •
edited
Loading

Coverage Results ✅

Name    Stmts   Miss  Cover   Missing
-------------------------------------
TOTAL   17545      0   100%

225 files skipped due to complete coverage.

@coveralls
Copy link
Copy Markdown

coveralls commented Apr 13, 2024 •
edited
Loading

Coverage Status

coverage: 99.985%. remained the same
when pulling 01fdfb8 on kawashiro:snowflake-for-in-loop
into 26630ce on sqlfluff:main.

WittierDinosaur
WittierDinosaur reviewed Apr 14, 2024
View reviewed changes
Comment thread test/fixtures/dialects/snowflake/begin_end.sql Outdated
@alanmcruickshank alanmcruickshank added this pull request to the merge queue Apr 14, 2024
Merged via the queue into sqlfluff:main with commit f2b0c89 Apr 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@WittierDinosaur WittierDinosaur WittierDinosaur left review comments

@alanmcruickshank alanmcruickshank alanmcruickshank approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@kawashiro @coveralls @alanmcruickshank @WittierDinosaur