Murtadha Hubail has posted comments on this change. Change subject: [STO][IDX] Eliminated excess antimatter in LSMBTree ......................................................................
Patch Set 10: (9 comments) A general comment: many downcasts were introduced due to the newly added BTree specific classes. Can we get rid of these downcasts by making factories return BTree* type? https://asterix-gerrit.ics.uci.edu/#/c/1538/10/hyracks-fullstack/hyracks/hyracks-storage-am-btree/src/main/java/org/apache/hyracks/storage/am/btree/impls/FieldPrefixTupleReference.java File hyracks-fullstack/hyracks/hyracks-storage-am-btree/src/main/java/org/apache/hyracks/storage/am/btree/impls/FieldPrefixTupleReference.java: PS10, Line 26: FieldPrefixTupleReference If this is used only for BTree, rename it to reflect this. https://asterix-gerrit.ics.uci.edu/#/c/1538/10/hyracks-fullstack/hyracks/hyracks-storage-am-btree/src/main/java/org/apache/hyracks/storage/am/btree/tuples/BTreeTypeAwareTupleWriter.java File hyracks-fullstack/hyracks/hyracks-storage-am-btree/src/main/java/org/apache/hyracks/storage/am/btree/tuples/BTreeTypeAwareTupleWriter.java: PS10, Line 36: ITreeIndexTupleReference Change the return type here to BTreeTypeAwareTupleWriter to avoid the downcasts. https://asterix-gerrit.ics.uci.edu/#/c/1538/10/hyracks-fullstack/hyracks/hyracks-storage-am-common/src/main/java/org/apache/hyracks/storage/am/common/api/IBTreeIndexTupleReference.java File hyracks-fullstack/hyracks/hyracks-storage-am-common/src/main/java/org/apache/hyracks/storage/am/common/api/IBTreeIndexTupleReference.java: PS10, Line 22: interface Add javadocs on each method https://asterix-gerrit.ics.uci.edu/#/c/1538/10/hyracks-fullstack/hyracks/hyracks-storage-am-common/src/main/java/org/apache/hyracks/storage/am/common/api/IBTreeIndexTupleWriter.java File hyracks-fullstack/hyracks/hyracks-storage-am-common/src/main/java/org/apache/hyracks/storage/am/common/api/IBTreeIndexTupleWriter.java: PS10, Line 22: interface javadocs https://asterix-gerrit.ics.uci.edu/#/c/1538/10/hyracks-fullstack/hyracks/hyracks-storage-am-lsm-btree/src/main/java/org/apache/hyracks/storage/am/lsm/btree/impls/LSMBTree.java File hyracks-fullstack/hyracks/hyracks-storage-am-lsm-btree/src/main/java/org/apache/hyracks/storage/am/lsm/btree/impls/LSMBTree.java: PS10, Line 331: if (!isPrimaryIndex() && ((LSMBTreeTupleReference) scanCursor.getTuple()).isUpdated()) { : continue; : } Can this be done in a better way where we pass false for updateAware in the primary case? https://asterix-gerrit.ics.uci.edu/#/c/1538/10/hyracks-fullstack/hyracks/hyracks-storage-am-lsm-btree/src/main/java/org/apache/hyracks/storage/am/lsm/btree/tuples/LSMBTreeTupleWriter.java File hyracks-fullstack/hyracks/hyracks-storage-am-lsm-btree/src/main/java/org/apache/hyracks/storage/am/lsm/btree/tuples/LSMBTreeTupleWriter.java: PS10, Line 61: double numBits = numFields + 1.0; : if (updateAware) { : numBits += 1.0; : } : return (int) Math.ceil(numBits / 8.0); This is repeated in multiple places. It would be nice if it could be moved to a util class PS10, Line 100: (1 << 6) This should be a constant. https://asterix-gerrit.ics.uci.edu/#/c/1538/10/hyracks-fullstack/hyracks/hyracks-storage-am-lsm-btree/src/main/java/org/apache/hyracks/storage/am/lsm/btree/utils/LSMBTreeUtil.java File hyracks-fullstack/hyracks/hyracks-storage-am-lsm-btree/src/main/java/org/apache/hyracks/storage/am/lsm/btree/utils/LSMBTreeUtil.java: PS10, Line 67: updateInPlaceAware Please use consistent naming for this flag. https://asterix-gerrit.ics.uci.edu/#/c/1538/10/hyracks-fullstack/hyracks/hyracks-tests/hyracks-storage-am-lsm-btree-test/src/test/java/org/apache/hyracks/storage/am/lsm/btree/util/LSMBTreeTestContext.java File hyracks-fullstack/hyracks/hyracks-tests/hyracks-storage-am-lsm-btree-test/src/test/java/org/apache/hyracks/storage/am/lsm/btree/util/LSMBTreeTestContext.java: PS10, Line 77: isPrimary I don't think adding a parameter called isPrimary in Hyracks storage level is a good idea. I know that there is a method called isPrimaryIndex in LSMBTree but that should be renamed. -- To view, visit https://asterix-gerrit.ics.uci.edu/1538 To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-MessageType: comment Gerrit-Change-Id: I12a67eff8431b52d1f9051b793a5a64b15c009e9 Gerrit-PatchSet: 10 Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Owner: Ildar Absalyamov <[email protected]> Gerrit-Reviewer: Ian Maxon <[email protected]> Gerrit-Reviewer: Ildar Absalyamov <[email protected]> Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-Reviewer: Luo Chen <[email protected]> Gerrit-Reviewer: Murtadha Hubail <[email protected]> Gerrit-Reviewer: Till Westmann <[email protected]> Gerrit-Reviewer: abdullah alamoudi <[email protected]> Gerrit-HasComments: Yes
