Andrew Wong 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 17: (10 comments) Mostly a nit pass http://gerrit.cloudera.org:8080/#/c/10983/17/src/kudu/tablet/cfile_set.h File src/kudu/tablet/cfile_set.h: http://gerrit.cloudera.org:8080/#/c/10983/17/src/kudu/tablet/cfile_set.h@238 PS17, Line 238: / nit: prefer using // instead of /**/ http://gerrit.cloudera.org:8080/#/c/10983/17/src/kudu/tablet/cfile_set.h@244 PS17, Line 244: leftmost (non-first) primary key column nit: how about "leftmost, non-leading primary key column with a predicate" http://gerrit.cloudera.org:8080/#/c/10983/17/src/kudu/tablet/cfile_set.h@245 PS17, Line 245: equality value on the "predicate column". nit: how about something like "Value that the predicate column is predicated on. Since only equality predicates are supported, there is only a single value of interest." http://gerrit.cloudera.org:8080/#/c/10983/17/src/kudu/tablet/cfile_set.h@246 PS17, Line 246: all columns (together) to the left of the "predicate column". nit: how about "The collective columns to the left of the predicate column" http://gerrit.cloudera.org:8080/#/c/10983/17/src/kudu/tablet/cfile_set.h@247 PS17, Line 247: - nit: extra space, also maybe use : to separate these? http://gerrit.cloudera.org:8080/#/c/10983/17/src/kudu/tablet/cfile_set.h@270 PS17, Line 270: 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 : // this function). nit: now that we have some definitions above, we don't need to redefine these there. http://gerrit.cloudera.org:8080/#/c/10983/17/src/kudu/tablet/cfile_set.h@276 PS17, Line 276: In this case, prior to calling key_iter->SeekAtOrAfter(), the search key is : // modified such that the predicate column is set to the predicate value and the : // succeeding columns are set to their minimum possible values. This is done to : // make sure that after calling key_iter->SeekAtOrAfter() the key_iter is correctly : // placed at the first occurrence of the row (if it exists) that matches the next : // prefix key with the predicate value. I think this is getting a bit too deep into the implementation details (in that it's referencing function names in the CFileReader class), and it's not immediately clear why setting `cache_seeked_value=true` should seek to a different value. Maybe there's a better parameter name that is more telling. Also now that the term "prefix key" is defined to be the columns before the predicate, the name "SeekToNextPrefixKey" may be confusing when run with `skip_scan_predicate_column_id_ + 1`. Maybe we could change this to be a bit clearer. enum { UPPER, LOWER } BoundType; // If seeking for the lower bound, seeks to ... // If seeking for the upper bound, seeks to ... SeekToNextBound(BoundType bound_type); Maybe it'd be fine with just a name change. Or comments that explain why we're seeking to what we're seeking. http://gerrit.cloudera.org:8080/#/c/10983/17/src/kudu/tablet/cfile_set.h@282 PS17, Line 282: // nit: extra line http://gerrit.cloudera.org:8080/#/c/10983/17/src/kudu/tablet/cfile_set.h@285 PS17, Line 285: // Seek to the next predicate match within the current prefix. nit: "Seek to the next row that matches the predicate and has the same prefix as that of `enc_key`" http://gerrit.cloudera.org:8080/#/c/10983/17/src/kudu/tablet/cfile_set.h@333 PS17, Line 333: * key_iter_ is not NULL. : // : // Postconditions upon exiting this method: : // 1. cur_idx_ is updated to the row_id of the next row(containing the next : // higher distinct prefix) to scan. : // 2. 'remaining' stores the number the entries to be scanned in the current scan range. nit: standardize the bullet types -- 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: 17 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: Fri, 17 Aug 2018 23:08:45 +0000 Gerrit-HasComments: Yes
