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 15: (9 comments) http://gerrit.cloudera.org:8080/#/c/10983/15/src/kudu/tablet/cfile_set.cc File src/kudu/tablet/cfile_set.cc: http://gerrit.cloudera.org:8080/#/c/10983/15/src/kudu/tablet/cfile_set.cc@557 PS15, Line 557: Arena arena(FLAGS_max_encoded_key_size_bytes); I think it's possible to pass an instance of arena from the upper level to use it here, like it's done in DecodeCurrentKey(). BTW, that parameter for the Arena constructor -- it's just initial size for the arena, it can grow bigger up to the allowed max size (by default 127 * 8KB, i.e. almost 1 MB). However, setting it up to FLAGS_max_encoded_key_size_bytes is a good enough (arena grows 2x times when needed). http://gerrit.cloudera.org:8080/#/c/10983/15/src/kudu/tablet/cfile_set.cc@571 PS15, Line 571: ( nit: this and the pairing brace are not needed. http://gerrit.cloudera.org:8080/#/c/10983/15/src/kudu/tablet/cfile_set.cc@597 PS15, Line 597: KuduPartialRow *p_row, : const gscoped_ptr<EncodedKey>& cur_enc_key style nit: usually we have input parameter specified first, then go output parameters. http://gerrit.cloudera.org:8080/#/c/10983/15/src/kudu/tablet/cfile_set.cc@603 PS15, Line 603: auto const &value style nit: const auto& value http://gerrit.cloudera.org:8080/#/c/10983/15/src/kudu/tablet/cfile_set.cc@606 PS15, Line 606: p_row->Set(col_id, data); What if this returns non-OK? http://gerrit.cloudera.org:8080/#/c/10983/15/src/kudu/tablet/cfile_set.cc@611 PS15, Line 611: p_row->Set(skip_scan_predicate_column_id_, suffix_col_value); ditto http://gerrit.cloudera.org:8080/#/c/10983/15/src/kudu/tablet/cfile_set.cc@619 PS15, Line 619: ( nit: this and the closing parenthesis are not needed http://gerrit.cloudera.org:8080/#/c/10983/15/src/kudu/tablet/cfile_set.cc@742 PS15, Line 742: CheckPredicateMatch(lower_bound_key); I'm not sure this is what is needed: CheckPredicateMatch() verifies that only the first column of the predicate matches, but we want all columns of the predicate to match, no? If it's what we actually need, maybe add a comment that the lower bound key is just about the match of the first column of the predicate. http://gerrit.cloudera.org:8080/#/c/10983/15/src/kudu/tablet/cfile_set.cc@811 PS15, Line 811: ERROR I think this should be WARNING, because there are some legit cases where SkipToNextScan() can return non-OK. E.g.: E0815 21:19:24.748093 1681 cfile_set.cc:811] Skip scan failed: Illegal state: No lexicographically greater key exists -- 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 <anupama.gu...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Anupama Gupta <anupama.gu...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <mpe...@apache.org> Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Thu, 16 Aug 2018 06:49:03 +0000 Gerrit-HasComments: Yes
