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

TSQL: Remove dependency on ANSI keyword lists #2170

Merged
merged 14 commits into from
Dec 26, 2021

Conversation

jpers36
Copy link
Contributor

@jpers36 jpers36 commented Dec 22, 2021

Up to this point, the TSQL dialect has incorporated the ANSI dialect's keyword list into its own keyword list. The upside of this has been the ability to rely on ANSI's definitions even when those definitions include references to keywords not existent in TSQL. The downside is that some non-keywords may flag as keywords when parsing TSQL.

This PR aims to rip off the bandage. This is driven by an issue where I have a stored procedure that can't parse because the parser incorrectly flags ROWS as a reserved keyword. So this PR clears the dialect keyword list before applying TSQL's list, and incorporates all changes needed to make that work.

TSQL keyword changes:

  • Added a number of unreserved keywords that are actually used by TSQL but were previously not in the list

TSQL dialect changes:

  • Clear ANSI keywords from list
  • Add BaseExpressionElementGrammar to override ANSI and remove IntervalExpressionSegment reference
  • Add IsClauseGrammar to override ANSI and remove NAN reference
  • Add LikeGrammar to override ANSI and remove RLIKE/ILIKE
  • Add FunctionContentsGrammar to override ANSI and remove SEPARATOR
  • Remove IntervalExpressionSegment reference from Expression_D_Grammar
  • remove CreateExtensionStatementSegment, CreateModelStatementSegment, DropModelStatementSegment, and DescribeStatementSegment from StatementSegment
  • Add CreateSequenceOptionsSegment to properly represent TSQL functionality
  • Adjust ColumnConstraintSegment to remove invalid AUTO_INCREMENT and UNSIGNED options, and remove reference to invalid CommentClauseSegment
  • Remove CreateExtensionStatementSegment, CreateModelStatementSement, DropModelStatementSegment, OverlapsClauseSegment Nothing() stubs, replacing them with removals elsewhere
  • Add MLTableExpressionSegment Nothing() stub to remove ML keyword; add TO DO to possibly represent PREDICT statment here
  • Rename PartitionByClause to PartitionClauseSegment to match ANSI dialect naming convention; fix type name to match as well
  • Add AlterTableSegment to override ANSI and remove MODIFY
  • Add SetOperatorSegment to override ANSI and remove MINUS
  • Add WindowSpecificationSegment to override ANSI and remove the unsupported functionality to name a window; this needs to be done because PARTITION's status change from a reserved keyword to an unreserved keyword otherwise confuses this definition

Test cases:

  • Added TSQL non-keywords to select test case
  • Added more detailed case to sequence test case
  • Added ALTER table test cases
  • Minor adjustments due to the PartitionByClause/PartitionClauseSegment change

@codecov
Copy link

codecov bot commented Dec 22, 2021

Codecov Report

Merging #2170 (da96bcf) into main (f20bfae) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##              main     #2170   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          148       148           
  Lines        10796     10798    +2     
=========================================
+ Hits         10796     10798    +2     
Impacted Files Coverage Δ
src/sqlfluff/dialects/dialect_tsql_keywords.py 100.00% <ø> (ø)
src/sqlfluff/dialects/dialect_tsql.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 f20bfae...da96bcf. Read the comment docs.

@jpers36 jpers36 marked this pull request as ready for review December 23, 2021 16:28
@jpers36 jpers36 changed the title Tsql remove ansi keywords TSQL: Remove dependency on ANSI keyword lists Dec 23, 2021
@jpy-git jpy-git self-assigned this Dec 24, 2021
Copy link
Contributor

@jpy-git jpy-git left a comment

Choose a reason for hiding this comment

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

@jpers36 looks great, couple points to clear up 👍

Comment on lines 1665 to 1673
@tsql_dialect.segment(replace=True)
class DescribeStatementSegment(BaseSegment):
"""A `Describe` statement.

Not present in T-SQL.
"""

type = "describe_statement"
match_grammar = Nothing()
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than have a stub I think you can just remove this from the StatementSegment like this:
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jpy-git thanks for the syntax help! I moved a couple more stubs to this design now that I know how.

src/sqlfluff/dialects/dialect_tsql.py Outdated Show resolved Hide resolved
src/sqlfluff/dialects/dialect_tsql.py Outdated Show resolved Hide resolved
src/sqlfluff/dialects/dialect_tsql.py Outdated Show resolved Hide resolved
src/sqlfluff/dialects/dialect_tsql.py Show resolved Hide resolved
@jpers36 jpers36 requested a review from jpy-git December 26, 2021 16:33
Copy link
Contributor

@jpy-git jpy-git left a comment

Choose a reason for hiding this comment

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

@jpers36 really great stuff, thanks for driving the T-SQL syntax coverage forwards 💪 LTGM 🚀 ⭐

@jpy-git jpy-git merged commit a92e1c2 into sqlfluff:main Dec 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants