Mahesh Reddy 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) 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 I initially marked it as WIP but at this point I don't see a reason for it to be marked WIP either. I've discussed the feasibility of this feature with 'split_rows' with you and Grant so I feel comfortable not making this compatible with 'split_rows'. 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. Done 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 Done 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 'r Done 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 wi I thought about this as well, I was curious why the previous implementation of this method didn't do that since 'EncodeRangeBounds()' does a similar check with 'range_bounds'. I don't see a reason why we couldn't short circuit here but I will test the changes and see. 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 If we get to this point, bound.hash_schemas will be {} so they would be the same thing. I guess it comes down to which is more clear, which is probably {}. Also, even if we don't short circuit, bound.hash_schemas at this point will still be {} since if both 'split_rows' and any vectors within 'range_hash_schema' are defined then an error would be returned before this point. -- 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 23:54:20 +0000 Gerrit-HasComments: Yes
