Anupama Gupta 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 11: (41 comments) http://gerrit.cloudera.org:8080/#/c/10983/10/src/kudu/cfile/cfile_reader.h File src/kudu/cfile/cfile_reader.h: http://gerrit.cloudera.org:8080/#/c/10983/10/src/kudu/cfile/cfile_reader.h@59 PS10, Line 59: static const int kEncodedCompositeKeyMaxSize = > Great question. This is from http://kudu.apache.org/docs/known_issues.html# Refactored this name to: kEncodedCompositeKeyMaxSize as it makes more sense wrt our skip scan use case. http://gerrit.cloudera.org:8080/#/c/10983/10/src/kudu/cfile/cfile_reader.h@339 PS10, Line 339: to store the value obtaine > nit: note that the "current value" refers to the value after seeking. Done http://gerrit.cloudera.org:8080/#/c/10983/10/src/kudu/common/encoded_key.h File src/kudu/common/encoded_key.h: http://gerrit.cloudera.org:8080/#/c/10983/10/src/kudu/common/encoded_key.h@63 PS10, Line 63: ; > nit: what does this do? Created another function IncrementEncodedKeyColumns(..) for better separation of responsibility and documented this parameter. http://gerrit.cloudera.org:8080/#/c/10983/10/src/kudu/common/partial_row.h File src/kudu/common/partial_row.h: http://gerrit.cloudera.org:8080/#/c/10983/10/src/kudu/common/partial_row.h@52 PS10, Line 52: mplate<typename KeyTypeWrapper> struct SliceTypeRowOps; // IWYU pragma: keep : template<typename KeyTypeWrapper> struct NumTypeRowOps; // IWYU pragma: keep > nit: not your fault, but mind removing the spaces here? Done http://gerrit.cloudera.org:8080/#/c/10983/10/src/kudu/tablet/cfile_set.h File src/kudu/tablet/cfile_set.h: http://gerrit.cloudera.org:8080/#/c/10983/10/src/kudu/tablet/cfile_set.h@207 PS10, Line 207: gscoped_ptr > nit: if the change would be contained within cfile_set.cc, mind updating th Noted. Sticking with gscoped_ptr for now since it's used outside cfile_set.cc (in encode_key.cc). http://gerrit.cloudera.org:8080/#/c/10983/10/src/kudu/tablet/cfile_set.h@209 PS10, Line 209: refix key. > nit: Since this isn't a variable name, maybe take out the underscore. Alright. Changed here and elsewhere. http://gerrit.cloudera.org:8080/#/c/10983/10/src/kudu/tablet/cfile_set.h@210 PS10, Line 210: um_prefix_ > nit: 'num_prefix_cols' Done http://gerrit.cloudera.org:8080/#/c/10983/10/src/kudu/tablet/cfile_set.h@210 PS10, Line 210: // prefix key refers to the first "num_prefix_cols" columns of the current key. : // current key is the key currently pointed to by validx_iter_ (prior to calling SeekToNextPrefixKey()). : // 'cache_seeked_value' controls whether we will remember the next key seeked to in : // this function. > nit: can you reword this to clarify whether "current" refers to before or a Done 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 en Done http://gerrit.cloudera.org:8080/#/c/10983/9/src/kudu/tablet/cfile_set.cc@80 PS9, Line 80: "Max number of skip attempts the skip scan optimization should make before " > Add: Done http://gerrit.cloudera.org:8080/#/c/10983/9/src/kudu/tablet/cfile_set.cc@405 PS9, Line 405: e > nit: extra space Done 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 = a Done http://gerrit.cloudera.org:8080/#/c/10983/9/src/kudu/tablet/cfile_set.cc@430 PS9, Line 430: key_cols; > nit: move this comment inside the 'if' to right below this line Done http://gerrit.cloudera.org:8080/#/c/10983/9/src/kudu/tablet/cfile_set.cc@436 PS9, Line 436: nst auto& co > nit: unnecessary parentheses Done http://gerrit.cloudera.org:8080/#/c/10983/9/src/kudu/tablet/cfile_set.cc@437 PS9, Line 437: > nit: put this comment on its own line Done http://gerrit.cloudera.org:8080/#/c/10983/9/src/kudu/tablet/cfile_set.cc@444 PS9, Line 444: p); > nit: unnecessary parentheses Done http://gerrit.cloudera.org:8080/#/c/10983/9/src/kudu/tablet/cfile_set.cc@522 PS9, Line 522: << KUD > nit: remove so much extra horizontal white space on this line Done http://gerrit.cloudera.org:8080/#/c/10983/9/src/kudu/tablet/cfile_set.cc@531 PS9, Line 531: " > s/or/which we consider to be/ Done http://gerrit.cloudera.org:8080/#/c/10983/9/src/kudu/tablet/cfile_set.cc@532 PS9, Line 532: > currently-seeked key in the primary key index For better clarity, changed this to - "cached value obtained after the previous seek 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/cppg Done http://gerrit.cloudera.org:8080/#/c/10983/9/src/kudu/tablet/cfile_set.cc@581 PS9, Line 581: r > nit: put a space on both sides of the colon in this case to be consistent w Done 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 Done http://gerrit.cloudera.org:8080/#/c/10983/9/src/kudu/tablet/cfile_set.cc@600 PS9, Line 600: n > nit: put the sigil next to the type, not the variable, for consistency with Done http://gerrit.cloudera.org:8080/#/c/10983/9/src/kudu/tablet/cfile_set.cc@695 PS9, Line 695: // Onl > nit: is there something else we should do in this scenario (current key dec Makes sense. Changed to RETURN_NOT_OK here and elsewhere. http://gerrit.cloudera.org:8080/#/c/10983/10/src/kudu/tablet/cfile_set.cc File src/kudu/tablet/cfile_set.cc: http://gerrit.cloudera.org:8080/#/c/10983/10/src/kudu/tablet/cfile_set.cc@413 PS10, Line 413: num_key_cols = base_data_->tablet_schema().num_key_columns > nit: IMO slightly easier to read as (lower_bound_idx_ != 0 || upper_bound_i Done http://gerrit.cloudera.org:8080/#/c/10983/10/src/kudu/tablet/cfile_set.cc@423 PS10, Line 423: return; : } > Merge these into something like: Done http://gerrit.cloudera.org:8080/#/c/10983/10/src/kudu/tablet/cfile_set.cc@441 PS10, Line 441: : // Get the column id from the predicate : StringPiece sp(reinterpret_cast<const char*>(col_name.data()), col_name.size()); : int col_id = schema.find_column(sp); > I think it's generally good hygiene that these don't get modified until aft Makes sense. Now modifying these values only when enabling skip scan. http://gerrit.cloudera.org:8080/#/c/10983/10/src/kudu/tablet/cfile_set.cc@509 PS10, Line 509: if (spec->exclusive_upper > nit: I know it doesn't make a difference execution-wise, but I think this m You are right. Moved this function call. 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: namespace kudu { > nit: multiple consecutive blank lines not needed here, one is enough Done 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 Done 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://googl Done http://gerrit.cloudera.org:8080/#/c/10983/9/src/kudu/tablet/index_skipscan-test.cc@68 PS9, Line 68: KuduTabletTest::SetUp(); > style nit: remove this blank line at the end of the constructor per https:/ Done http://gerrit.cloudera.org:8080/#/c/10983/9/src/kudu/tablet/index_skipscan-test.cc@71 PS9, Line 71: > use the more modern c++11 version of this declaration: Done http://gerrit.cloudera.org:8080/#/c/10983/9/src/kudu/tablet/index_skipscan-test.cc@214 PS9, Line 214: > s/ASSERT_OK_FAST/ASSERT_OK/ here and elsewhere Done http://gerrit.cloudera.org:8080/#/c/10983/9/src/kudu/tablet/index_skipscan-test.cc@244 PS9, Line 244: CHECK_OK(row.SetInt8(4, p5)); > style nit: remove the blank lines inside this for-loop since they don't app Done http://gerrit.cloudera.org:8080/#/c/10983/9/src/kudu/tablet/index_skipscan-test.cc@263 PS9, Line 263: ASSERT_OK(IterateToStringList(iter.get(), results)); > style nit: type modifier sigils (pointer, reference) should go next to type Done http://gerrit.cloudera.org:8080/#/c/10983/9/src/kudu/tablet/index_skipscan-test.cc@291 PS9, Line 291: r > The braces in a case statement only serve a purpose as a scope for declarin Done 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 Done http://gerrit.cloudera.org:8080/#/c/10983/9/src/kudu/tablet/index_skipscan-test.cc@342 PS9, Line 342: 0 > outer braces not needed here Done http://gerrit.cloudera.org:8080/#/c/10983/9/src/kudu/tablet/index_skipscan-test.cc@367 PS9, Line 367: > outer braces not needed Done http://gerrit.cloudera.org:8080/#/c/10983/9/src/kudu/tablet/index_skipscan-test.cc@475 PS9, Line 475: > nit: superfluous blank line here Done -- 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: 11 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: Tue, 07 Aug 2018 23:23:06 +0000 Gerrit-HasComments: Yes
