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

Adds OUTER set operators to parser and type domain #678

Merged
merged 4 commits into from
Jul 25, 2022
Merged

Conversation

rchowell
Copy link
Contributor

Issue #, if available:

Part 2 of — #184

Description of changes:

These changes add the OUTER set operators defined in RFC-0007 to the type universe, lexer, and parser.

Have you updated the Unreleased section of CHANGELOG.md with your changes? (y/n), If not, please explain why: No, N/A

Does your PR include any backward-incompatible changes? (y/n), if yes, please explain the reason. In addition, please
also mention any other alternatives you've considered and the reason they've been discarded?:
N

For this purpose, we define backward-incompatible changes as changes that—when consumed—can potentially result in 
errors for users that are using our public APIs or the entities that have `public` visibility in our code-base.

Does your PR introduce a new external dependency? (y/n), if yes, please explain the reason. In addition, please
also mention any other alternatives you've considered and the reason they've been discarded?:
N

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

@codecov-commenter
Copy link

codecov-commenter commented Jul 20, 2022

Codecov Report

Merging #678 (7a302e5) into main (9d20e4a) will increase coverage by 0.79%.
The diff coverage is 71.27%.

@@             Coverage Diff              @@
##               main     #678      +/-   ##
============================================
+ Coverage     79.61%   80.40%   +0.79%     
- Complexity     2350     2558     +208     
============================================
  Files           236      240       +4     
  Lines         19358    21003    +1645     
  Branches       3768     4248     +480     
============================================
+ Hits          15411    16888    +1477     
- Misses         2660     2793     +133     
- Partials       1287     1322      +35     
Flag Coverage Δ
CLI 22.68% <ø> (ø)
EXAMPLES 76.24% <ø> (ø)
EXTENSIONS 69.23% <ø> (ø)
LANG 82.36% <71.27%> (+0.69%) ⬆️
PTS ∅ <ø> (∅)
TEST_SCRIPT 77.98% <ø> (ø)

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

Impacted Files Coverage Δ
...ng/src/org/partiql/lang/ast/StatementToExprNode.kt 90.38% <0.00%> (-0.76%) ⬇️
.../eval/physical/PhysicalExprToThunkConverterImpl.kt 85.17% <0.00%> (+3.98%) ⬆️
...planner/transforms/AstToLogicalVisitorTransform.kt 90.29% <ø> (+1.65%) ⬆️
lang/src/org/partiql/lang/syntax/SqlLexer.kt 93.54% <ø> (ø)
...ng/src/org/partiql/lang/eval/EvaluatingCompiler.kt 77.60% <14.28%> (-0.10%) ⬇️
...c/org/partiql/lang/prettyprint/ASTPrettyPrinter.kt 79.78% <28.57%> (-1.76%) ⬇️
...ng/src/org/partiql/lang/ast/ExprNodeToStatement.kt 92.95% <86.95%> (+0.03%) ⬆️
lang/src/org/partiql/lang/syntax/LexerConstants.kt 99.14% <100.00%> (+0.04%) ⬆️
lang/src/org/partiql/lang/syntax/SqlParser.kt 82.63% <100.00%> (+4.87%) ⬆️
...ang/src/org/partiql/lang/ast/AstDeserialization.kt 82.03% <0.00%> (-2.23%) ⬇️
... and 16 more

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 9d20e4a...7a302e5. Read the comment docs.

Copy link
Member

@alancai98 alancai98 left a comment

Choose a reason for hiding this comment

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

Left some comments about the modeling of OUTER in set operators and not needing to support OUTER in the older AST versions. Also noticed that specifying DISTINCT following the set operator results in a parser error.

@rchowell rchowell requested a review from alancai98 July 25, 2022 16:07
Copy link
Member

@alancai98 alancai98 left a comment

Choose a reason for hiding this comment

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

Thanks for adding more tests and addressing the feedback. Left a comment about some operators missing from the precedence map and some rewritten SqlParserTest test cases.

Copy link
Member

@alancai98 alancai98 left a comment

Choose a reason for hiding this comment

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

:shipit:

@rchowell rchowell merged commit e455cf4 into main Jul 25, 2022
rchowell added a commit that referenced this pull request Aug 16, 2022
Co-authored-by: R. C. Howell <howero@amazon.com>
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.

3 participants