Dmitry Lychagin has posted comments on this change. Change subject: [ASTERIXDB-1972][COMP][RT][TX] index-only plan ......................................................................
Patch Set 41: (9 comments) 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 'useOpCallbackProceedResult' to 'appendCallbackProceedResult' to show that it's about adding something to the output? also make this field final. 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 secondValueForUseOpCallbackProceedResult if useOpCallbackProceedResult is true. is this ok for nonFilterTupleBuild created here? Line 207: if (appendIndexFilter) { I wonder whether we can handle useOpCallbackProceedResult here in the same manner appendIndexFilter is handled? So instead of pushing useOpCallbackProceedResult, firstValueForUseOpCallbackProceedResult, secondValueForUseOpCallbackProceedResult to cursors and having each of them write it to the output, we expose this value through the cursor api (like getFileterMinTuple()/ getFilterMaxTuple()), obtain it here and write to the output if necessary. If we can do this then we may be able to remove setUseOpCallbackProceedResult/getUseOpCallbackProceedResult/etc methods from ILSMIndexOperationContext Line 328: // we need to write down the result of searchOperationCallback.proceed(). need to change the comment. here we're not writing "result of searchOperationCallback.proceed()", but rather the mapping value for 'true' (secondValue...). So the comment should say that Line 330: nullTuple.addField(secondValueForUseOpCallbackProceedResult, 0, 5); use secondValueForUseOpCallbackProceedResult.length instead of '5' here and everywhere else. 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 constructor anyway Line 132: resultOfSearchCallBackProceed = searchCallback.proceed(queueHead.getTuple()); is it always the case that resultOfSearchCallBackProceed is update on each invocation of checkPriorityQueue()? Could it be the case the while loop exits without updating resultOfSearchCallBackProceed? so next() would output its previous value? Line 393: firstReturnValueArrayForProccedResult = opCtx.getFirstValueForUseProceedResult(); firstReturnValueArrayForProccedResult and secondReturnValueArrayForProccedResult are assigned in the constructor and never changed. so reassign them here? 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 opCtx.getFirstValueForUseProceedResult() in the constructor and we're getting its bytes starting from offset 5. are you sure this is correct? why opCtx.getSecondValueForUseProceedResult() is not used in this class? -- 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
