Mahesh Reddy 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: (8 comments) http://gerrit.cloudera.org:8080/#/c/16596/6//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/16596/6//COMMIT_MSG@18 PS6, Line 18: n specify > ambiguous Ack 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@1000 PS8, Line 1000: { // [(a1, _, c1), (a2, _, c2)) : KuduPartialRow lower(&schema); : KuduPartialRow upper(&schema); : ASSERT_OK(lower.SetStringCopy("a", "a1")); : ASSERT_OK(lower.SetStringCopy("c", "c1")); : ASSERT_OK(upper.SetStringCopy("a", "a2")); : ASSERT_OK(upper.SetStringCopy("c", "c2")); : PartitionSchema::HashBucketSchemas hash_schema_4_buckets = {{{ColumnId(0)}, 4, 0}}; : bounds.emplace_back(lower, upper); : range_hash_schemas.emplace_back(hash_schema_4_buckets); : bounds_with_hash_schemas.emplace_back(make_pair(std::move(lower), std::move(upper)), : std::move(hash_schema_4_buckets)); : } > Would it make sense to check for the result of partition building after eac The partitions don't get filled until CreatePartitions() is called later. These blocks are just populating the bounds and their respective hash schemas that will be passed as parameters to CreatePartitions(). So I'm not exactly sure what we would check after these blocks, maybe I misunderstood your question. 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 bui Ack 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@128 PS8, Line 128: column_ids > nit: not exactly the essential part of this changelist, but since you are t Ack 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? Ack 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? Ack 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 Ack http://gerrit.cloudera.org:8080/#/c/16596/8/src/kudu/common/partition.cc@409 PS8, Line 409: return Status::InvalidArgument("Both 'split_rows' and 'range_hash_schemas' cannot be " : "populated at the same time."); > Could you add a testcase to trigger this error to make sure this method beh Ack -- 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 20:20:59 +0000 Gerrit-HasComments: Yes
