Till Westmann has posted comments on this change. Change subject: Clean up GROUP BY and WITH clause. ......................................................................
Patch Set 18: (16 comments) Only a few formalisms - otherwise things look good (as far as I can tell :) ). https://asterix-gerrit.ics.uci.edu/#/c/1050/18/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/base/RuleCollections.java File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/base/RuleCollections.java: Line 135: List<IAlgebraicRewriteRule> translationRule = new LinkedList<>(); s/translationRule/translationRules/ https://asterix-gerrit.ics.uci.edu/#/c/1050/18/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/ResolveVariableRule.java File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/ResolveVariableRule.java: Line 57: * based on the available type and metatadata information. s/metatadata/metadata/ https://asterix-gerrit.ics.uci.edu/#/c/1050/18/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/translator/LangExpressionToPlanTranslator.java File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/translator/LangExpressionToPlanTranslator.java: Line 792: // A gloabal aggregation can still have a decoration variable list which are used for propagate s/gloabal/global/ https://asterix-gerrit.ics.uci.edu/#/c/1050/18/asterixdb/asterix-lang-sqlpp/src/main/java/org/apache/asterix/lang/sqlpp/rewrites/visitor/VariableCheckAndRewriteVisitor.java File asterixdb/asterix-lang-sqlpp/src/main/java/org/apache/asterix/lang/sqlpp/rewrites/visitor/VariableCheckAndRewriteVisitor.java: Line 109: // asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/subquery/exists Should we capture this somewhere (e.g. in an issue)? Then we could track it and also put it into release notes (if we ever start doing those ...) https://asterix-gerrit.ics.uci.edu/#/c/1050/18/asterixdb/asterix-lang-sqlpp/src/main/java/org/apache/asterix/lang/sqlpp/util/SqlppVariableUtil.java File asterixdb/asterix-lang-sqlpp/src/main/java/org/apache/asterix/lang/sqlpp/util/SqlppVariableUtil.java: Line 89: if(includeWithVariables || !liveVar.getVar().namedValueAccess()) { missing space Line 105: if(langExpr == null){ missing space Also, when do we ever call this on a null expression ... https://asterix-gerrit.ics.uci.edu/#/c/1050/18/asterixdb/asterix-lang-sqlpp/src/main/java/org/apache/asterix/lang/sqlpp/visitor/SqlppSubstituteExpressionVisitor.java File asterixdb/asterix-lang-sqlpp/src/main/java/org/apache/asterix/lang/sqlpp/visitor/SqlppSubstituteExpressionVisitor.java: Line 20: */ Too much nesting in the license. Line 51: // get substituted first if some of their child expressions present as keys in the exprMap. child expressions *are* present ? https://asterix-gerrit.ics.uci.edu/#/c/1050/18/asterixdb/asterix-om/src/main/java/org/apache/asterix/om/typecomputer/impl/FieldAccessByNameResultType.java File asterixdb/asterix-om/src/main/java/org/apache/asterix/om/typecomputer/impl/FieldAccessByNameResultType.java: Line 49: throw new AlgebricksException("The second argument of a field access should be an STRING, but it is found " remove "found" ? https://asterix-gerrit.ics.uci.edu/#/c/1050/18/asterixdb/asterix-om/src/main/java/org/apache/asterix/om/typecomputer/impl/PropagateTypeComputer.java File asterixdb/asterix-om/src/main/java/org/apache/asterix/om/typecomputer/impl/PropagateTypeComputer.java: Line 20: */ Too much nesting in the license. https://asterix-gerrit.ics.uci.edu/#/c/1050/18/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/aggregates/collections/FirstElementAggregateDescriptor.java File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/aggregates/collections/FirstElementAggregateDescriptor.java: Line 20: */ Too much nesting in the license. https://asterix-gerrit.ics.uci.edu/#/c/1050/18/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/aggregates/collections/FirstElementEvalFactory.java File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/aggregates/collections/FirstElementEvalFactory.java: Line 20: */ Too much nesting in the license. Line 62: first = true; Only 1 space? Line 67: if(!first){ space between "if" and "(" https://asterix-gerrit.ics.uci.edu/#/c/1050/18/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/aggregates/collections/LocalFirstElementAggregateDescriptor.java File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/aggregates/collections/LocalFirstElementAggregateDescriptor.java: Line 20: */ Too much nesting in the license. https://asterix-gerrit.ics.uci.edu/#/c/1050/18/hyracks-fullstack/algebricks/algebricks-rewriter/src/main/java/org/apache/hyracks/algebricks/rewriter/rules/ExtractGroupByDecorVariablesRule.java File hyracks-fullstack/algebricks/algebricks-rewriter/src/main/java/org/apache/hyracks/algebricks/rewriter/rules/ExtractGroupByDecorVariablesRule.java: Line 21: Too much nesting in the license. -- To view, visit https://asterix-gerrit.ics.uci.edu/1050 To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-MessageType: comment Gerrit-Change-Id: I62fca7f937aa007d97ed87c75cef19f6aa3e5ade Gerrit-PatchSet: 18 Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Owner: Yingyi Bu <buyin...@gmail.com> Gerrit-Reviewer: Jenkins <jenk...@fulliautomatix.ics.uci.edu> Gerrit-Reviewer: Till Westmann <ti...@apache.org> Gerrit-HasComments: Yes
