abdullah alamoudi has posted comments on this change.

Change subject: Enhanced Insert AQL
......................................................................


Patch Set 10:

(6 comments)

Comments added.

Beside the comments, I am worried about the implications of the use of the 
extension operator. I don't fully understand it but would be nice to get an 
explanation on how it changes things.

https://asterix-gerrit.ics.uci.edu/#/c/1150/10/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/algebra/extension/IExtensionStatement.java
File 
asterixdb/asterix-algebra/src/main/java/org/apache/asterix/algebra/extension/IExtensionStatement.java:

Line 42:      * Called when the {@code IQueryTranslator} encounters an 
extension statement.
Fix the javadoc?


Line 52:     void handle(IStatementExecutor statementExecutor, 
AqlMetadataProvider metadataProvider,
Should we change this to only pass IStatementExecutor and create getters for 
the other passed parameters?


https://asterix-gerrit.ics.uci.edu/#/c/1150/10/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/algebra/operators/CommitOperator.java
File 
asterixdb/asterix-algebra/src/main/java/org/apache/asterix/algebra/operators/CommitOperator.java:

Line 63: 
setVar is not used by core. add a comment that it is used by extensions? 
Thoughts @Till?


https://asterix-gerrit.ics.uci.edu/#/c/1150/10/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/algebra/operators/physical/UpsertCommitRuntime.java
File 
asterixdb/asterix-algebra/src/main/java/org/apache/asterix/algebra/operators/physical/UpsertCommitRuntime.java:

Line 35:                 resourcePartition, true);
mmmmmm, wouldn't this cause a problem if someone attempts Upsert with returning 
new values?


Add a test case?


https://asterix-gerrit.ics.uci.edu/#/c/1150/10/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/IntroduceSecondaryIndexInsertDeleteRule.java
File 
asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/IntroduceSecondaryIndexInsertDeleteRule.java:

Line 96:         if (op0.getOperatorTag() != 
LogicalOperatorTag.EXTENSION_OPERATOR
I think that all rewrite rules should be extension-agnostic. If an extension 
needs a rewrite rule behavior to change, it should override it.


https://asterix-gerrit.ics.uci.edu/#/c/1150/10/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 423:         ILogicalOperator leafOperator = new ExtensionOperator(new 
CommitOperator(true));
I just noticed this ExtensionOperator which seems to have been here for a long 
time. why would we want to use it with a commit operator instead of a sink?
I think this could have some hidden effects!


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I65789d2a861d15232dd29156a6987d0635ec6c94
Gerrit-PatchSet: 10
Gerrit-Project: asterixdb
Gerrit-Branch: master
Gerrit-Owner: Steven Jacobs <sjaco...@ucr.edu>
Gerrit-Reviewer: Jenkins <jenk...@fulliautomatix.ics.uci.edu>
Gerrit-Reviewer: Michael Carey <dtab...@gmail.com>
Gerrit-Reviewer: Steven Jacobs <sjaco...@ucr.edu>
Gerrit-Reviewer: Till Westmann <ti...@apache.org>
Gerrit-Reviewer: Yingyi Bu <buyin...@gmail.com>
Gerrit-Reviewer: abdullah alamoudi <bamou...@gmail.com>
Gerrit-HasComments: Yes

Reply via email to