asterixdb-notifications mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Till Westmann (Code Review)" <do-not-re...@asterixdb.incubator.apache.org>
Subject Change in asterixdb[master]: ASTERIXDB-1755: add UPSERT in SQL++.
Date Mon, 02 Jan 2017 22:35:58 GMT
Till Westmann has posted comments on this change.

Change subject: ASTERIXDB-1755: add UPSERT in SQL++.
......................................................................


Patch Set 9:

(14 comments)

https://asterix-gerrit.ics.uci.edu/#/c/1401/9//COMMIT_MSG
Commit Message:

Line 9: Detailed list of changes include:
s/include/included/?


https://asterix-gerrit.ics.uci.edu/#/c/1401/9/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/CheckInsertUpsertRule.java
File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/CheckInsertUpsertRule.java:

Line 3:  *  * Licensed to the Apache Software Foundation (ASF) under one
Double indentation of comment.


Line 37: public class CheckInsertUpsertRule implements IAlgebraicRewriteRule {
Should this be called CheckReturningRule? It's really about RETURNING and not about any other
INSERTs or UPSERTs. (Maybe even CheckReturningClauseRule ...).


https://asterix-gerrit.ics.uci.edu/#/c/1401/9/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/util/InsertUpsertCheckUtil.java
File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/util/InsertUpsertCheckUtil.java:

Line 3:  *  * Licensed to the Apache Software Foundation (ASF) under one
Double indentation of comment.


Line 39: 
Remove the empty line?


https://asterix-gerrit.ics.uci.edu/#/c/1401/9/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/translator/AqlExpressionToPlanTranslator.java
File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/translator/AqlExpressionToPlanTranslator.java:

Line 175:         if (expr.getKind() == Kind.FLWOGR_EXPRESSION) {
reuse "isFLWOGR" ?


https://asterix-gerrit.ics.uci.edu/#/c/1401/9/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 567:             if (compiledUpsert.getReturnExpression() != null) {
Reuse "returnExpression" declared above?


Line 629:             rootOperator = new DelegateOperator(new CommitOperator(returnExpression
== null ? true : false));
s/returnExpression == null ? true : false/returnExpression == null/ ?


Line 645:             rootOperator.getInputs().add(new MutableObject<>(upsertOp));
Could we pull these last 2 lines behind the else?


https://asterix-gerrit.ics.uci.edu/#/c/1401/9/asterixdb/asterix-lang-aql/src/main/java/org/apache/asterix/lang/aql/rewrites/AqlQueryRewriter.java
File asterixdb/asterix-lang-aql/src/main/java/org/apache/asterix/lang/aql/rewrites/AqlQueryRewriter.java:

Line 61:     private void setup(List<FunctionDecl> declaredFunctions, StatementWithReturn
topExpr,
Rename parameter to "topStatement" (or "topStmt") as well?


Line 71:     public void rewrite(List<FunctionDecl> declaredFunctions, StatementWithReturn
topExpr,
Rename parameter to "topStatement" (or "topStmt") as well?


https://asterix-gerrit.ics.uci.edu/#/c/1401/9/asterixdb/asterix-lang-common/src/main/java/org/apache/asterix/lang/common/base/StatementWithReturn.java
File asterixdb/asterix-lang-common/src/main/java/org/apache/asterix/lang/common/base/StatementWithReturn.java:

Line 3:  *  * Licensed to the Apache Software Foundation (ASF) under one
Double indentation of comment.


Line 30: public interface StatementWithReturn extends Statement {
Maybe rename this to "IReturningStatement"? 

"StatementWithReturn" confused me in the beginning as e.g. SQL++ queries don't mention RETURN
at all and INSERT/UPSERT have a RETURNING clause. 

I'm proposing "IReturningStatement" as a way of saying "this is a statement that returns data"
and not as "this is a a statement that has a returning clause". I see that that still could
be confusing but IStatementThatReturnsData seems bad as well ...

I propose to prefix the interface name with "I" even though this is inconsistent with "Statement",
because is is consistent with most other interface is in org.apache.asterix.lang.common.base
(and I think that we should probably rename "Statement" to "IStatement" in another change
for more consistency).


https://asterix-gerrit.ics.uci.edu/#/c/1401/9/asterixdb/asterix-lang-sqlpp/src/main/java/org/apache/asterix/lang/sqlpp/visitor/base/AbstractSqlppExpressionScopingVisitor.java
File asterixdb/asterix-lang-sqlpp/src/main/java/org/apache/asterix/lang/sqlpp/visitor/base/AbstractSqlppExpressionScopingVisitor.java:

Line 365:         Expression returningExpr = insertStatement.getReturnExpression();
Move this down to the place where it is used?


-- 
To view, visit https://asterix-gerrit.ics.uci.edu/1401
To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I02b2be7ff2653573eccb48037895f5c8c4bc8c74
Gerrit-PatchSet: 9
Gerrit-Project: asterixdb
Gerrit-Branch: master
Gerrit-Owner: Yingyi Bu <buyingyi@gmail.com>
Gerrit-Reviewer: Jenkins <jenkins@fulliautomatix.ics.uci.edu>
Gerrit-Reviewer: Michael Blow <mblow@apache.org>
Gerrit-Reviewer: Steven Jacobs <sjaco002@ucr.edu>
Gerrit-Reviewer: Till Westmann <tillw@apache.org>
Gerrit-Reviewer: abdullah alamoudi <bamousaa@gmail.com>
Gerrit-HasComments: Yes

Mime
View raw message