Taewoo Kim has posted comments on this change. Change subject: Index-only plan ......................................................................
Patch Set 11: (18 comments) https://asterix-gerrit.ics.uci.edu/#/c/744/11/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/operators/logical/AbstractReplicateOperator.java File algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/operators/logical/AbstractReplicateOperator.java: Line 36: private boolean[] outputMaterializationFlags = new boolean[outputArity]; > I wonder why this flag is required. This is introduced in the patch set - https://asterix-gerrit.ics.uci.edu/#/c/86/. >From the explanation: "modified the replicate operator descriptor to >materialize the input if needed, and read from the materialized file for the >outputs that requires materialization" https://asterix-gerrit.ics.uci.edu/#/c/744/11/algebricks/algebricks-rewriter/src/main/java/org/apache/hyracks/algebricks/rewriter/rules/RemoveRedundantGroupByDecorVarsRule.java File algebricks/algebricks-rewriter/src/main/java/org/apache/hyracks/algebricks/rewriter/rules/RemoveRedundantGroupByDecorVarsRule.java: Line 45: public class RemoveRedundantGroupByDecorVarsRule implements IAlgebraicRewriteRule { > Is this class introduced for just for optimization, not for the correctness Actually, the old rule name is "RemoveRedundantGroupByDecorVars". I think it is also related to the correctness issue since it removes duplicated variables. I have changed the name and revised the rule. https://asterix-gerrit.ics.uci.edu/#/c/744/11/asterix-transactions/src/main/java/org/apache/asterix/transaction/management/opcallbacks/PrimaryIndexSearchOperationDatasetLevelCallback.java File asterix-transactions/src/main/java/org/apache/asterix/transaction/management/opcallbacks/PrimaryIndexSearchOperationDatasetLevelCallback.java: Line 35: public class PrimaryIndexSearchOperationDatasetLevelCallback extends AbstractOperationCallback implements ISearchOperationCallback { > This class should be removed. Done https://asterix-gerrit.ics.uci.edu/#/c/744/11/asterix-transactions/src/main/java/org/apache/asterix/transaction/management/opcallbacks/SecondaryIndexInstantSearchOperationTryLockOnlyOnPKCallback.java File asterix-transactions/src/main/java/org/apache/asterix/transaction/management/opcallbacks/SecondaryIndexInstantSearchOperationTryLockOnlyOnPKCallback.java: Line 33: public class SecondaryIndexInstantSearchOperationTryLockOnlyOnPKCallback extends AbstractOperationCallback > Can we give a simpler class name, e.g., SecondaryIndexInstantSearchOperatio Done https://asterix-gerrit.ics.uci.edu/#/c/744/11/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/am/AbstractIntroduceAccessMethodRule.java File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/am/AbstractIntroduceAccessMethodRule.java: Line 644: .getIxJoinOuterAdditionalDataSourceVariables(i); > What's the difference between subTreeDataSourceVars and subTreePKs? why is DataSourceVars include both PK and RECORD. subTreePKs only includes PK. subTreeDataSourceVars nor subTreePKs will not be changed. Here, we only get filename and field types. Line 792: expr = (AbstractLogicalExpression) childFuncExpr.getArguments().get(0).getValue(); > better be dealing with an else case explicitly, i.e., neither Assign nor un There are no cases other than (assign or unseats) since we only keep these operators. Besides, the master branch has been refactored. So, this will be changed in the next patch set that I am going to upload. https://asterix-gerrit.ics.uci.edu/#/c/744/11/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/am/AccessMethodAnalysisContext.java File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/am/AccessMethodAnalysisContext.java: Line 65: // whether a verification (especially for R-Tree case) is required after the secondary index search? > what's the difference between 3. requireVerificationAfterSIdxSearch and 4. Done Line 67: // Does the given index can cover all search predicates? > It seems that the line 67 and 69 should be switched?? Done https://asterix-gerrit.ics.uci.edu/#/c/744/11/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/am/AccessMethodJobGenParams.java File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/am/AccessMethodJobGenParams.java: Line 44: protected boolean isPrimaryIndex; > Please say that "isPrimaryIndex is a derived parameter from the index name, Done https://asterix-gerrit.ics.uci.edu/#/c/744/11/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 260: // If it is not granted, then we need to do a secondary index lookup, sort PKs, > Do we sort in this case? ( I thought we don't do sort for this trylockfailu You are correct. We don't do sort. I have corrected the comment. Line 1301: ((AbstractFunctionCallExpression) createOriginalSpatialObjectExpr).getArguments().addAll(expressions); > Here, the createOriginalSpatialObjectExpr should be added to restoredSecond Done https://asterix-gerrit.ics.uci.edu/#/c/744/11/hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/operators/logical/AbstractLogicalOperator.java File hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/operators/logical/AbstractLogicalOperator.java: Line 61: // The code used for canDecreaseCardinality() - whether this operator can decrease the input cardinality > Please explain the meaning of each enum value, especially for CANBEBOTH and Done Line 70: public enum canPreserveOrderCode { > Please explain the meaning of each enum value, especially for CANBEBOTH and Done https://asterix-gerrit.ics.uci.edu/#/c/744/11/hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/operators/logical/LeftOuterJoinOperator.java File hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/operators/logical/LeftOuterJoinOperator.java: Line 83: return canPreserveOrderCode.CANBEBOTH; > Again, what's the meaning of CANBEBOTH, here? Done https://asterix-gerrit.ics.uci.edu/#/c/744/11/hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/operators/logical/TokenizeOperator.java File hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/operators/logical/TokenizeOperator.java: Line 188: return canDecreaseCardinalityCode.FALSE; > Empty string may not generate any token? If yes, should this be true? I think you are correct. I have changed this to true. https://asterix-gerrit.ics.uci.edu/#/c/744/11/hyracks-fullstack/algebricks/algebricks-rewriter/src/main/java/org/apache/hyracks/algebricks/rewriter/rules/RemoveRedundantGroupByDecorVarsRule.java File hyracks-fullstack/algebricks/algebricks-rewriter/src/main/java/org/apache/hyracks/algebricks/rewriter/rules/RemoveRedundantGroupByDecorVarsRule.java: Line 45: public class RemoveRedundantGroupByDecorVarsRule implements IAlgebraicRewriteRule { > Why this class is duplicated in hyracks and algebricks ? Corrected. https://asterix-gerrit.ics.uci.edu/#/c/744/11/hyracks-fullstack/algebricks/algebricks-rewriter/src/main/java/org/apache/hyracks/algebricks/rewriter/rules/RemoveRedundantVariablesRule.java File hyracks-fullstack/algebricks/algebricks-rewriter/src/main/java/org/apache/hyracks/algebricks/rewriter/rules/RemoveRedundantVariablesRule.java: Line 188: if (equivalentVars != null && !equivalentVars.isEmpty()) { > equivalentVars are not null for sure since it's created in line 186. Done https://asterix-gerrit.ics.uci.edu/#/c/744/11/hyracks/hyracks-data/hyracks-data-std/src/main/java/org/apache/hyracks/data/std/util/BinaryHashSet.java File hyracks/hyracks-data/hyracks-data-std/src/main/java/org/apache/hyracks/data/std/util/BinaryHashSet.java: Line 40: public class BinaryHashSet { > Why is this class not shown in the checked out codebase?? Sorry, this is not a part of index-only plan change. This is a part of full-text change. I have removed this. -- To view, visit https://asterix-gerrit.ics.uci.edu/744 To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ifa02c13d4fddd880e1ee9e85eef6577301fb4560 Gerrit-PatchSet: 11 Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Owner: Taewoo Kim <wangs...@yahoo.com> Gerrit-Reviewer: Jenkins <jenk...@fulliautomatix.ics.uci.edu> Gerrit-Reviewer: Taewoo Kim <wangs...@yahoo.com> Gerrit-Reviewer: Till Westmann <ti...@apache.org> Gerrit-Reviewer: Yingyi Bu <buyin...@gmail.com> Gerrit-Reviewer: Young-Seok Kim <kiss...@gmail.com> Gerrit-HasComments: Yes
