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 10: (8 comments) Please review the changes. http://gerrit.cloudera.org:8080/#/c/10983/9/src/kudu/tablet/cfile_set.h File src/kudu/tablet/cfile_set.h: http://gerrit.cloudera.org:8080/#/c/10983/9/src/kudu/tablet/cfile_set.h@260 PS9, Line 260: key_iter_ is not NULL. > this is no longer true (checked inside the method) Done http://gerrit.cloudera.org:8080/#/c/10983/9/src/kudu/tablet/cfile_set.h@264 PS9, Line 264: // higher unique prefix) to scan. : // 2. 'remaining' stores the number the entries to be scanned in the current scan range > Is this an implementation detail or part of the interface of this method? I It is an implementation detail based on the assumption that currently after initializing CFileSet::Iterator, CFileSet::Iterator::SkipToNextScan(..) has exclusive access to this variable. Have added a line about this where this variable is defined. http://gerrit.cloudera.org:8080/#/c/10983/9/src/kudu/tablet/cfile_set.h@266 PS9, Line 266: // : // See the .cc file for details on the approach and the implementation. : void SkipToNextScan(size_t *rema > These variables are local to this method, so I don't see why a caller would Done http://gerrit.cloudera.org:8080/#/c/10983/9/src/kudu/tablet/cfile_set.h@269 PS9, Line 269: : // Collect the IO statistics for each of the underlying columns. > This seems to be one of the key postconditions from my perspective, since i Done http://gerrit.cloudera.org:8080/#/c/10983/9/src/kudu/tablet/cfile_set.h@271 PS9, Line 271: l void Ge > nit: 'remaining' 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@639 PS9, Line 639: // 1. Search within the ad-hoc index for the next unique prefix : // (set of keys prior to the first predicate column). : // Searching is done using validx_iter_. : // 2. Read that unique prefix from the ad-hoc index, append the lower bound : // of the first predicate column, and seek to that. : // If this matches, this is the lower bound of our desired scan. : // 3. If we found our desired lower bou > How about using something a bit higher level to describe this algorithm: Done http://gerrit.cloudera.org:8080/#/c/10983/9/src/kudu/tablet/cfile_set.cc@671 PS9, Line 671: ek to the next prefix if our p > The values need to be inverted due to the variable name change; it doesn't Done http://gerrit.cloudera.org:8080/#/c/10983/9/src/kudu/tablet/cfile_set.cc@820 PS9, Line 820: tion( > nit: unexpected indentation 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: 10 Gerrit-Owner: Anupama Gupta <anupama.gu...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@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: Tue, 07 Aug 2018 00:45:14 +0000 Gerrit-HasComments: Yes
