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

Initializes non-reserved PartiQL keywords #780

Closed
wants to merge 2 commits into from

Conversation

johnedquinn
Copy link
Member

@johnedquinn johnedquinn commented Sep 14, 2022

Relevant Issues

Description

  • Adds non-reserved keywords to grammar files, allowing the use of several keywords as identifiers.
    • We do this by creating a nonreservedKeywords rule in ANTLR that is a fallback for symbolPrimitive
  • This realistically does not make many changes -- since we treat the majority of nonreservedKeywords as non-reserved anyways. We initially did this by allowing identifiers in their place, then, in the PartiQLVisitor, we would check that they conform to whatever keyword we expected.
    • For example, the TRIM function allowed an optional IDENTIFIER as the first argument -- then, in the PartiQLVisitor, we would check that the identifier was either leading, trailing, or both. Now, we have formalized a way to still use leading, trailing, or both as identifiers while still enforcing that the first optional parameter must be one of the 3.
    • Similarly, take pattern restrictors for example. In GPML, the pattern restrictors (trail, simple, and acyclic) were parsed as IDENTIFIERS, but upon visiting, we would check that the identifiers are one of the allowed 3. It's more ideal that we have a formal rule -- that way, writing grammars is easier, and we don't need to write checks in the visitor.
    • Now, we do add PUBLIC, DOMAIN, and USER as non-reserved keywords -- since we don't use them at all in our grammar.
  • We also initialized a Keywords Wiki document that specifies all of our keywords and distinguishes between reserved and non-reserved. See the doc for more information.

Other Information

  • Updated Unreleased Section in CHANGELOG: NO
    • Not yet.
  • Any backward-incompatible changes? NO
  • Any new external dependencies? NO

License Information

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@johnedquinn johnedquinn added the enhancement New feature or request label Sep 14, 2022
@codecov-commenter
Copy link

codecov-commenter commented Sep 14, 2022

Codecov Report

Base: 81.34% // Head: 81.33% // Decreases project coverage by -0.01% ⚠️

Coverage data is based on head (7f05b89) compared to base (1dff516).
Patch coverage: 92.30% of modified lines in pull request are covered.

Additional details and impacted files
@@             Coverage Diff              @@
##               main     #780      +/-   ##
============================================
- Coverage     81.34%   81.33%   -0.02%     
- Complexity     2613     2614       +1     
============================================
  Files           257      257              
  Lines         21260    21249      -11     
  Branches       3841     3836       -5     
============================================
- Hits          17295    17283      -12     
- Misses         2761     2765       +4     
+ Partials       1204     1201       -3     
Flag Coverage Δ
CLI 24.08% <ø> (ø)
EXAMPLES 77.19% <ø> (ø)
EXTENSIONS 66.66% <ø> (ø)
LANG 83.24% <92.30%> (-0.02%) ⬇️
PTS ∅ <ø> (∅)
TEST_SCRIPT 80.90% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...ng/src/org/partiql/lang/visitors/PartiQLVisitor.kt 90.89% <92.30%> (-0.20%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@johnedquinn johnedquinn marked this pull request as ready for review September 14, 2022 19:22
@@ -0,0 +1,325 @@
# Overview
Copy link
Member

Choose a reason for hiding this comment

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

This is a really good start to a proper keyword specification table; however, I think it belongs in partiql-docs. I've created an issue in that repository partiql/partiql-docs#28

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed. I can move it.

@@ -539,7 +553,7 @@ extract
: EXTRACT PAREN_LEFT IDENTIFIER FROM rhs=expr PAREN_RIGHT;

trimFunction
: func=TRIM PAREN_LEFT ( mod=IDENTIFIER? sub=expr? FROM )? target=expr PAREN_RIGHT;
: func=TRIM PAREN_LEFT ( mod=(BOTH|LEADING|TRAILING)? sub=expr? FROM )? target=expr PAREN_RIGHT;
Copy link
Member

Choose a reason for hiding this comment

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

Is pattern restrictor its own AST node? Also I was just reading through the grammar and noticed that we don't need many of the element labels. The element and list element labels are useful when the rule has more than one element with the same type. Unless that's the case, they're clutter

For example, we don't need any of the element labels here. It makes it harder to read
Ex: https://github.com/partiql/partiql-lang-kotlin/blob/main/partiql-grammar/src/main/antlr/org/partiql/grammar/PartiQL.g4#L329-L337

Copy link
Member Author

Choose a reason for hiding this comment

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

For this example, are you talking about labels sub and target? Only did that because sub is optional.

Or func? I added the func label just because it made the PartiQLVisitor code look uniform. AKA DateFunction, and TrimFunction, and others result in:

call(ctx.func.text.toLowerCase(), args, metas)

But doesn't matter to me.

And yes, PatternRestrictor is its own AST node.

@johnedquinn johnedquinn marked this pull request as draft October 27, 2022 00:21
@RCHowell RCHowell closed this Mar 31, 2023
@RCHowell RCHowell deleted the nonreserved-keywords branch December 20, 2023 19:35
@RCHowell RCHowell restored the nonreserved-keywords branch December 20, 2023 19:36
@RCHowell RCHowell deleted the nonreserved-keywords branch December 20, 2023 19:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ability to modify SQL keyword list
3 participants