asterixdb-notifications mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Murtadha Hubail (Code Review)" <do-not-re...@asterixdb.incubator.apache.org>
Subject Change in asterixdb[master]: [STO][IDX] Eliminated excess antimatter in LSMBTree
Date Fri, 25 Aug 2017 19:58:32 GMT
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 <ildar.absalyamov@gmail.com>
Gerrit-Reviewer: Ian Maxon <imaxon@apache.org>
Gerrit-Reviewer: Ildar Absalyamov <ildar.absalyamov@gmail.com>
Gerrit-Reviewer: Jenkins <jenkins@fulliautomatix.ics.uci.edu>
Gerrit-Reviewer: Luo Chen <cluo8@uci.edu>
Gerrit-Reviewer: Murtadha Hubail <mhubail@apache.org>
Gerrit-Reviewer: Till Westmann <tillw@apache.org>
Gerrit-Reviewer: abdullah alamoudi <bamousaa@gmail.com>
Gerrit-HasComments: Yes

Mime
View raw message