Dan Burkert has posted comments on this change. Change subject: KUDU-1165: Implement partition pruning for C++ client and server ......................................................................
Patch Set 10: (19 comments) http://gerrit.cloudera.org:8080/#/c/2413/10/src/kudu/client/client.cc File src/kudu/client/client.cc: Line 1063: if (!data_->partition_pruner_.HasMorePartitions()) { > can you just make this a || for the above if? Done http://gerrit.cloudera.org:8080/#/c/2413/10/src/kudu/common/CMakeLists.txt File src/kudu/common/CMakeLists.txt: Line 89: #ADD_KUDU_TEST(partition_pruner-test) > hrm? Done http://gerrit.cloudera.org:8080/#/c/2413/10/src/kudu/common/partition_pruner-test.cc File src/kudu/common/partition_pruner-test.cc: Line 54: size_t expected_pruned_tablets) { > maybe it's just me, but as I was reviewing this file, I found I was able to Done Line 343: Check(vector<ColumnPredicate> {}, 0); > curious (still learning c++11): why do you need to list the type here for t you are right, it will get inferred correctly. Updated. http://gerrit.cloudera.org:8080/#/c/2413/10/src/kudu/common/partition_pruner.cc File src/kudu/common/partition_pruner.cc: Line 72: if (scan_spec.lower_bound_key() != nullptr || scan_spec.exclusive_upper_bound_key() != nullptr) { > invert this condition to return early Done Line 130: Arena arena(max<size_t>(16, schema.key_byte_size()), 4096); > maybe add Arena::kMinimumChunkSize so this isn't a magic number 16? Done Line 147: const ScanSpec& scan_spec) { > is there a way we can CHECK(scan_spec.optimized()) or something to enforce These preconditions are checked by CanShortCircuit, so it's not necessary for the scan spec to be optimized (all though in all cases it should be). There isn't an easy way to check if the scan spec has been optimized. Line 174: // Construct the list of hash buckets, or none if the hash bucket is not > this could do with a big header above it something like: Done Line 179: int constrained_index = -1; > hrm, not entirely following the purpose of this. This is just the index of Done Line 186: for (int i = 0; i < hash_bucket_schema.column_ids.size(); i++) { > can you rename this or the other (or both) loop index to avoid shadowing 'i Done Line 208: if (!range_lower_bound.empty() && !range_upper_bound.empty()) { > I don't entirely understand what this is doing either, should add a comment Done Line 258: for (auto range = ranges.rbegin(); range != ranges.rend(); range++) { > can you refactor this into another method like RemovePartitionKeyRange belo This is only done in this method, and it's working on the set of partition keys in an intermediate representation (sorted as opposed to reverse sorted), so I don't know if it would be good to expose that. Line 260: scan_spec.exclusive_upper_bound_partition_key() < get<1>(*range)) { > can you invert this and break early to avoid the extra nesting? Done Line 291: void PartitionPruner::RemovePartitionKeyRange(const string& bound) { > clarify bound as upper or lower bound Done Line 301: if (bound > get<0>(*range)) { > same Done http://gerrit.cloudera.org:8080/#/c/2413/10/src/kudu/common/partition_pruner.h File src/kudu/common/partition_pruner.h: Line 33: class Schema; > given the above forward decls, do we need the #includes above? Done Line 35: class PartitionPruner { > could do with a little class-doc Done Line 51: void RemovePartitionKeyRange(const std::string& upper_bound); > I think this method is named somewhat unintuitively would you prefer RemovePartitionKeyRangeBefore? I'm somewhat on the fence about the class being named PartitionPruner at all. I named it that when the primary interface was ShouldPrune, but then later changed the mechanics in order to save on master lookups. Line 64: }; > DISALLOW_COPY_AND_ASSIGN Done -- To view, visit http://gerrit.cloudera.org:8080/2413 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I9f4fc14b44af63a17b34cd9c2a9134127b1afe4d Gerrit-PatchSet: 10 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
