Taewoo Kim has posted comments on this change. Change subject: Hyracks Unit tests for LSM Filters ......................................................................
Patch Set 9: (7 comments) One Q: is there a test for "filtered = true" case? All I saw was "filtered = false". https://asterix-gerrit.ics.uci.edu/#/c/1704/9/hyracks-fullstack/hyracks/hyracks-storage-am-lsm-common/src/main/java/org/apache/hyracks/storage/am/lsm/common/impls/StubIOOperationCallback.java File hyracks-fullstack/hyracks/hyracks-storage-am-lsm-common/src/main/java/org/apache/hyracks/storage/am/lsm/common/impls/StubIOOperationCallback.java: PS9, Line 35: Can you move this to the line 28 and make it as a java doc style? https://asterix-gerrit.ics.uci.edu/#/c/1704/9/hyracks-fullstack/hyracks/hyracks-test-support/src/main/java/org/apache/hyracks/storage/am/btree/OrderedIndexTestDriver.java File hyracks-fullstack/hyracks/hyracks-test-support/src/main/java/org/apache/hyracks/storage/am/btree/OrderedIndexTestDriver.java: Line 43: BTreeLeafFrameType leafType, boolean filtered) throws Exception; > CRITICAL SonarQube violation: Can we handle this? Or, it doesn't have to be handled? https://asterix-gerrit.ics.uci.edu/#/c/1704/9/hyracks-fullstack/hyracks/hyracks-test-support/src/main/java/org/apache/hyracks/storage/am/btree/OrderedIndexTestUtils.java File hyracks-fullstack/hyracks/hyracks-test-support/src/main/java/org/apache/hyracks/storage/am/btree/OrderedIndexTestUtils.java: Line 302: if (filtered) { Please put a simple comment about this? What does this do? https://asterix-gerrit.ics.uci.edu/#/c/1704/9/hyracks-fullstack/hyracks/hyracks-test-support/src/main/java/org/apache/hyracks/storage/am/common/TreeIndexTestUtils.java File hyracks-fullstack/hyracks/hyracks-test-support/src/main/java/org/apache/hyracks/storage/am/common/TreeIndexTestUtils.java: Line 187: boolean filtered, Random rnd) throws Exception { > CRITICAL SonarQube violation: Here, too: Exception Line 210: if (filtered) { I have a feeling that the same code is repeated. Can we integrate them and put them in one place? Just a thought. https://asterix-gerrit.ics.uci.edu/#/c/1704/9/hyracks-fullstack/hyracks/hyracks-tests/hyracks-storage-am-lsm-btree-test/src/test/java/org/apache/hyracks/storage/am/lsm/btree/LSMBTreeFilterMergeTest.java File hyracks-fullstack/hyracks/hyracks-tests/hyracks-storage-am-lsm-btree-test/src/test/java/org/apache/hyracks/storage/am/lsm/btree/LSMBTreeFilterMergeTest.java: Line 32: Can we put java-doc style comments here? What does this test do? https://asterix-gerrit.ics.uci.edu/#/c/1704/9/hyracks-fullstack/hyracks/hyracks-tests/hyracks-storage-am-lsm-btree-test/src/test/java/org/apache/hyracks/storage/am/lsm/btree/LSMBTreeFilterMergeTestDriver.java File hyracks-fullstack/hyracks/hyracks-tests/hyracks-storage-am-lsm-btree-test/src/test/java/org/apache/hyracks/storage/am/lsm/btree/LSMBTreeFilterMergeTestDriver.java: Line 46: Also, can we put java-doc style comments here? -- To view, visit https://asterix-gerrit.ics.uci.edu/1704 To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ie0e79c0a25d5d6be727b9ebad841801024c00e94 Gerrit-PatchSet: 9 Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Owner: Ian Maxon <ima...@apache.org> Gerrit-Reviewer: Ian Maxon <ima...@apache.org> Gerrit-Reviewer: Jenkins <jenk...@fulliautomatix.ics.uci.edu> Gerrit-Reviewer: Jianfeng Jia <jianfeng....@gmail.com> Gerrit-Reviewer: Taewoo Kim <wangs...@gmail.com> Gerrit-Reviewer: abdullah alamoudi <bamou...@gmail.com> Gerrit-HasComments: Yes
