Taewoo Kim has posted comments on this change. Change subject: [ASTERIXDB-1972][COMP][RT][TX] index-only plan ......................................................................
Patch Set 41: (25 comments) @Dmitry: thanks for the detailed comments. I think I have addressed all of your comments. https://asterix-gerrit.ics.uci.edu/#/c/1866/43/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 1059: if (dataverseName == null || datasetName == null) { > Can this happen (dataverseName/datasetName is null) or it would be a bug in I think we can remove this check. Removed. Line 1064: if (idxUsedInUnnestMap == null) { > this 'if' block is equivalent to just returning 'idxUsedInUnnestMap' Fixed. https://asterix-gerrit.ics.uci.edu/#/c/1866/43/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 2236: // This variable is a direct field from the probe tree so that we can find the field type. > ARecordType.getFieldType() does not throw exception, so what are we catchin You are correct. Removed. https://asterix-gerrit.ics.uci.edu/#/c/1866/43/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/am/IntroduceSelectAccessMethodRule.java File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/am/IntroduceSelectAccessMethodRule.java: Line 67: * NEW: > We need to cleanup this javadoc, so there's no longer NEW/OLD there, but a Done Line 383: if (chosenIndexes.size() > 1) { > This code is a bit confusing now. There used to be a special handling insid Makes sense. Done. Line 387: // Since only a single (secondary) index is chosen, checks whether this plan is an index-only plan. > This comment is confusing. It says "single (secondary) index is chosen" (so Fixed. Line 393: // If the chosen index is the primary index, adds variable:fieldname to subTree.fieldNames. > This block is not going to be executed if the primary index was chosen alon That's correct. I have fixed the issue so that we don't need to update the field-name for the primary index. https://asterix-gerrit.ics.uci.edu/#/c/1866/43/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 88: private Map<LogicalVariable, List<String>> varsTofieldNameMap = new HashMap<>(); > rename to 'varsToFieldNameMap' (capital 'F'). Also rename corresponding get Done Line 115: if (subTreeOp.getOperatorTag() == LogicalOperatorTag.LIMIT) { > Previously this code was looking for (select)? <-- (assign | unnest)* <-- ( As we talked, it's a legacy of implementing "limit pushdown". Removed. Line 133: if (subTreeOp.getOperatorTag() == LogicalOperatorTag.UNIONALL) { > When would this code encounter an index-only plan? Removed, too. https://asterix-gerrit.ics.uci.edu/#/c/1866/43/asterixdb/asterix-common/src/main/java/org/apache/asterix/common/exceptions/CompilationException.java File asterixdb/asterix-common/src/main/java/org/apache/asterix/common/exceptions/CompilationException.java: Line 37: public CompilationException(int errorCode, Throwable cause) { > I could find any usages of this new constructor. Looks like it's not necess Still, it's a legacy code that I generated and removed the usage at a time. Removed. https://asterix-gerrit.ics.uci.edu/#/c/1866/43/asterixdb/asterix-common/src/main/java/org/apache/asterix/common/exceptions/ErrorCode.java File asterixdb/asterix-common/src/main/java/org/apache/asterix/common/exceptions/ErrorCode.java: Line 146: public static final int NO_RETAIN_INPUT_IN_LEFT_OUTER_JOIN = 1067; > NO_RETAIN_INPUT_IN_LEFT_OUTER_JOIN is not used. Let's remove it Done https://asterix-gerrit.ics.uci.edu/#/c/1866/43/asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/declared/MetadataProvider.java File asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/declared/MetadataProvider.java: Line 1606: private static byte[] computeByteArrayForIndexOnlyPlan(int resultValue) throws AlgebricksException { > This is a generic method that serializes integer value into byte array. Can Good! Done. https://asterix-gerrit.ics.uci.edu/#/c/1866/43/asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/entities/Dataset.java File asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/entities/Dataset.java: Line 578: case BTREE: > use index.resourceType() here instead of this switch statement. It does exa Thanks! Done. https://asterix-gerrit.ics.uci.edu/#/c/1866/43/asterixdb/asterix-om/src/main/java/org/apache/asterix/om/types/hierachy/DoubleToFloatTypeConvertComputer.java File asterixdb/asterix-om/src/main/java/org/apache/asterix/om/types/hierachy/DoubleToFloatTypeConvertComputer.java: Line 59: public IAObject convertType(IAObject sourceObject, TypeCastingMathFunctionType mathFunction) > mathFunction is not used in the method. Seems like we should either only ex Done https://asterix-gerrit.ics.uci.edu/#/c/1866/43/hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/operators/logical/AbstractUnnestMapOperator.java File hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/operators/logical/AbstractUnnestMapOperator.java: Line 55: return variables.subList(0, variables.size() - 2); > This is minor, but can we simplify it a bit, to make clear what's going on. Much cleaner! Done. Line 142: } > Can we add a comment that this variable must be defined as the 3rd variable Done https://asterix-gerrit.ics.uci.edu/#/c/1866/43/hyracks-fullstack/algebricks/algebricks-rewriter/src/main/java/org/apache/hyracks/algebricks/rewriter/rules/IntroduceProjectsRule.java File hyracks-fullstack/algebricks/algebricks-rewriter/src/main/java/org/apache/hyracks/algebricks/rewriter/rules/IntroduceProjectsRule.java: Line 199: eliminateProject = canEliminateProjectBelowUnion(unionOp, projectOp, parentInputIndex); > parentInputIndex will be incorrect if we have UNIONALL-> EXCHANGE -> PROJEC As we discussed, I removed my "adding projects" logic in the index transformation part and let "InsertProjectBeforeUnionRule" handle that. Also, this part has been removed. https://asterix-gerrit.ics.uci.edu/#/c/1866/41/hyracks-fullstack/hyracks/hyracks-storage-am-btree/src/main/java/org/apache/hyracks/storage/am/btree/dataflow/BTreeSearchOperatorDescriptor.java File hyracks-fullstack/hyracks/hyracks-storage-am-btree/src/main/java/org/apache/hyracks/storage/am/btree/dataflow/BTreeSearchOperatorDescriptor.java: Line 48: protected boolean useOpCallbackProceedResult; > 'use' can have different meanings, so might be confusing. May be rename 'us Done https://asterix-gerrit.ics.uci.edu/#/c/1866/41/hyracks-fullstack/hyracks/hyracks-storage-am-common/src/main/java/org/apache/hyracks/storage/am/common/dataflow/IndexSearchOperatorNodePushable.java File hyracks-fullstack/hyracks/hyracks-storage-am-common/src/main/java/org/apache/hyracks/storage/am/common/dataflow/IndexSearchOperatorNodePushable.java: Line 171: buildMissingTuple(numIndexFilterFields, nonFilterTupleBuild, nonMatchWriter); > There might be a problem here. buildMissingTuple() now sets last field to s That issue is dealt within the bulidMissingTuple() method itself. Line 207: if (appendIndexFilter) { > I wonder whether we can handle useOpCallbackProceedResult here in the same What a good idea it is. Done! https://asterix-gerrit.ics.uci.edu/#/c/1866/41/hyracks-fullstack/hyracks/hyracks-storage-am-lsm-btree/src/main/java/org/apache/hyracks/storage/am/lsm/btree/impls/LSMBTreeRangeSearchCursor.java File hyracks-fullstack/hyracks/hyracks-storage-am-lsm-btree/src/main/java/org/apache/hyracks/storage/am/lsm/btree/impls/LSMBTreeRangeSearchCursor.java: Line 56: private byte[] firstReturnValueArrayForProccedResult = new byte[5]; > why create new arrays here? both fields would be assigned in the constructo Done Line 132: resultOfSearchCallBackProceed = searchCallback.proceed(queueHead.getTuple()); > is it always the case that resultOfSearchCallBackProceed is update on each Yes. The only case when the loop exits is that when we find a valid tuple. And resultOfSearchCallBackProceed has the result for that tuple. All other cases (e.g., finding a deleted tuple), the loop continues and resultOfSearchCallBackProceed is reassigned for each tuple. Line 393: firstReturnValueArrayForProccedResult = opCtx.getFirstValueForUseProceedResult(); > firstReturnValueArrayForProccedResult and secondReturnValueArrayForProccedR Fixed. https://asterix-gerrit.ics.uci.edu/#/c/1866/41/hyracks-fullstack/hyracks/hyracks-storage-am-lsm-rtree/src/main/java/org/apache/hyracks/storage/am/lsm/rtree/impls/LSMRTreeSearchCursor.java File hyracks-fullstack/hyracks/hyracks-storage-am-lsm-rtree/src/main/java/org/apache/hyracks/storage/am/lsm/rtree/impls/LSMRTreeSearchCursor.java: Line 159: tupleBuilderForProceedResult.addField(valuesForOperationCallbackProceedReturnResult, 5, 5); > valuesForOperationCallbackProceedReturnResult is assigned to You are correct. I think this class is not used anymore and LSMRTreeWithAntiMatterTuplesSearchCursor is used instead. So, I stopped updating this class for a while. Anyway, I have corrected the issue. -- To view, visit https://asterix-gerrit.ics.uci.edu/1866 To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ifd5c9ab1cf2e4bedb7d8db582441919875e74d51 Gerrit-PatchSet: 41 Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Owner: Taewoo Kim <[email protected]> Gerrit-Reviewer: Anon. E. Moose #1000171 Gerrit-Reviewer: Dmitry Lychagin <[email protected]> Gerrit-Reviewer: Ildar Absalyamov <[email protected]> Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-Reviewer: Taewoo Kim <[email protected]> Gerrit-Reviewer: Till Westmann <[email protected]> Gerrit-Reviewer: Yingyi Bu <[email protected]> Gerrit-HasComments: Yes
