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 10: (31 comments) Just did a nit pass. http://gerrit.cloudera.org:8080/#/c/10983/9/src/kudu/cfile/cfile_reader.h File src/kudu/cfile/cfile_reader.h: http://gerrit.cloudera.org:8080/#/c/10983/9/src/kudu/cfile/cfile_reader.h@344 PS9, Line 344: Status SeekAtOrAfter(const EncodedKey &encoded_key, : bool *exact_match, bool set_current_value = false); style nit: since 'exact_match' is an out-parameter, it should go last, so let's make the signature of this: Status SeekAtOrAfter(const EncodedKey &encoded_key, bool set_current_value = false, bool *exact_match = nullptr); Per https://google.github.io/styleguide/cppguide.html#Output_Parameters http://gerrit.cloudera.org:8080/#/c/10983/9/src/kudu/common/encoded_key.h File src/kudu/common/encoded_key.h: http://gerrit.cloudera.org:8080/#/c/10983/9/src/kudu/common/encoded_key.h@63 PS9, Line 63: num_columns document this parameter http://gerrit.cloudera.org:8080/#/c/10983/9/src/kudu/tablet/cfile_set.cc File src/kudu/tablet/cfile_set.cc: http://gerrit.cloudera.org:8080/#/c/10983/9/src/kudu/tablet/cfile_set.cc@76 PS9, Line 76: true Let's default this to false for the initial merge and do some testing to enable it. Add: TAG_FLAG(enable_skip_scan, experimental); http://gerrit.cloudera.org:8080/#/c/10983/9/src/kudu/tablet/cfile_set.cc@80 PS9, Line 80: Add: TAG_FLAG(skip_scan_short_circuit_loops, hidden); TAG_FLAG(skip_scan_short_circuit_loops, advanced); http://gerrit.cloudera.org:8080/#/c/10983/9/src/kudu/tablet/cfile_set.cc@405 PS9, Line 405: nit: extra space http://gerrit.cloudera.org:8080/#/c/10983/9/src/kudu/tablet/cfile_set.cc@424 PS9, Line 424: = nit: add spaces around your operators on this line, both the assignment = and the comparison != http://gerrit.cloudera.org:8080/#/c/10983/9/src/kudu/tablet/cfile_set.cc@430 PS9, Line 430: // Predicate exists on the first PK column. nit: move this comment inside the 'if' to right below this line http://gerrit.cloudera.org:8080/#/c/10983/9/src/kudu/tablet/cfile_set.cc@436 PS9, Line 436: (it->second) nit: unnecessary parentheses http://gerrit.cloudera.org:8080/#/c/10983/9/src/kudu/tablet/cfile_set.cc@437 PS9, Line 437: // Track the minimum column id. nit: put this comment on its own line http://gerrit.cloudera.org:8080/#/c/10983/9/src/kudu/tablet/cfile_set.cc@444 PS9, Line 444: (it->second) nit: unnecessary parentheses http://gerrit.cloudera.org:8080/#/c/10983/9/src/kudu/tablet/cfile_set.cc@522 PS9, Line 522: nit: remove so much extra horizontal white space on this line http://gerrit.cloudera.org:8080/#/c/10983/9/src/kudu/tablet/cfile_set.cc@531 PS9, Line 531: or s/or/which we consider to be/ http://gerrit.cloudera.org:8080/#/c/10983/9/src/kudu/tablet/cfile_set.cc@532 PS9, Line 532: current key currently-seeked key in the primary key index http://gerrit.cloudera.org:8080/#/c/10983/9/src/kudu/tablet/cfile_set.cc@576 PS9, Line 576: nit: just indent 4 spaces here per https://google.github.io/styleguide/cppguide.html#Function_Declarations_and_Definitions The preferred alternative is to align the parameters vertically but in this case I think that would make the line too long and it would get flagged by the lint checker. http://gerrit.cloudera.org:8080/#/c/10983/9/src/kudu/tablet/cfile_set.cc@581 PS9, Line 581: : nit: put a space on both sides of the colon in this case to be consistent with the rest of the Kudu code base http://gerrit.cloudera.org:8080/#/c/10983/9/src/kudu/tablet/cfile_set.cc@598 PS9, Line 598: + nit: space around the + for consistency with the rest of the Kudu code base please http://gerrit.cloudera.org:8080/#/c/10983/9/src/kudu/tablet/cfile_set.cc@600 PS9, Line 600: & nit: put the sigil next to the type, not the variable, for consistency with the rest of the Kudu code base, i.e. const ColumnSchema& col = ... Here and elsewhere http://gerrit.cloudera.org:8080/#/c/10983/9/src/kudu/tablet/cfile_set.cc@695 PS9, Line 695: gscoped_ nit: is there something else we should do in this scenario (current key decoding failure) besides crash? Maybe we should RETURN_NOT_OK and have this function return a Status? That way we can detect that skip scan failed, log it, and then disable the optimization? In fact I think that would be much better for the rest of the places we CHECK_OK here... I think RETURN_NOT_OK would be better. http://gerrit.cloudera.org:8080/#/c/10983/9/src/kudu/tablet/index_skipscan-test.cc File src/kudu/tablet/index_skipscan-test.cc: http://gerrit.cloudera.org:8080/#/c/10983/9/src/kudu/tablet/index_skipscan-test.cc@50 PS9, Line 50: nit: multiple consecutive blank lines not needed here, one is enough http://gerrit.cloudera.org:8080/#/c/10983/9/src/kudu/tablet/index_skipscan-test.cc@52 PS9, Line 52: nit: remove this blank line between namespace declarations for consistency with the rest of the Kudu code base http://gerrit.cloudera.org:8080/#/c/10983/9/src/kudu/tablet/index_skipscan-test.cc@65 PS9, Line 65: style nit: remove the blank line after the public keyword per https://google.github.io/styleguide/cppguide.html#Class_Format http://gerrit.cloudera.org:8080/#/c/10983/9/src/kudu/tablet/index_skipscan-test.cc@68 PS9, Line 68: style nit: remove this blank line at the end of the constructor per https://google.github.io/styleguide/cppguide.html#Vertical_Whitespace http://gerrit.cloudera.org:8080/#/c/10983/9/src/kudu/tablet/index_skipscan-test.cc@71 PS9, Line 71: virtual void SetUp() OVERRIDE use the more modern c++11 version of this declaration: void SetUp() override { http://gerrit.cloudera.org:8080/#/c/10983/9/src/kudu/tablet/index_skipscan-test.cc@214 PS9, Line 214: ASSERT_OK_FAST s/ASSERT_OK_FAST/ASSERT_OK/ here and elsewhere http://gerrit.cloudera.org:8080/#/c/10983/9/src/kudu/tablet/index_skipscan-test.cc@244 PS9, Line 244: style nit: remove the blank lines inside this for-loop since they don't apparently help readability http://gerrit.cloudera.org:8080/#/c/10983/9/src/kudu/tablet/index_skipscan-test.cc@263 PS9, Line 263: void ScanTablet(ScanSpec *spec, vector<string> *results, const char *descr) { style nit: type modifier sigils (pointer, reference) should go next to types, not variable names, for consistency with the rest of the Kudu code base (there are a few places in very old code where we violate this, but we no longer produce code that looks like that). So: void ScanTablet(ScanSpec* spec, vector<string>* results, const char* descr) { http://gerrit.cloudera.org:8080/#/c/10983/9/src/kudu/tablet/index_skipscan-test.cc@291 PS9, Line 291: { The braces in a case statement only serve a purpose as a scope for declaring variables, and since there are sub-scopes for that, the outer braces here are not needed and should be removed http://gerrit.cloudera.org:8080/#/c/10983/9/src/kudu/tablet/index_skipscan-test.cc@329 PS9, Line 329: { inner braces serve no purpose here http://gerrit.cloudera.org:8080/#/c/10983/9/src/kudu/tablet/index_skipscan-test.cc@342 PS9, Line 342: { outer braces not needed here http://gerrit.cloudera.org:8080/#/c/10983/9/src/kudu/tablet/index_skipscan-test.cc@367 PS9, Line 367: { outer braces not needed http://gerrit.cloudera.org:8080/#/c/10983/9/src/kudu/tablet/index_skipscan-test.cc@475 PS9, Line 475: nit: superfluous blank line here -- 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: 10 Gerrit-Owner: Anupama Gupta <[email protected]> Gerrit-Reviewer: Alexey Serbin <[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: Tue, 07 Aug 2018 01:28:08 +0000 Gerrit-HasComments: Yes
