Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/17879 )
Change subject: KUDU-2671: Follow up pruning patch. ...................................................................... Patch Set 9: (3 comments) http://gerrit.cloudera.org:8080/#/c/17879/9/src/kudu/common/partition_pruner.cc File src/kudu/common/partition_pruner.cc: http://gerrit.cloudera.org:8080/#/c/17879/9/src/kudu/common/partition_pruner.cc@524 PS9, Line 524: !scan_contains_predicates_ The presence of this condition looks a bit fishy to me from the conceptual point of view. As discussed offline, the partition pruning logic builds intervals in the tablet key range space that corresponds to the specified predicates. PartitionPruner::RemovePartitionKeyRange() is invoked only after those have been built: it moves the 'iterator' through once data has been read by a scanner up to specified 'upper_bound', so no predicates or their absence would matter at this point, I'd think. Maybe, there is some issue with building range_bounds_to_partition_key_ranges_ itself when no predicates are specified? But I might be missing something here. http://gerrit.cloudera.org:8080/#/c/17879/9/src/kudu/common/partition_pruner.cc@525 PS9, Line 525: range_key_upper != (*outer_range_it).range_bounds.upper) { Overall, I'm not sure there is a guarantee from the scanner that 'upper_bound' exactly corresponds to the boundary of an existing range partition. This might be brittle -- it make sense to double-check that. http://gerrit.cloudera.org:8080/#/c/17879/9/src/kudu/common/partition_pruner.cc@542 PS9, Line 542: // Remove set of range bounds and its partition key ranges if : // all the partition key ranges within the set have been removed. : range_bounds_to_partition_key_ranges_.erase( : std::remove_if(range_bounds_to_partition_key_ranges_.begin(), : range_bounds_to_partition_key_ranges_.end(), : [] (const auto& v) : { return v.partition_key_ranges.empty(); }), : range_bounds_to_partition_key_ranges_.end()); Instead of going over all the elements in the 'range_bounds_to_partition_key_ranges_' contains, is it possible to remove the element right after calling 'partition_key_range.pop_back();' at line 537 if it turns out that 'partition_key_range' is empty? Overall, I'd expect that the scan ranges built by PartitionPruner would be ordered (at least that was the case some time ago), so it was easy to have that sort of logic implemented. -- To view, visit http://gerrit.cloudera.org:8080/17879 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3a1bf5344c0ef856072d3ed102714dce5ba21060 Gerrit-Change-Number: 17879 Gerrit-PatchSet: 9 Gerrit-Owner: Mahesh Reddy <[email protected]> Gerrit-Reviewer: Abhishek Chennaka <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy <[email protected]> Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Fri, 24 Jun 2022 22:38:09 +0000 Gerrit-HasComments: Yes
