Mike Percy 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: (8 comments) Did a cursory review pass, nice to see the improvements, looking good to me http://gerrit.cloudera.org:8080/#/c/10983/15/src/kudu/cfile/cfile_reader.cc File src/kudu/cfile/cfile_reader.cc: http://gerrit.cloudera.org:8080/#/c/10983/15/src/kudu/cfile/cfile_reader.cc@790 PS15, Line 790: Todo nit: s/Todo/TODO/ http://gerrit.cloudera.org:8080/#/c/10983/15/src/kudu/cfile/cfile_reader.cc@793 PS15, Line 793: 16*1024 Why did we go from using a constant back to using a magic number? http://wiki.c2.com/?MagicNumber We should use the gflag we are using in the other file here as well. http://gerrit.cloudera.org:8080/#/c/10983/15/src/kudu/cfile/cfile_reader.cc@795 PS15, Line 795: DCHECK(!prepared_blocks_.empty()); : if (prepared_blocks_.empty()) { : return Status::NotFound("blocks not found"); : } This has a code smell. https://martinfowler.com/bliki/CodeSmell.html In debug mode, we make it impossible to hit this condition, yet in release mode, we handle it. That means it is impossible to have test coverage of this situation, which is bad. We should pick one or the other: either this is impossible, i.e. a code error, in which case just use a CHECK, or it's possible, and just handle it by returning Status::NotFound. http://gerrit.cloudera.org:8080/#/c/10983/15/src/kudu/tablet/cfile_set.h File src/kudu/tablet/cfile_set.h: http://gerrit.cloudera.org:8080/#/c/10983/15/src/kudu/tablet/cfile_set.h@175 PS15, Line 175: // Due to the caveat(see below) listed for SkipToNextScan(size_t *remaining), : // this class should not reference a separate "client" class that uses key_iter->cur_val. I find this part of the comment confusing and I think we should just remove it http://gerrit.cloudera.org:8080/#/c/10983/15/src/kudu/tablet/cfile_set.h@248 PS15, Line 248: Caveat: I don't think we need to specifically call this out as a caveat http://gerrit.cloudera.org:8080/#/c/10983/15/src/kudu/tablet/cfile_set.h@249 PS15, Line 249: key_iter->cur_val_ How about just key_iter_? Is that sufficient? Talking about a private member of a different class breaks encapsulation for that class. http://gerrit.cloudera.org:8080/#/c/10983/15/src/kudu/tablet/cfile_set.h@362 PS15, Line 362: skip_scan_num_seeks_cutoff missing underscore suffix on member variable http://gerrit.cloudera.org:8080/#/c/10983/14/src/kudu/tablet/cfile_set.cc File src/kudu/tablet/cfile_set.cc: http://gerrit.cloudera.org:8080/#/c/10983/14/src/kudu/tablet/cfile_set.cc@770 PS14, Line 770: reak; > How does this happen? Could you elaborate a bit? What does seeking backward We effectively have two pointers we are tracking with skip-scan: the primary key (PK) value index (ad-hoc index) search pointer, which gives us a forward index of value to position, and the scan pointer, which is cur_idx_, representing an offset one more than the last row that we actually scanned. In this case, we searched the PK and came up with a position lower than our scan offset, which means even though we tried to search forward relative to our previous PK lookups, we ended up landing at an offset that we'd already scanned. The awkward reason we have to deal with this possibility is because we don't have a reverse PK index from offset (position) to PK (value), otherwise we'd . Maybe we should try to explain the above somewhere in a comment, although I'm not sure of the best place to discuss it. Maybe near the top of this method in the .cc file around line 665? -- 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: Wed, 15 Aug 2018 17:27:12 +0000 Gerrit-HasComments: Yes
