asterixdb-notifications mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dmitry Lychagin (Code Review)" <do-not-re...@asterixdb.incubator.apache.org>
Subject Change in asterixdb[master]: [ASTERIXDB-1972][COMP][RT][TX] index-only plan
Date Tue, 23 Jan 2018 02:26:34 GMT
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 <wangsaeu@gmail.com>
Gerrit-Reviewer: Anon. E. Moose #1000171
Gerrit-Reviewer: Dmitry Lychagin <dmitry.lychagin@couchbase.com>
Gerrit-Reviewer: Ildar Absalyamov <ildar.absalyamov@gmail.com>
Gerrit-Reviewer: Jenkins <jenkins@fulliautomatix.ics.uci.edu>
Gerrit-Reviewer: Taewoo Kim <wangsaeu@gmail.com>
Gerrit-Reviewer: Till Westmann <tillw@apache.org>
Gerrit-Reviewer: Yingyi Bu <buyingyi@gmail.com>
Gerrit-HasComments: Yes

Mime
View raw message