Young-Seok Kim has posted comments on this change. Change subject: Index-only plan ......................................................................
Patch Set 7: (19 comments) Please address comments https://asterix-gerrit.ics.uci.edu/#/c/744/7/asterix-algebra/src/main/java/org/apache/asterix/algebra/operators/physical/InvertedIndexPOperator.java File asterix-algebra/src/main/java/org/apache/asterix/algebra/operators/physical/InvertedIndexPOperator.java: Line 290: // We deduct 1 because the last field will be the result of searchCallback.proceed() 1) If I remember correctly, the index-only plan for the inverted index is not possible due to the instantlock, right? 2) Even though the index-only plan is not possible for now, it's good to have more comments here to explain the layout of the fields in the inverted index. For example, an entry of inverted index consists of [SK, PK] and when the index only plan is enabled, where the additional field for the trylock result is appended. With this explanation, the following -1 and primaryKeyFieldsInSecondaryIndex array variable can be more clearly understandable. The same comments for other indexes. Line 301: primaryKeyFieldsInSecondaryIndex, txnSubsystemProvider, ResourceType.LSM_BTREE, should be LSM_INVERTED_INDEX? Line 320: valuesForIndexOnlyPlan = castBuffer.getByteArray(); This byte array is only required for index-only plan case. So, code creating the byte array should be executed only for index-only plan by moving the code into above else clause. If it's not index-only plan the byte array could be null. The same comments for other indexes. https://asterix-gerrit.ics.uci.edu/#/c/744/7/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/am/AbstractIntroduceAccessMethodRule.java File asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/am/AbstractIntroduceAccessMethodRule.java: Line 669: if (subTree.dataSourceType != DataSourceType.COLLECTION_SCAN) { What's the reason for ignoring the COLLECTION_SCAN? Line 1036: UnnestMapOperator newUnnestMapOp = replaceExpressionOfUnnestMapOperator(jobGenParams, Why can't the existing unnestMapOp be reused? Why is a new unnestMapOp required? Line 1057: UnnestMapOperator newUnnestMapOp = replaceExpressionOfUnnestMapOperator(jobGenParams, Why can't the existing unnestMapOp be reused? Why is a new unnestMapOp required? Line 1086: return false; let's remove this unreachable code. https://asterix-gerrit.ics.uci.edu/#/c/744/7/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/am/AccessMethodUtils.java File asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/am/AccessMethodUtils.java: Line 745: // For the composite index, a secondary index search generates a superset of the results. Why does the composite index generate a superset of the results? Line 1625: // context.computeAndSetTypeEnvironmentForOperator(lastAssignBeforeTopOp); Is the above code not sufficient? Is the following for clause needed? Line 1760: // unionAllOp.getInputs().add(new MutableObject<ILogicalOperator>(currentTopOpInRightPath)); better be removing the above two lines if they are not used. Also, if the following three lines are needed for the debugging purpose, introduce DEBUG flag as private static final boolean and set it to false. Then put the 3 lines into if clause as follows: if (DEBUG) { //3 lines here. } https://asterix-gerrit.ics.uci.edu/#/c/744/7/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/am/BTreeAccessMethod.java File asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/am/BTreeAccessMethod.java: Line 656: Why do we do this? why can't we remove false positives in this composite key case? Line 657: // If the select condition contains mixed open/closed intervals on multiple keys, then we make all intervals closed to obtain a superset of answers and leave the original selection in place. typo:primaryIndexPostProccessingIsNeeded -> primaryIndexPostProcessingIsNeeded https://asterix-gerrit.ics.uci.edu/#/c/744/7/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/am/IntroduceSelectAccessMethodRule.java File asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/am/IntroduceSelectAccessMethodRule.java: Line 176: if (op.getOperatorTag() != LogicalOperatorTag.SINK) { Can we make a private function isRootOp(op) to replace these two if clauses? This will hide the details such as which operators can be root operators. Line 293: protected boolean checkAndApplyTheRule(Mutable<ILogicalOperator> opRef, int nthChild) throws AlgebricksException { nthChild variable is not used. Let's remove it. Line 311: if (subTree.selectRefs == null || (subTree.selectRefs != null && subTree.selectRefs.size() == 0)) { In which AQL query, does the above if condition become false? If there is such an AQL query, it's good to make the query as a test case. If such a query is covered by a test case already, please explain the case here and mention the tc in comments here. https://asterix-gerrit.ics.uci.edu/#/c/744/7/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/am/OptimizableOperatorSubTree.java File asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/am/OptimizableOperatorSubTree.java: Line 189: if (subTreeOp.getInputs().size() < 1 || subTreeOp.getInputs() == null) { should be as follows to avoid null pointer exception: if (subTreeOp.getInputs() == null || subTreeOp.getInputs().size() < 1 ) Line 278: private boolean initFromIndexOnlyOrReducingTheNumberOfTheSelectVerficationPlan( This method should be split into two methods, one for indexOnly and one for reducingSelectVerfication. It seems no reason to put these two logics in a same method. Line 550: // Control-flow should not reach here. remove unreachable code. https://asterix-gerrit.ics.uci.edu/#/c/744/7/asterix-metadata/src/main/java/org/apache/asterix/metadata/declared/AqlMetadataProvider.java File asterix-metadata/src/main/java/org/apache/asterix/metadata/declared/AqlMetadataProvider.java: Line 740: AqlMetadataImplConfig aqlMetadataImplConfig = (AqlMetadataImplConfig) implConfig; impleConfig is created and set when IntroduceInstantLockSearchCallbackRule is fired. This rule is useless since we decided to support read-committed isolation level. From the rulecollection, the rule should be commented out. So, regardless of whether a dataset is seen more than once or not, we will always use instantLock. Therefore, the code from line760~783 should be changed to use only instant lock. No dataset level lock! -- 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: 7 Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Owner: Taewoo Kim <[email protected]> Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-Reviewer: Till Westmann <[email protected]> Gerrit-Reviewer: Yingyi Bu <[email protected]> Gerrit-Reviewer: Young-Seok Kim <[email protected]> Gerrit-HasComments: Yes
