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 <[email protected]> Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-Reviewer: Michael Blow <[email protected]> Gerrit-Reviewer: Steven Jacobs <[email protected]> Gerrit-Reviewer: Till Westmann <[email protected]> Gerrit-Reviewer: abdullah alamoudi <[email protected]> Gerrit-HasComments: Yes
