Young-Seok Kim has posted comments on this change. Change subject: Add Support for Upsert Operation ......................................................................
Patch Set 6: (9 comments) Please address comments. https://asterix-gerrit.ics.uci.edu/#/c/476/6/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/operators/logical/ReplicateOperator.java File algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/operators/logical/ReplicateOperator.java: Line 76: return true; What's the meaning of Map in isMap()? https://asterix-gerrit.ics.uci.edu/#/c/476/6/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/operators/logical/SinkOperator.java File algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/operators/logical/SinkOperator.java: Line 93: } Why does sink operator have any variable, now? https://asterix-gerrit.ics.uci.edu/#/c/476/6/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/operators/logical/visitors/VariableUtilities.java File algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/operators/logical/visitors/VariableUtilities.java: Line 47: */ Can we explain here the definition of the following terms? UsedVariables: variables used in the logicalOperator, op. ProducedVariables: variables produced in the logicalOperator, op LiveVariables: variables coming from the input/child operator of the logicalOperator, op. https://asterix-gerrit.ics.uci.edu/#/c/476/6/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/operators/physical/InsertDeleteUpsertPOperator.java File algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/operators/physical/InsertDeleteUpsertPOperator.java: Line 116: } better throw exception by adding else { } here. https://asterix-gerrit.ics.uci.edu/#/c/476/6/hyracks/hyracks-storage-am-btree/src/main/java/org/apache/hyracks/storage/am/btree/impls/RangePredicate.java File hyracks/hyracks-storage-am-btree/src/main/java/org/apache/hyracks/storage/am/btree/impls/RangePredicate.java: Line 141: } Can we just name as getLowKey()? I know this key is only used for prior to primary index lookup. Unless we enforce the caller to call this method only when the two keys are the same, adding "This will only be called when low key and high key are the same." will confuse the code reader. https://asterix-gerrit.ics.uci.edu/#/c/476/6/hyracks/hyracks-storage-am-common/src/main/java/org/apache/hyracks/storage/am/common/api/ISearchPredicate.java File hyracks/hyracks-storage-am-common/src/main/java/org/apache/hyracks/storage/am/common/api/ISearchPredicate.java: Line 37: public ITupleReference getSearchKey(); again, can we rename getLowKey()? Can we change the sentence "This method will only be called with point search predicates." as follows? This method will only be called with point search predicates that only happen in primary index. https://asterix-gerrit.ics.uci.edu/#/c/476/6/hyracks/hyracks-storage-am-lsm-btree/src/main/java/org/apache/hyracks/storage/am/lsm/btree/impls/LSMBTreeOpContext.java File hyracks/hyracks-storage-am-lsm-btree/src/main/java/org/apache/hyracks/storage/am/lsm/btree/impls/LSMBTreeOpContext.java: Line 219: @Override Where are the following two methods used? https://asterix-gerrit.ics.uci.edu/#/c/476/6/hyracks/hyracks-storage-am-lsm-common/src/main/java/org/apache/hyracks/storage/am/lsm/common/api/ILSMIndexOperationContext.java File hyracks/hyracks-storage-am-lsm-common/src/main/java/org/apache/hyracks/storage/am/lsm/common/api/ILSMIndexOperationContext.java: Line 44: Where are the following two methods used? https://asterix-gerrit.ics.uci.edu/#/c/476/6/hyracks/hyracks-storage-am-lsm-invertedindex/src/main/java/org/apache/hyracks/storage/am/lsm/invertedindex/search/InvertedIndexSearchPredicate.java File hyracks/hyracks-storage-am-lsm-invertedindex/src/main/java/org/apache/hyracks/storage/am/lsm/invertedindex/search/InvertedIndexSearchPredicate.java: Line 86: return queryTuple; Probably, better be throwing exception here since this method is only legal to be called by primary index (which is not inverted index) -- To view, visit https://asterix-gerrit.ics.uci.edu/476 To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-MessageType: comment Gerrit-Change-Id: I2705f43b6e6d187ee29b9ba5a7946d422990022a Gerrit-PatchSet: 6 Gerrit-Project: hyracks Gerrit-Branch: master Gerrit-Owner: abdullah alamoudi <bamou...@gmail.com> Gerrit-Reviewer: Jenkins <jenk...@fulliautomatix.ics.uci.edu> Gerrit-Reviewer: Taewoo Kim <wangs...@gmail.com> Gerrit-Reviewer: Till Westmann <ti...@apache.org> Gerrit-Reviewer: Young-Seok Kim <kiss...@gmail.com> Gerrit-Reviewer: abdullah alamoudi <bamou...@gmail.com> Gerrit-HasComments: Yes
