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 3: (14 comments) http://gerrit.cloudera.org:8080/#/c/17643/3/src/kudu/common/partition.cc File src/kudu/common/partition.cc: http://gerrit.cloudera.org:8080/#/c/17643/3/src/kudu/common/partition.cc@1109 PS3, Line 1109: hb_schemas nit: parameter's name is different between the declaration and the definition http://gerrit.cloudera.org:8080/#/c/17643/3/src/kudu/common/partition.cc@1147 PS3, Line 1147: for (int i = 0; i < ranges_with_hash_schemas_.size(); i++) { : if (!EqualHashBucketSchemas(ranges_with_hash_schemas_[i].hash_schemas, : rhs.ranges_with_hash_schemas_[i].hash_schemas)) { : return false; : } : } In addition, does it make sense to compare the lower and the upper boundaries per every pair of corresponding elements in PartitionSchema::ranges_with_hash_schemas_? Also, it would be great to add a unit test to cover the functionality of PartitionSchema::operator==(). http://gerrit.cloudera.org:8080/#/c/17643/3/src/kudu/common/partition_pruner-test.cc File src/kudu/common/partition_pruner-test.cc: http://gerrit.cloudera.org:8080/#/c/17643/3/src/kudu/common/partition_pruner-test.cc@1079 PS3, Line 1079: nit: the indent for this line is off Overall, I guess it would be easier to read if it were formatted as: void X( const Y& y, ... size_t z) { ... } http://gerrit.cloudera.org:8080/#/c/17643/3/src/kudu/common/partition_pruner-test.cc@1304 PS3, Line 1304: // CREATE TABLE t : // (a INT8, b INT8, c INT8) : // PRIMARY KEY (a, b, c) : // PARTITION BY RANGE(a, b) : : // Setup the Schema : Schema schema({ ColumnSchema("a", INT8), : ColumnSchema("b", INT8), : ColumnSchema("c", INT8) }, : { ColumnId(0), ColumnId(1), ColumnId(2) }, : 3); Do you mind adding a scenario for a scheme like this but with custom hash bucket schemas per range, so it would be 2, 3, 4 buckets per range correspondingly? http://gerrit.cloudera.org:8080/#/c/17643/3/src/kudu/common/partition_pruner-test.cc@1326 PS3, Line 1326: std::make_pair("a", 0) nit here and below where std::make_pair() is used: would it be enough to have just {"a", 0} instead of std::make_pair("a", 0)? http://gerrit.cloudera.org:8080/#/c/17643/3/src/kudu/common/partition_pruner-test.cc@1355 PS3, Line 1355: Check nit: add 'const' and turn into all-lowercase http://gerrit.cloudera.org:8080/#/c/17643/3/src/kudu/common/partition_pruner-test.cc@1366 PS3, Line 1366: CHECK_OK Why not to use ASSERT_OK() here? http://gerrit.cloudera.org:8080/#/c/17643/3/src/kudu/common/partition_pruner-test.cc@1381 PS3, Line 1381: CheckPrunedPartitions nit: add NO_FATALS() here? http://gerrit.cloudera.org:8080/#/c/17643/3/src/kudu/common/partition_pruner-test.cc@1411 PS3, Line 1411: } Do you mind adding a scenario where the range for the primary key should lead to empty list of partitions to scan? http://gerrit.cloudera.org:8080/#/c/17643/3/src/kudu/common/partition_pruner-test.cc@1428 PS3, Line 1428: make_tuple(columns, 3, 0) nit: would {columns, 3, 0} be enough here? http://gerrit.cloudera.org:8080/#/c/17643/3/src/kudu/common/partition_pruner-test.cc@1478 PS3, Line 1478: int8_t zero = 0; : int8_t one = 1; : int8_t eight = 8; nit: make these constexpr? http://gerrit.cloudera.org:8080/#/c/17643/3/src/kudu/common/partition_pruner.h File src/kudu/common/partition_pruner.h: http://gerrit.cloudera.org:8080/#/c/17643/3/src/kudu/common/partition_pruner.h@49 PS3, Line 49: nit: it should be 4 spaces here per style guide http://gerrit.cloudera.org:8080/#/c/17643/3/src/kudu/common/partition_pruner.cc File src/kudu/common/partition_pruner.cc: http://gerrit.cloudera.org:8080/#/c/17643/3/src/kudu/common/partition_pruner.cc@229 PS3, Line 229: void PartitionPruner::ConstructPartitionKeyRanges(const Schema& schema, : const PartitionSchema& partition_schema, : const ScanSpec& scan_spec, : const PartitionSchema::HashBucketSchemas& hash_bucket_schemas, : const LowerUpperRangeBounds& range_bounds, : vector<PartitionKeyRange>* partition_key_ranges) { nit: fix the indent misalignment for parameters http://gerrit.cloudera.org:8080/#/c/17643/3/src/kudu/common/partition_pruner.cc@435 PS3, Line 435: if (!partition_schema.ranges_with_hash_schemas_.empty()) nit: could you swap the predicate and corresponding code blocks? It would be more readable the other way: if (partition_schema.ranges_with_hash_schemas_.empty()) { ... } ... } -- 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: 3 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: Fri, 23 Jul 2021 19:27:56 +0000 Gerrit-HasComments: Yes
