Skip to content

snowflake: Fix handling of /~% in the stage name - #1009

Merged
alamb merged 2 commits into
apache:mainfrom
getsynq:snowflake-division
Oct 26, 2023
Merged

snowflake: Fix handling of /~% in the stage name#1009
alamb merged 2 commits into
apache:mainfrom
getsynq:snowflake-division

Conversation

@lustefaniak

@lustefaniaklustefaniak commented Oct 19, 2023•
edited
Loading

Copy link
Copy Markdown
Contributor

Fixes: #1008

Changed as discussed in the code review. Tokenizer accepts /, %, ~ normally, and then when parsing stage name, that is accepted as part of identifier.

@lustefaniaklustefaniak changed the title snowflake: Fix handling of @~% in the stage namesnowflake: Fix handling of /~% in the stage nameOct 19, 2023
@coveralls

coveralls commented Oct 19, 2023•
edited
Loading

Copy link
Copy Markdown

Pull Request Test Coverage Report for Build 6652040155

  • 34 of 35(97.14%) changed or added relevant lines in 3 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage increased (+0.008%) to 87.655%
Changes Missing CoverageCovered LinesChanged/Added Lines%
src/dialect/snowflake.rs232495.83%
Files with Coverage ReductionNew Missed Lines%
src/tokenizer.rs188.67%
TotalsCoverage Status
Change from base Build 6643642320:0.008%
Covered Lines:17545
Relevant Lines:20016
💛 - Coveralls

alamb
alamb reviewed Oct 24, 2023
View reviewed changes

@alambalamb left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks @lustefaniak -- I have a question about why the tests were changed

Perhaps @pawel-big-lebowski can help review this as the author of #841

Comment threadtests/sqlparser_snowflake.rs Outdated
@yuval-illumex

yuval-illumex commented Oct 24, 2023

Copy link
Copy Markdown
Contributor

Just wanted to say thanks to @lustefaniak - I also noticed this issue and wanted to tackle it.

pawel-big-lebowski
pawel-big-lebowski reviewed Oct 25, 2023
View reviewed changes
Comment threadsrc/tokenizer.rs Outdated
pawel-big-lebowski
pawel-big-lebowski approved these changes Oct 26, 2023
View reviewed changes

@pawel-big-lebowskipawel-big-lebowski left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for fixing this.
Great job.

@lustefaniaklustefaniak mentioned this pull request Oct 26, 2023
Merged
alamb
alamb approved these changes Oct 26, 2023
View reviewed changes

@alambalamb left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thank you @lustefaniak for both this PR as well as #1025 -- they look great.

@alamb
alamb merged commit 7b3cc18 into apache:mainOct 26, 2023
@lustefaniak
lustefaniak deleted the snowflake-division branch October 26, 2023 23:31
serprex pushed a commit to serprex/sqlparser-rs that referenced this pull request Nov 6, 2023
@lustefaniak@serprex
snowflake: Fix handling of/~%in the stage name (apache#1009)
b256730
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alambalambalamb approved these changes
+2 more reviewers
@yuval-illumexyuval-illumexyuval-illumex left review comments
@pawel-big-lebowskipawel-big-lebowskipawel-big-lebowski approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

In Snowflake dialect field/100 tokenizes as Word(field/100) instead of Word(field),Div,Number(100)

5 participants

@lustefaniak@coveralls@yuval-illumex@alamb@pawel-big-lebowski