Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16596 )
Change subject: [partitioning] KUDU-2671: Support for range specific HashSchemas. ...................................................................... Patch Set 8: Code-Review+1 (4 comments) http://gerrit.cloudera.org:8080/#/c/16596/8/src/kudu/common/partition-test.cc File src/kudu/common/partition-test.cc: http://gerrit.cloudera.org:8080/#/c/16596/8/src/kudu/common/partition-test.cc@1052 PS8, Line 1052: // Inserting bounds and their hash schemas out of sorted order, : // yet resulting partitions will still be the same. : bounds.emplace_back(std::move(bounds_with_hash_schemas[1].first)); : range_hash_schemas.emplace_back(std::move(bounds_with_hash_schemas[1].second)); : : bounds.emplace_back(std::move(bounds_with_hash_schemas[2].first)); : range_hash_schemas.emplace_back(std::move(bounds_with_hash_schemas[2].second)); : : bounds.emplace_back(std::move(bounds_with_hash_schemas[0].first)); : range_hash_schemas.emplace_back(std::move(bounds_with_hash_schemas[0].second)); nit: Could we shuffle 'bounds_with_hash_schemas' with std::shuffle, and build 'bounds' and 'range_hash_schemas' with the shuffled ordering? It'd bump the coverage of this test a bit at least. http://gerrit.cloudera.org:8080/#/c/16596/8/src/kudu/common/partition.cc File src/kudu/common/partition.cc: http://gerrit.cloudera.org:8080/#/c/16596/8/src/kudu/common/partition.cc@333 PS8, Line 333: unsigned long nit: auto probably works here too? http://gerrit.cloudera.org:8080/#/c/16596/8/src/kudu/common/partition.cc@335 PS8, Line 335: if (splits.empty()) { : return Status::OK(); : } nit: Could we do this before L333? http://gerrit.cloudera.org:8080/#/c/16596/8/src/kudu/common/partition.cc@407 PS8, Line 407: for (auto schema: range_hash_schemas) { nit: add a space after Also maybe call it 'hash_schemas' so it doesn't collide with 'schema'? -- To view, visit http://gerrit.cloudera.org:8080/16596 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8725f4bd072a81b05b36dfc7df0c074c172b4ce8 Gerrit-Change-Number: 16596 Gerrit-PatchSet: 8 Gerrit-Owner: Mahesh Reddy <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Bankim Bhavsar <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy <[email protected]> Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Wed, 25 Nov 2020 00:25:16 +0000 Gerrit-HasComments: Yes
