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

Added Pos and Neg operators. #532

Merged
merged 4 commits into from
Feb 23, 2022
Merged

Conversation

lziq
Copy link
Contributor

@lziq lziq commented Feb 23, 2022

Motivation:
As we are replacing ExprNode with PIG-generated AST, we need to resolve the inconsistency between them.

With 2 or more arguments provided, ExprNode uses NAry.add(NAry.Sub) to model the addition operator, while PIG-generated AST uses Plus(Minus) to model it. However, with only one argument provided, the addition operator is modeled as Pos(Neg) in PIG AST, while in ExprNode it is still NAry.add(NAry.Sub).

This PR aims to resolve this inconsistency.

Suggested reviewing order:

  1. SqlParser.kt and EvaluatingCompiler.kt
  2. ExprNodeToStatement.kt and StatementToExprNode.kt.
  3. Other files.

@codecov-commenter
Copy link

codecov-commenter commented Feb 23, 2022

Codecov Report

Merging #532 (7494ec0) into deprecate-exprnode (d165c77) will decrease coverage by 0.07%.
The diff coverage is 65.21%.

Impacted file tree graph

@@                   Coverage Diff                    @@
##             deprecate-exprnode     #532      +/-   ##
========================================================
- Coverage                 80.72%   80.64%   -0.08%     
- Complexity                 1694     1696       +2     
========================================================
  Files                       196      196              
  Lines                     14387    14413      +26     
  Branches                   2977     2985       +8     
========================================================
+ Hits                      11614    11624      +10     
- Misses                     1811     1819       +8     
- Partials                    962      970       +8     
Flag Coverage Δ
CLI 26.53% <ø> (ø)
EXAMPLES 74.49% <ø> (ø)
LANG 82.43% <65.21%> (-0.09%) ⬇️
PTS ∅ <ø> (∅)
TEST_SCRIPT 76.92% <ø> (ø)

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

Impacted Files Coverage Δ
...ng/src/org/partiql/lang/ast/ExprNodeToStatement.kt 92.66% <50.00%> (-1.11%) ⬇️
...al/visitors/StaticTypeInferenceVisitorTransform.kt 78.46% <50.00%> (-0.67%) ⬇️
lang/src/org/partiql/lang/syntax/SqlParser.kt 76.83% <50.00%> (-0.17%) ⬇️
...ng/src/org/partiql/lang/eval/EvaluatingCompiler.kt 79.27% <75.00%> (-0.11%) ⬇️
...ng/src/org/partiql/lang/ast/StatementToExprNode.kt 92.12% <100.00%> (+0.58%) ⬆️
...c/org/partiql/lang/ast/passes/StatementRedactor.kt 80.00% <100.00%> (-0.89%) ⬇️

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 d165c77...7494ec0. Read the comment docs.

@am357
Copy link
Contributor

am357 commented Feb 23, 2022

Could we also have the motivation in the PR notes (or the related issue if there is one)?

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.

A couple typos in the EvaluatingCompiler and a couple nits in the inferencer. Could you also add more context for this PR? May want to mention that ExprNode had modeled unary + and - as the same as NAry, while PartiqlAst models them as separate nodes.

lziq and others added 3 commits February 23, 2022 12:29
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:

@lziq lziq merged commit f64590e into deprecate-exprnode Feb 23, 2022
@lziq lziq deleted the deprecate-exprnode-plus-minus branch February 23, 2022 20:44
@lziq lziq mentioned this pull request Feb 25, 2022
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.

4 participants