Dan Burkert has posted comments on this change. Change subject: Tightening ScanSpec primary bounds when range predicate exists ......................................................................
Patch Set 2: (7 comments) http://gerrit.cloudera.org:8080/#/c/5360/2/src/kudu/common/key_util.cc File src/kudu/common/key_util.cc: PS2, Line 146: : Keep this, but change it to stopping after the first missing predicate or range predicate which can't be transformed to an inclusive upper bound. Line 155: bool DecrementStringCell(void* cell_ptr, Arena* arena) { I don't think it's necessary to do any copying of the data here - you should just be able to make the Slice shorter by 1, but still pointing at the same data. As a result you won't need to take an Arena and the signature can be simplified. PS2, Line 209: need_increase I think it makes sense to think about this in terms of exclusive vs. inclusive upper bound, so maybe rename this variable 'is_inclusive_bound'. PS2, Line 226: align with 'row' above. PS2, Line 251: if range upper bound decrease fail, no need to increase key change this to If the upper bound is inclusive, increment it to become exclusive. http://gerrit.cloudera.org:8080/#/c/5360/2/src/kudu/common/key_util.h File src/kudu/common/key_util.h: Line 64: bool DecrementCell(const ColumnSchema& col, void* cell_ptr, Arena* arena); Please add unit tests to key_util-test.cc for this method. http://gerrit.cloudera.org:8080/#/c/5360/2/src/kudu/common/scan_spec-test.cc File src/kudu/common/scan_spec-test.cc: Line 184: } Could you add a few cases for LT predicates on non-decrementable values of type int and string? -- To view, visit http://gerrit.cloudera.org:8080/5360 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I931a617605434700352d285fc2039033cf9eb07e Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Haijie Hong <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-HasComments: Yes
