Migrate AggregateSupportRewriter to PartiqlAst.VisitorTransform and ExprNode GroupBy-> PIG GroupBy bug

[alancai98]

Migrate AggregateSupportRewriter to PartiqlAst.VisitorTransform.

Also fixes bug in ExprNodeToStatement when converting ExprNode GroupBy into PIG GroupBy. Previously, metas part of a given groupKey were not included, causing some missing meta errors when calling .toAstStatement with an ExprNode that had a GroupBy clause.

Need to resolve whether a CallAgg used in the evaluator should be stored as an ExprNode or PIG version when doing the VisitorTransform (i.e. when should the CallAgg be converted into an ExprNode CallAgg, in the VisitorTransform, evaluator, or somewhere else?). This current PR does the conversion in the evaluator and leaves the ExprNode concept out of the VisitorTransform. However, it may actually be better to do it in the VisitorTransform (still need to try it out).

Also need to discuss if there's a better way to convert parts of the CallAgg from the PIG version to the ExprNode version. In the current approach, I had to expose some methods from StatementToExprNode.kt to get this working.

Lastly, ran into an issue where an existing test, selectListNestedAggregateCall, may have incorrect behavior? It would make more sense to me that the error's source location should be column 12, which is where the nested aggregate starts. This could be a potential bug in using the wrong metas in the evaluator.

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

[codecov-io]

Codecov Report

Merging #305 into visitor-transforms will increase coverage by 0.03%.
The diff coverage is 93.02%.

@@                   Coverage Diff                    @@
##             visitor-transforms     #305      +/-   ##
========================================================
+ Coverage                 82.33%   82.36%   +0.03%     
- Complexity                 1227     1229       +2     
========================================================
  Files                       159      159              
  Lines                      9386     9402      +16     
  Branches                   1531     1533       +2     
========================================================
+ Hits                       7728     7744      +16     
  Misses                     1197     1197              
  Partials                    461      461              

Flag	Coverage Δ	Complexity Δ
#CLI	18.11% <ø> (ø)	19.00 <ø> (ø)
#EXAMPLES	76.01% <ø> (ø)	27.00 <ø> (ø)
#LANG	85.00% <93.02%> (+0.02%)	1026.00 <7.00> (+2.00)
#PTS	100.00% <ø> (ø)	0.00 <ø> (ø)
#TEST_SCRIPT	79.68% <ø> (ø)	157.00 <ø> (ø)

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

Impacted Files	Coverage Δ	Complexity Δ
...ng/src/org/partiql/lang/ast/ExprNodeToStatement.kt	92.85% <33.33%> (-0.37%)	0.00 <0.00> (ø)
...ng/src/org/partiql/lang/ast/StatementToExprNode.kt	96.72% <88.88%> (+0.06%)	0.00 <0.00> (ø)
.../org/partiql/lang/ast/AggregateCallSiteListMeta.kt	66.66% <100.00%> (ø)	2.00 <1.00> (ø)
lang/src/org/partiql/lang/ast/passes/Rewriters.kt	90.00% <100.00%> (ø)	0.00 <0.00> (ø)
...ng/src/org/partiql/lang/eval/EvaluatingCompiler.kt	81.38% <100.00%> (+0.14%)	152.00 <0.00> (+1.00)
.../eval/visitors/AggregateSupportVisitorTransform.kt	100.00% <100.00%> (ø)	6.00 <6.00> (?)

---

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 6082cc3...df66f86. Read the comment docs.

[alancai98]

Alternate version converting the CallAgg from the VisitorTransform can be found on the visitor-transforms-aggregate-support-alternate branch.

This commit shows how converting may look from the VisitorTransform, leaving the evaluator the same (i.e. using the ExprNode type for CallAgg).

Wasn't sure how to get around passing the IonSystem to the VisitorTransform since the valueFactory must have the same instance of IonSystem as ionValue

[dlurton]

LGTM in general, the only changes I recommend are to my own poor wording in the kdoc I originally wrote.

[dlurton]

Lastly, ran into an issue where an existing test, selectListNestedAggregateCall, may have incorrect behavior? It would make more sense to me that the error's source location should be column 12, which is where the nested aggregate starts. This could be a potential bug in using the wrong metas in the evaluator.

Yes, column 12 seems to be the correct location for that error.