Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/10983 )
Change subject: KUDU-1291. Efficiently support predicates on non-prefix key components ...................................................................... Patch Set 15: (4 comments) A bit more feedback for the test. http://gerrit.cloudera.org:8080/#/c/10983/15/src/kudu/tablet/index_skipscan-test.cc File src/kudu/tablet/index_skipscan-test.cc: PS15: We discussed that offline, but just to re-iterate: It would be nice to add a few scenario where cardinality of predicate-matching rows is close to the number of all rows inserted, where number of inserted rows would be in order of 10^3. The idea is to exercise SkipToNextScan() method's logic more often. http://gerrit.cloudera.org:8080/#/c/10983/15/src/kudu/tablet/index_skipscan-test.cc@18 PS15, Line 18: #include <stddef.h> nit: please use c++ style include: #include <cstddef> http://gerrit.cloudera.org:8080/#/c/10983/15/src/kudu/tablet/index_skipscan-test.cc@106 PS15, Line 106: s.IsAlreadyPresent What if num_matching was incremented, but the IsAlreadyPresent() was reported on an attempt to insert a row? Maybe, num_matching should be incremented only in case if row was inserted? http://gerrit.cloudera.org:8080/#/c/10983/15/src/kudu/tablet/index_skipscan-test.cc@129 PS15, Line 129: { nit: here and elsewhere under this switch -- remove those braces, they are not needed. -- To view, visit http://gerrit.cloudera.org:8080/10983 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I230cd5a288e28ace796b352a603e0d1bcc1e4e0f Gerrit-Change-Number: 10983 Gerrit-PatchSet: 15 Gerrit-Owner: Anupama Gupta <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Anupama Gupta <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Thu, 16 Aug 2018 00:03:33 +0000 Gerrit-HasComments: Yes
