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 13: (13 comments) Went through a few more files; leaving what I have so far. http://gerrit.cloudera.org:8080/#/c/10983/12/src/kudu/cfile/cfile_reader.h File src/kudu/cfile/cfile_reader.h: http://gerrit.cloudera.org:8080/#/c/10983/12/src/kudu/cfile/cfile_reader.h@345 PS12, Line 345: bool set_current_value nit: Even though this is calling StoreCurrentValue(), I think calling this `cache_seeked_value` makes it this a bit clearer. http://gerrit.cloudera.org:8080/#/c/10983/12/src/kudu/cfile/cfile_reader.h@472 PS12, Line 472: // Value currently pointed to by validx_iter_. : // Currently, it is assumed that CFileSet::Iterator::SkipToNextScan(size_t *remaining) : // has exclusive access to use this variable. > Any chance to make update this to keep better level of encapsulation? +1 I might be missing something, but would we be able to only have a single public function GetCurrentValue() that does what StoreCurrentValue() does, but has a string as an outparameter? Not sure if this would yield the different results than if we were to continue storing via 'set_current_value'. http://gerrit.cloudera.org:8080/#/c/10983/12/src/kudu/tablet/cfile_set.h File src/kudu/tablet/cfile_set.h: http://gerrit.cloudera.org:8080/#/c/10983/12/src/kudu/tablet/cfile_set.h@213 PS12, Line 213: // 'cache_seeked_value' controls whether we will remember the next key seeked to in : // this function. nit: Reword as "If `cache_seeked_value` is true, the predicate column iterator will store the value seeked to. http://gerrit.cloudera.org:8080/#/c/10983/12/src/kudu/tablet/cfile_set.h@220 PS12, Line 220: // Builds a key containing the current prefix key, predicate column value : // and the minimum value for rest of the key columns. nit: reword as "Build the key with the same prefix as `cur_enc_key`, that has `skip_scan_predicate_value_` in its predicate column, and the minimum possible value for all other columns. http://gerrit.cloudera.org:8080/#/c/10983/12/src/kudu/tablet/cfile_set.cc File src/kudu/tablet/cfile_set.cc: http://gerrit.cloudera.org:8080/#/c/10983/12/src/kudu/tablet/cfile_set.cc@436 PS12, Line 436: nit: this can just be `spec.predicates()`, then there would be no need for a `predicates` var http://gerrit.cloudera.org:8080/#/c/10983/12/src/kudu/tablet/cfile_set.cc@440 PS12, Line 440: const string& col_name = col_and_pred.first; nit: move this out of the loop, maybe up by L413 and then use it in defining num_key_cols. http://gerrit.cloudera.org:8080/#/c/10983/12/src/kudu/tablet/cfile_set.cc@462 PS12, Line 462: } : : if (nonfirst_key_pred_exists) { : use_skip_scan_ = true; : : // Store the predicate column id. : skip_scan_predicate_column_id_ = min_col_id; : : > nit: why not to move this under the same scope where the nonfirst_key_pred_ It was originally written as described, but I asked her to make the change, reason being it seems a bit off to update these variables if we end up disabling the skip scan. Not sure my concern is important, since these variables (I think) are unused if we're not skip-scanning. I'd be ok reverting if you prefer, but I agree the semantics should be updated. http://gerrit.cloudera.org:8080/#/c/10983/12/src/kudu/tablet/cfile_set.cc@551 PS12, Line 551: : Status CFileSet::Iterator::SeekToNextPrefixKey(size_t num_prefix_cols, bool cache_seeked_value) { : gscoped_ptr<EncodedKey> enc_key; Hmm.. This is doing a lot of heap-allocating and gets called _very often_. I wonder if we could keep a single Arena or unique_ptr<Arena> as a member of the iterator, though I'm not sure our Arena implementation is reusable in that way. Might be worth looking into if you notice high memory usage in your cluster testing http://gerrit.cloudera.org:8080/#/c/10983/12/src/kudu/tablet/cfile_set.cc@561 PS12, Line 561: &enc_key, &arena, num_prefix_cols)); : : if (!cache_seeked_value) { : return key_iter_->SeekAtOrAfter(*enc_key, : /* set_current_value= */ cache_seeked_value, : /* exact_match= */ nullptr); : } : : // Set the predicate column to the predicate value in case we can find a : // predicate match in one search. As a side effect, GetKeyWithPredicateVal() : // sets minimum values on the columns after the predicate value, which is : // required for correctness here. : KuduPartialRow partial_row(&(base_data_->tablet_schema())); : enc_key = GetKeyWithPredicateVal(&partial_row, enc_key); : return key_iter_->SeekAtOrAfter( I think this would be clearer as if (cache_seeked_value) { KuduPartialRow partial_row(&schema); enc_key = GetKeyWithPredicateVal(&partial_row, enc_key); } return key_iter_->SeekAtOrAfter(*enc_key, cache_seeked_value, nullptr); http://gerrit.cloudera.org:8080/#/c/10983/12/src/kudu/tablet/cfile_set.cc@583 PS12, Line 583: RETURN_NOT_OK(DecodeCurrentKey(&arena, &enc_key)); : : // Check to see if the current key matches the predicate value. If so, then : // there is no need to seek forward. : i Hmm.. IMO SeekToNext* should always move forward at least one row, unless we're at the end of a scan. Can we rename this to SeekToRowWithCurPrefixMatchingPred() or something? http://gerrit.cloudera.org:8080/#/c/10983/12/src/kudu/tablet/cfile_set.cc@662 PS12, Line 662: : : // This is a three seek approach for index skip scan implementation: : // 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 bound, find an upper Can you also label down below where 1., 2., and 3. are? Also nit: s/unique/distinct http://gerrit.cloudera.org:8080/#/c/10983/12/src/kudu/tablet/cfile_set.cc@704 PS12, Line 704: I don't think upper_bound_idx_ changes for the duration of a scan, so maybe keep some `skip_scan_num_seeks_cutoff_` so we don't have to keep computing this http://gerrit.cloudera.org:8080/#/c/10983/12/src/kudu/tablet/cfile_set.cc@721 PS12, Line 721: gscoped_ptr<EncodedKey> next_prefix_key; This decodes the current key, and then immediately after we call SeekToNextPredicateMatchCurPrefix(), which decodes the current key again. Could we have passed in `next_prefix_key`? -- 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: 13 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: Wed, 08 Aug 2018 07:35:33 +0000 Gerrit-HasComments: Yes
