Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16596 )
Change subject: WIP [partitioning] KUDU-2671: Support for range specific HashSchemas. ...................................................................... Patch Set 7: (6 comments) Overall looking better. Just some more cosmetic suggestions and a suggestion to reduce code duplication in test. http://gerrit.cloudera.org:8080/#/c/16596/7//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/16596/7//COMMIT_MSG@14 PS7, Line 14: Since split_rows only exists for backwards compatibility reasons, I'm : leaning towards not supporting this feature with split_rows. Returning : a message to the user stating to specify both upper and lower bounds : either at table creation or alteration time should suffice. Split_rows : is also more syntactically ambigious when specifying bounds. +1 It isn't clear to me whether this is the reason this is marked WIP. If so, could you make that clearer by saying something like, "WIP because ..." Also, if you want to discuss this further, I'd recommend pulling people into a meeting or an email thread or Slack discussion to discuss further. http://gerrit.cloudera.org:8080/#/c/16596/7/src/kudu/common/partition-test.cc File src/kudu/common/partition-test.cc: http://gerrit.cloudera.org:8080/#/c/16596/7/src/kudu/common/partition-test.cc@121 PS7, Line 121: CheckPartitions nit: maybe move this closer to the test that uses it. Protip: you can create anonymous namespaces anywhere in the file, not just at the top. The top is just convenient if we expect many tests to uses these methods, which isn't the case here. http://gerrit.cloudera.org:8080/#/c/16596/7/src/kudu/common/partition-test.cc@1101 PS7, Line 1101: range_hash_schema nit: make this plural http://gerrit.cloudera.org:8080/#/c/16596/7/src/kudu/common/partition-test.cc@1042 PS7, Line 1042: PartitionTest, TestVaryingHashSchemasPerRangeOutOfOrder) { : // CREATE TABLE t (a VARCHAR, b VARCHAR, c VARCHAR, PRIMARY KEY (a, b, c)) : // PARTITION BY [HASH BUCKET (a, c), HASH BUCKET (b), RANGE (a, b, c)]; : Schema schema({ ColumnSchema("a", STRING), : ColumnSchema("b", STRING), : ColumnSchema("c", STRING) }, : { ColumnId(0), ColumnId(1), ColumnId(2) }, 3); : : PartitionSchemaPB schema_builder; : // Table-wide HashSchema defined below, 3 by 2 buckets so 6 total. : AddHashBucketComponent(&schema_builder, { "a", "c" }, 3, 0); : AddHashBucketComponent(&schema_builder, { "b" }, 2, 0); : PartitionSchema partition_schema; : ASSERT_OK(PartitionSchema::FromPB(schema_builder, schema, &partition_schema)); : : ASSERT_EQ("HASH (a, c) PARTITIONS 3, HASH (b) PARTITIONS 2, RANGE (a, b, c)", : partition_schema.DebugString(schema)); : : vector<pair<KuduPartialRow, KuduPartialRow>> bounds; : : // The bounds aren't inserted in sorted order unlike the previous test, : // yet the resulting partitions will be the same. : { // [(a3, b3, _), (a4, b4, _)) : KuduPartialRow lower(&schema); : KuduPartialRow upper(&schema); : ASSERT_OK(lower.SetStringCopy("a", "a3")); : ASSERT_OK(lower.SetStringCopy("b", "b3")); : ASSERT_OK(upper.SetStringCopy("a", "a4")); : ASSERT_OK(upper.SetStringCopy("b", "b4")); : bounds.emplace_back(std::move(lower), std::move(upper)); : } : : { // [(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")); : bounds.emplace_back(std::move(lower), std::move(upper)); : } : : { // [(a5, b5, _), (a6, _, c6)) : KuduPartialRow lower(&schema); : KuduPartialRow upper(&schema); : ASSERT_OK(lower.SetStringCopy("a", "a5")); : ASSERT_OK(lower.SetStringCopy("b", "b5")); : ASSERT_OK(upper.SetStringCopy("a", "a6")); : ASSERT_OK(upper.SetStringCopy("c", "c6")); : bounds.emplace_back(std::move(lower), std::move(upper)); : } : : vector<PartitionSchema::HashBucketSchema> hash_schema_4_buckets = {{{ColumnId(0)}, 4, 0}}; : vector<PartitionSchema::HashBucketSchema> hash_schema_2_buckets_by_3 = { : {{ColumnId(0)}, 2, 0}, : {{ColumnId(1)}, 3, 0} : }; : : // HashBucketSchemas have to be in the same order as the bounds it corresponds to : PartitionSchema::RangeHashSchema range_hash_schema = { : {vector<PartitionSchema::HashBucketSchema>()}, : {hash_schema_4_buckets}, : {hash_schema_2_buckets_by_3} : }; : : vector<Partition> partitions; : ASSERT_OK(partition_schema.CreatePartitions({}, bounds, range_hash_schema, schema, &partitions)); : CheckPartitions(partitions); : } How about instead of duplicating the above test, we combine 'bounds' and 'range_hash_schema' to 'bounds_and_hash_schemas' of type vector<pair<pair<KuduPartialRow, KuduPartialRow>, RangeHashSchema>>. Then, we could generate 'bounds' and 'range_hash_schemas' from the 'bounds_and_hash_schemas' that would correctly correspond with each other, even if we were to, say, shuffle 'bounds_and_hash_schemas'. http://gerrit.cloudera.org:8080/#/c/16596/7/src/kudu/common/partition.cc File src/kudu/common/partition.cc: http://gerrit.cloudera.org:8080/#/c/16596/7/src/kudu/common/partition.cc@329 PS7, Line 329: bounds_with_hash_schemas) const { If 'splits' is empty, could we short circuit out of here and just return without changing 'bounds_with_hash_schemas'? http://gerrit.cloudera.org:8080/#/c/16596/7/src/kudu/common/partition.cc@359 PS7, Line 359: bound.hash_schemas If you go my suggested route of short circuiting, we should be able to just replace these with {}, right? Since hash schemas + split rows isn't supported? -- 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: 7 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: Tue, 24 Nov 2020 07:29:27 +0000 Gerrit-HasComments: Yes
