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 4: (7 comments) I took a quick look: my comments are nits regarding the style and other minor stuff; I didn't look the semantics or the patch yet. http://gerrit.cloudera.org:8080/#/c/10983/4/src/kudu/cfile/cfile_reader.h File src/kudu/cfile/cfile_reader.h: http://gerrit.cloudera.org:8080/#/c/10983/4/src/kudu/cfile/cfile_reader.h@343 PS4, Line 343: std::string GetCurrentValue(); nit: add const specifier for the method. Also, consider returning const reference from this method, if possible. http://gerrit.cloudera.org:8080/#/c/10983/4/src/kudu/cfile/cfile_reader.cc File src/kudu/cfile/cfile_reader.cc: http://gerrit.cloudera.org:8080/#/c/10983/4/src/kudu/cfile/cfile_reader.cc@788 PS4, Line 788: prepared_blocks_ what if prepared_blocks_ is empty? If that the thing-that-cannot-be, add DCHECK() for the non-emptiness condition. http://gerrit.cloudera.org:8080/#/c/10983/4/src/kudu/tablet/cfile_set.h File src/kudu/tablet/cfile_set.h: http://gerrit.cloudera.org:8080/#/c/10983/4/src/kudu/tablet/cfile_set.h@248 PS4, Line 248: ScanSpec *spec style nit: here and elsewhere -- we tend to stick the asterisk and ampersand to the type, not to the variable/parameter name. http://gerrit.cloudera.org:8080/#/c/10983/4/src/kudu/tablet/cfile_set.cc File src/kudu/tablet/cfile_set.cc: http://gerrit.cloudera.org:8080/#/c/10983/4/src/kudu/tablet/cfile_set.cc@399 PS4, Line 399: ScanSpec If 'spec' instance is not modified here, why not to pass it as a const reference? http://gerrit.cloudera.org:8080/#/c/10983/4/src/kudu/tablet/cfile_set.cc@414 PS4, Line 414: std::unordered_map<std::string, ColumnPredicate> predicates Is copying necessary here? Why not to use const reference? http://gerrit.cloudera.org:8080/#/c/10983/4/src/kudu/tablet/cfile_set.cc@418 PS4, Line 418: (it->second) nit: I don't think the parentheses are necessary here http://gerrit.cloudera.org:8080/#/c/10983/4/src/kudu/tablet/cfile_set.cc@500 PS4, Line 500: skip_scan_enabled_ = CanUseSkipScan(spec); Why not to assign the 'skip_scan_enabled_' field in the CanUseSkipScan() method itself since other aux members (like 'suffix_key_column_id_') are assigned in that method already? And rename CanUseSkipScan() into TryEnableSkipScan() or alike? -- 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: 4 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: Thu, 02 Aug 2018 00:33:01 +0000 Gerrit-HasComments: Yes
