Skip to content

Adds non-reserved keywords (POC) #221

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Adds non-reserved keywords (POC) #221

wants to merge 1 commit into from

Conversation

johnedquinn
Copy link
Member

@johnedquinn johnedquinn commented Nov 18, 2022 •
edited
Loading

Relevant Issues

Description

  • This is a proof of concept (POC) for adding non-reserved keywords to the Rust implementation
  • I've added several keywords to the NonReservedKeywords list in LALRPOP
    • ACYCLIC, BOTH, DOMAIN, LEADING, TRAIL, TRAILING, and USER to match the PR linked above
    • Also added ORDER, but this is only because ORDER is already used within Rust's grammar as a keyword -- and I wanted to show how existing keywords can be converted to non-reserved keywords.
  • I also modified the preprocessor so that it can parse TRAILING, LEADING, and BOTH appropriately.
  • If they can be parsed as VarRefExprs, we convert them to SymbolPrimitives (case-insensitive), so we don't even need to know what their original case is. I didn't add qualified/unqualified to this PR, as it's a quick POC.
  • I've added tests to show several scenarios of using non-reserved keywords.
  • I don't intend for this to be merged, as I am not familiar with the codebase -- however I wanted to show how it can possibly be done.
  • Note: I'm unfamiliar with this language, so please excuse mistakes made along the way

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

@johnedquinn johnedquinn requested a review from jpschorr November 18, 2022 01:26
@johnedquinn johnedquinn added the enhancement New feature or request label Nov 18, 2022
@johnedquinn johnedquinn force-pushed the non-reserved-keywords branch from 4d3745a to 0ca11b3 Compare November 18, 2022 01:30
@codecov
Copy link

codecov bot commented Nov 18, 2022 •
edited
Loading

Codecov Report

Base: 86.28% // Head: 86.72% // Increases project coverage by +0.43% 🎉

Coverage data is based on head (489c9f7) compared to base (a279276).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main     #221      +/-   ##
==========================================
+ Coverage   86.28%   86.72%   +0.43%     
==========================================
  Files          31       31              
  Lines        5074     5226     +152     
==========================================
+ Hits         4378     4532     +154     
+ Misses        696      694       -2
Impacted Files Coverage Δ
partiql-parser/src/lexer.rs 98.34% <100.00%> (+0.08%) ⬆️
partiql-parser/src/parse/mod.rs 95.21% <100.00%> (+0.37%) ⬆️
partiql-parser/src/preprocessor.rs 94.47% <100.00%> (+1.63%) ⬆️

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.

@github-actions
Copy link

github-actions bot commented Nov 18, 2022 •
edited
Loading

Conformance comparison report

main (a279276) e3a3e60 +/-
% Passing 12.65% 12.65% 0.00%
✅ Passing 349 349 0
❌ Failing 0 0 0
🔶 Ignored 2410 2410 0
Total Tests 2759 2759 0

Number passing in both: 349

Number failing in both: 0

Number passing in main but now fail: 0

Number failing in main but now pass: 0

@johnedquinn johnedquinn force-pushed the non-reserved-keywords branch from 0ca11b3 to 489c9f7 Compare November 18, 2022 06:04
jpschorr
jpschorr reviewed Nov 18, 2022
View reviewed changes
partiql-parser/src/parse/mod.rs
@@ -679,6 +679,55 @@ mod tests {
}
}

// PROOF OF CONCEPT: NON-RESERVED KEYWORDS
mod non_reserved {
Copy link
Contributor

Choose a reason for hiding this comment

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

These tests all pass without any of the above parser changes, except for with_order (thought I think the gpml tests would fail when the gpml parsing rules are included).

I'm also not sure that i feel treating order as an unreserved keyword is a good idea...

@johnedquinn johnedquinn changed the title Adds non-reserved keywords Adds non-reserved keywords (POC) Nov 18, 2022
@johnedquinn johnedquinn mentioned this pull request Nov 18, 2022
Merged
@jpschorr
Copy link
Contributor

Non-reserved keywords were added as part of #552

@jpschorr jpschorr closed this Aug 18, 2025
@johnedquinn johnedquinn deleted the non-reserved-keywords branch August 18, 2025 21:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jpschorr jpschorr jpschorr left review comments

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@johnedquinn @jpschorr