Steven Jacobs has posted comments on this change.

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


Patch Set 9:

(14 comments)

I addressed everything except for the documentation comment. Does this comment 
fit more into the "It would be even cooler if we could do this" or the "it 
seems wrong to do it without this" category?

To me it seems like the former. If this is the case, maybe we can create this 
as an enhancement Jira issue? The reason I suggest this is that it would be a 
nontrivial change, and I'm not sure I'm currently in the position to dedicate 
the cycles for it.

I think there are definitely good use cases for this change as it is now, so it 
doesn't seem to be to be "baked" just for my change.

However, if the change as it is now fits into the "it seems wrong" category, 
please let me know and we can talk more about it.

https://asterix-gerrit.ics.uci.edu/#/c/1150/9/asterixdb/asterix-active/src/main/java/org/apache/asterix/active/IActiveEntityEventsListener.java
File 
asterixdb/asterix-active/src/main/java/org/apache/asterix/active/IActiveEntityEventsListener.java:

Line 34:     public boolean isEntityUsingDataset(String dataverseName, String 
datasetName);
> Seems unrelated ..
I need to add a line to the commit saying "Prepare Asterix for Channel 
Extension"


https://asterix-gerrit.ics.uci.edu/#/c/1150/9/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 54:             int resultSetIdCounter) throws HyracksDataException, 
AlgebricksException;
> Seems unrelated as well ...
I need to add a line to the commit saying "Prepare Asterix for Channel 
Extension"


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

Line 101:                 if (op1.getOperatorTag() == 
LogicalOperatorTag.EXTENSION_OPERATOR) {
> This seems unrelated as well ...
This one is a case of name crossover :)
An EXTENSION_OPERATOR actually has nothing to do with Asterix Extensions. It is 
an operator that holds an underlying operator (in this case a commit).


https://asterix-gerrit.ics.uci.edu/#/c/1150/9/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
> Seems unrelated.
This one is a case of name crossover :) An EXTENSION_OPERATOR actually has 
nothing to do with Asterix Extensions. It is an operator that holds an 
underlying operator (in this case a commit).


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

Line 96:         if (op1.getOperatorTag() == 
LogicalOperatorTag.EXTENSION_OPERATOR) {
> Seems unrelated ...
This one is a case of name crossover :) An EXTENSION_OPERATOR actually has 
nothing to do with Asterix Extensions. It is an operator that holds an 
underlying operator (in this case a commit).


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

Line 66:         ExtensionOperator eOp = (ExtensionOperator) op;
> Seems unrelated.
This one is a case of name crossover :) An EXTENSION_OPERATOR actually has 
nothing to do with Asterix Extensions. It is an operator that holds an 
underlying operator (in this case a commit).


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

Line 26: import org.apache.asterix.external.feed.watch.FeedActivityDetails;
> Seems unrelated.
I removed the class FeedActivity which wasn't being used.


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

Line 305:                     throws AlgebricksException {
> Revert this file?
Done


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

Line 86:                     AlgebricksBuiltinFunctions.GE, 
AlgebricksBuiltinFunctions.LT, AlgebricksBuiltinFunctions.GT));
> There seems to be no semantic change in this file, can we revert it?
Done


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

Line 108:                     assignsAndUnnestsRefs.add(subTreeOpRef);
> This seems to be a step back in terms of extensibility, could we revert the
Done


https://asterix-gerrit.ics.uci.edu/#/c/1150/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 423:         ILogicalOperator leafOperator = new ExtensionOperator(new 
CommitOperator(true));
> Why do we need an ExtensionOperator here?
Once again, the names are confusing here. This ExtensionOperator doesn't relate 
to Asterix Extensions, but is rather the way that Asterix holds commit 
operators.


https://asterix-gerrit.ics.uci.edu/#/c/1150/9/asterixdb/asterix-app/src/main/java/org/apache/asterix/app/translator/QueryTranslator.java
File 
asterixdb/asterix-app/src/main/java/org/apache/asterix/app/translator/QueryTranslator.java:

Line 399:                         ((IExtensionStatement) stmt).handle(this, 
metadataProvider, hcc, hdc, resultDelivery, stats,
> Is this related?
This is another piece of the change to prepare for the BAD extension


https://asterix-gerrit.ics.uci.edu/#/c/1150/9/asterixdb/asterix-app/src/test/java/org/apache/asterix/test/runtime/ExecutionTestUtil.java
File 
asterixdb/asterix-app/src/test/java/org/apache/asterix/test/runtime/ExecutionTestUtil.java:

Line 60:             AsterixHyracksIntegrationUtil alternateIntegrationUtil, 
boolean startHdfs) throws Exception {
> What is alternateIntegrationUtil?
This was a bug. ExecutionTestUtil has a built-in integration util, and this 
function should override it. I renamed the field to make it more clear what was 
happening.


https://asterix-gerrit.ics.uci.edu/#/c/1150/9/asterixdb/asterix-om/src/main/java/org/apache/asterix/om/base/AUUID.java
File asterixdb/asterix-om/src/main/java/org/apache/asterix/om/base/AUUID.java:

Line 98:         StringBuilder buf = new StringBuilder(UUID_CHARS + 9);
> Why 9 extra chars here?
I'm not sure why exactly, but it is the same as the toString method just above.


-- 
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: 9
Gerrit-Project: asterixdb
Gerrit-Branch: master
Gerrit-Owner: Steven Jacobs <sjaco...@ucr.edu>
Gerrit-Reviewer: Jenkins <jenk...@fulliautomatix.ics.uci.edu>
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