Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/17643 )
Change subject: [pruning] KUDU-2671: Pruning compatible with custom hash schemas. ...................................................................... Patch Set 7: (4 comments) http://gerrit.cloudera.org:8080/#/c/17643/6/src/kudu/common/partition_pruner-test.cc File src/kudu/common/partition_pruner-test.cc: http://gerrit.cloudera.org:8080/#/c/17643/6/src/kudu/common/partition_pruner-test.cc@1490 PS6, Line 1490: } > Custom hash schemas per range shouldn't change how ScanSpec::CanShortCircui Yeah, the idea is not to exercise the paths in the ScanSpec::CanShortCircuit() function, but rather see how the rest of the code works in case when CanShortCircuit() returns true. http://gerrit.cloudera.org:8080/#/c/17643/6/src/kudu/common/partition_pruner-test.cc@1491 PS6, Line 1491: } // namespace kudu > TestHashSchemasPerRangePruning has a PK of (A, B, C) and is range partition My concern is how the new code works in case of having a single element in a range. E.g., in https://gerrit.cloudera.org/#/c/17643/7/src/kudu/common/partition_pruner.cc@467 there is a code comparing the ranges with strict inequality comparisons -- do those work as intended for a single-element ranges? http://gerrit.cloudera.org:8080/#/c/17643/6/src/kudu/common/partition_pruner.h File src/kudu/common/partition_pruner.h: http://gerrit.cloudera.org:8080/#/c/17643/6/src/kudu/common/partition_pruner.h@48 PS6, Line 48: ounds(std > Nope, the pairs aren't related. Each pair contains the range bounds and the +1 for making RangeBoundsAndPartitionKeyRanges a struct. I guess it's more or less acceptable to keep RangeBounds and PartitionKeyRange as pairs/tuples, but having them as structs is better due to the observation pointed by Andrew. Thank you for making this update! http://gerrit.cloudera.org:8080/#/c/17643/6/src/kudu/common/partition_pruner.h@71 PS6, Line 71: : RangeBounds range_bounds; : std::vector<PartitionKeyRange> partition_key_ranges; : }; : > The issue with using the pre-computed result is that PartitionPruner::Remov Yep, makes sense. I guess we can update the behavior as needed if we find the computations in NumRangesRemaining() are an issue in a hot path, but as of now that would be a premature optimization, indeed. -- To view, visit http://gerrit.cloudera.org:8080/17643 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I05c37495430f61a2c6f6012c72251138aee465b7 Gerrit-Change-Number: 17643 Gerrit-PatchSet: 7 Gerrit-Owner: Mahesh Reddy <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy <[email protected]> Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Mon, 02 Aug 2021 22:40:38 +0000 Gerrit-HasComments: Yes
