Mailing-List: contact notifications-help@asterixdb.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@asterixdb.apache.org
Date: Sun, 14 Aug 2016 01:24:15 -0700
From: "Till Westmann (Code Review)" <do-not-reply@asterixdb.incubator.apache.org>
To: Yingyi Bu <buyingyi@gmail.com>
CC: Jenkins <jenkins@fulliautomatix.ics.uci.edu>
Reply-To: tillw@apache.org
Subject: Change in asterixdb[master]: Clean up GROUP BY and WITH clause.
In-Reply-To: <gerrit.1470372127281.I62fca7f937aa007d97ed87c75cef19f6aa3e5ade@asterix-gerrit.ics.uci.edu>
References: <gerrit.1470372127281.I62fca7f937aa007d97ed87c75cef19f6aa3e5ade@asterix-gerrit.ics.uci.edu>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Content-Disposition: inline
User-Agent: Gerrit/2.8.4
Message-Id: <20160814082415.43FA5241ED1@unhygienix.ics.uci.edu>
archived-at: Sun, 14 Aug 2016 08:24:20 -0000

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 <buyingyi@gmail.com>
Gerrit-Reviewer: Jenkins <jenkins@fulliautomatix.ics.uci.edu>
Gerrit-Reviewer: Till Westmann <tillw@apache.org>
Gerrit-HasComments: Yes