Young-Seok Kim has posted comments on this change. Change subject: Index-only plan ......................................................................
Patch Set 11: (21 comments) The following three issues are discovered. 1. R-tree index on rectangle type is not correctly dealt with for the corresponding index-only plan case This case should be added as a test case. 2. limit-push-down for primary index scan doesn't seem to work. 3. limit-push-down for primary-index search doesn't seem to work. Also, please address other 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. 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 issue? 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. 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., SecondaryIndexInstantSearchOperationCallback? 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 it OK for subTreeDataSourceVars to replace some of subTreePks below? Line 792: expr = (AbstractLogicalExpression) childFuncExpr.getArguments().get(0).getValue(); better be dealing with an else case explicitly, i.e., neither Assign nor unnest case. 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. doesSIdxSearchGenerateNoFalsePositiveResults? It seems the same? It would be good to explain the difference with example cases. Line 67: // Does the given index can cover all search predicates? It seems that the line 67 and 69 should be switched?? 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, so this parameter is not included in the 8 (NUM_PARAMS) parameters written and read." 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 trylockfailure case?) Line 1301: ((AbstractFunctionCallExpression) createOriginalSpatialObjectExpr).getArguments().addAll(expressions); Here, the createOriginalSpatialObjectExpr should be added to restoredSecondaryKeyFieldExprs and similarly assignRestoredSecondaryKeyFieldOp and restoredSecondaryKeyFieldVars should be set correctly. Index-only plan test cases including R-tree index on rectangle field should be added. 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 NOTAPPLICABLE Line 70: public enum canPreserveOrderCode { Please explain the meaning of each enum value, especially for CANBEBOTH and NOTAPPLICABLE 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? 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? 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 ? 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. https://asterix-gerrit.ics.uci.edu/#/c/744/11/hyracks-fullstack/hyracks/hyracks-storage-am-lsm-rtree/src/main/java/org/apache/hyracks/storage/am/lsm/rtree/impls/LSMRTreeWithAntiMatterTuplesSearchCursor.java File hyracks-fullstack/hyracks/hyracks-storage-am-lsm-rtree/src/main/java/org/apache/hyracks/storage/am/lsm/rtree/impls/LSMRTreeWithAntiMatterTuplesSearchCursor.java: Line 276: proceedFailCount += 1; remove this value or enable only if the code runs in debugging mode Line 282: proceedSuccessCount += 1; remove this value or enable only if the code runs in debugging mode Line 327: if (useProceedResult) { remove this value or enable only if the code runs in debugging mode 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?? -- 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
