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

Add support for OFFSET #451

Merged
merged 32 commits into from
Oct 12, 2021
Merged

Add support for OFFSET #451

merged 32 commits into from
Oct 12, 2021

Conversation

lziq
Copy link
Contributor

@lziq lziq commented Sep 30, 2021

Issue #422

Changes Details:

  1. Added OFFSET in the PIG domain.

lziq and others added 28 commits September 22, 2021 16:19
…s.kt

Co-authored-by: Alan Cai <caialan@amazon.com>
@lziq lziq self-assigned this Sep 30, 2021
@lziq lziq linked an issue Sep 30, 2021 that may be closed by this pull request
3 tasks
@partiql partiql deleted a comment from codecov-commenter Sep 30, 2021
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.

Addition to the PIG domain looks good.

I have a few minor suggestions for the PR:

  • Since this is the first task of issue Add support for OFFSET  #422, this PR shouldn't be linked to close that issue. I would link the issue in the final task's PR
  • I'd rename the PR title to reflect this is adding OFFSET to the PIG domain
  • Try keeping the PR's commit history clearer by creating a new branch off the target branch for each new PR. Doing this will prevent non-relevant commits from appearing in the PR, which may confuse the reviewer.

@lziq lziq changed the title Add support for OFFSET Add support for OFFSET in PIG domain Sep 30, 2021
@lziq lziq removed a link to an issue Sep 30, 2021
3 tasks
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.

Order needs changed for LIMIT and OFFSET in the visitor transform tasks

@lziq lziq linked an issue Sep 30, 2021 that may be closed by this pull request
3 tasks
@lziq lziq changed the title Add support for OFFSET in PIG domain Add support for OFFSET Sep 30, 2021
@codecov-commenter
Copy link

codecov-commenter commented Sep 30, 2021

Codecov Report

Merging #451 (96f781f) into main (28701e2) will increase coverage by 0.01%.
The diff coverage is 86.95%.

Impacted file tree graph

@@             Coverage Diff              @@
##               main     #451      +/-   ##
============================================
+ Coverage     82.36%   82.37%   +0.01%     
- Complexity     1395     1399       +4     
============================================
  Files           171      171              
  Lines         10722    10741      +19     
  Branches       1766     1771       +5     
============================================
+ Hits           8831     8848      +17     
- Misses         1350     1351       +1     
- Partials        541      542       +1     
Flag Coverage Δ
CLI 18.18% <ø> (ø)
EXAMPLES 74.85% <ø> (ø)
LANG 84.87% <86.95%> (+<0.01%) ⬆️
PTS ∅ <ø> (∅)
TEST_SCRIPT 79.68% <ø> (ø)

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

Impacted Files Coverage Δ
lang/src/org/partiql/lang/ast/AstSerialization.kt 85.57% <33.33%> (-0.41%) ⬇️
lang/src/org/partiql/lang/syntax/SqlParser.kt 79.79% <80.00%> (+0.06%) ⬆️
...ng/src/org/partiql/lang/ast/ExprNodeToStatement.kt 93.79% <100.00%> (+0.02%) ⬆️
...ng/src/org/partiql/lang/ast/StatementToExprNode.kt 97.36% <100.00%> (+<0.01%) ⬆️
lang/src/org/partiql/lang/ast/ast.kt 89.15% <100.00%> (+0.02%) ⬆️
...src/org/partiql/lang/ast/passes/AstRewriterBase.kt 100.00% <100.00%> (ø)
lang/src/org/partiql/lang/ast/passes/AstWalker.kt 71.33% <100.00%> (+0.19%) ⬆️
...ng/src/org/partiql/lang/eval/EvaluatingCompiler.kt 84.22% <100.00%> (ø)
.../visitors/GroupByPathExpressionVisitorTransform.kt 82.89% <100.00%> (+0.46%) ⬆️
...partiql/lang/eval/visitors/VisitorTransformBase.kt 97.43% <100.00%> (+0.13%) ⬆️
... and 2 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 28701e2...96f781f. Read the comment docs.

@lziq
Copy link
Contributor Author

lziq commented Sep 30, 2021

Will finish all the 3 tasks from issue #422 in this branch, and afterwards merge this PR into main.

alancai98
alancai98 previously approved these changes Sep 30, 2021
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.

Looks good for the PIG domain and VisitorTransform changes.

For the remaining two tasks, I'd create separate PRs targeting this branch, add-OFFSET. After those PRs are merged into add-OFFSET, this PR can be looked at again.

@lziq lziq merged commit 9427146 into main Oct 12, 2021
@lziq lziq deleted the add-OFFSET branch October 12, 2021 19:44
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.

Add support for OFFSET
3 participants