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