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 1: (17 comments) http://gerrit.cloudera.org:8080/#/c/16596/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/16596/1//COMMIT_MSG@12 PS1, Line 12: Open to discussion on whether or : not we should support this feature with split_rows. nit: it'd be good to add some context as to what this would entail, what user-facing effects this has, etc. Or outline the problem in the design doc so it can be discussed further. http://gerrit.cloudera.org:8080/#/c/16596/1/src/kudu/common/partition-test.cc File src/kudu/common/partition-test.cc: http://gerrit.cloudera.org:8080/#/c/16596/1/src/kudu/common/partition-test.cc@909 PS1, Line 909: hashSchema1 nit: snake_case, same elsewhere http://gerrit.cloudera.org:8080/#/c/16596/1/src/kudu/common/partition-test.cc@912 PS1, Line 912: hashSchema2.emplace_back(PartitionSchema::HashBucketSchema{{ColumnId(0)}, 2, 0}); : hashSchema2.emplace_back(PartitionSchema::HashBucketSchema{{ColumnId(1)}, 3, 0}); nit: pretty sure you can initialize this via initializer list, e.g. Vector<HashBucketSchema> hash_schema2 = { { { ColumnId(0) }, 2, 0 }, { { ColumnId(1) }, 3, 0 } }; http://gerrit.cloudera.org:8080/#/c/16596/1/src/kudu/common/partition-test.cc@919 PS1, Line 919: 16 nit: could you add a comment explaining this value? May also be able to adjust variable names to make this a bit clearer, e.g. default_hash_buckets_with_3_by_2 = { HashBucketSchema(3), HashBucketSchema(2) }; hash_buckets_with_4 = { HashBucketSchema(4) }; hash_buckets_with_2_by_3 = { HashBucketSchema(2), HashBucketSchema(3) }; At least right now, a reader trying to follow/verify this code would have read between the lines, parse out exactly what ranges have what, and then do the math. http://gerrit.cloudera.org:8080/#/c/16596/1/src/kudu/common/partition-test.cc@921 PS1, Line 921: EXPECT_EQ("RANGE (a, b, c) PARTITION (0, -2147483648, 2) <= VALUES < (1, -2147483648, 3), " : "HASH (a) PARTITION 0", : partition_schema.HashSchemaPerPartitionDebugString : (partitions[0], range_hash_schema[0].second, schema)); : EXPECT_EQ("RANGE (a, b, c) PARTITION (0, -2147483648, 2) <= VALUES < (1, -2147483648, 3), " : "HASH (a) PARTITION 1", : partition_schema.HashSchemaPerPartitionDebugString : (partitions[1], range_hash_schema[0].second, schema)); : EXPECT_EQ("RANGE (a, b, c) PARTITION (0, -2147483648, 2) <= VALUES < (1, -2147483648, 3), " : "HASH (a) PARTITION 2", : partition_schema.HashSchemaPerPartitionDebugString : (partitions[2], range_hash_schema[0].second, schema)); : EXPECT_EQ("RANGE (a, b, c) PARTITION (0, -2147483648, 2) <= VALUES < (1, -2147483648, 3), " : "HASH (a) PARTITION 3", : partition_schema.HashSchemaPerPartitionDebugString : (partitions[3], range_hash_schema[0].second, schema)); : : EXPECT_EQ("RANGE (a, b, c) PARTITION (1, 1, -2147483648) <= VALUES < (2, 3, -2147483648), " : "HASH (a, c) PARTITION 0, HASH (b) PARTITION 0", : partition_schema.HashSchemaPerPartitionDebugString : (partitions[4], range_hash_schema[1].second, schema)); : EXPECT_EQ("RANGE (a, b, c) PARTITION (1, 1, -2147483648) <= VALUES < (2, 3, -2147483648), " : "HASH (a, c) PARTITION 0, HASH (b) PARTITION 1", : partition_schema.HashSchemaPerPartitionDebugString : (partitions[5], range_hash_schema[1].second, schema)); : EXPECT_EQ("RANGE (a, b, c) PARTITION (1, 1, -2147483648) <= VALUES < (2, 3, -2147483648), " : "HASH (a, c) PARTITION 1, HASH (b) PARTITION 0", : partition_schema.HashSchemaPerPartitionDebugString : (partitions[6], range_hash_schema[1].second, schema)); : EXPECT_EQ("RANGE (a, b, c) PARTITION (1, 1, -2147483648) <= VALUES < (2, 3, -2147483648), " : "HASH (a, c) PARTITION 1, HASH (b) PARTITION 1", : partition_schema.HashSchemaPerPartitionDebugString : (partitions[7], range_hash_schema[1].second, schema)); : EXPECT_EQ("RANGE (a, b, c) PARTITION (1, 1, -2147483648) <= VALUES < (2, 3, -2147483648), " : "HASH (a, c) PARTITION 2, HASH (b) PARTITION 0", : partition_schema.HashSchemaPerPartitionDebugString : (partitions[8], range_hash_schema[1].second, schema)); : EXPECT_EQ("RANGE (a, b, c) PARTITION (1, 1, -2147483648) <= VALUES < (2, 3, -2147483648), " : "HASH (a, c) PARTITION 2, HASH (b) PARTITION 1", : partition_schema.HashSchemaPerPartitionDebugString : (partitions[9], range_hash_schema[1].second, schema)); : : EXPECT_EQ("RANGE (a, b, c) PARTITION (3, -2147483648, -2147483648) <= " : "VALUES < (4, -2147483648, 1), HASH (a) PARTITION 0, HASH (b) PARTITION 0", : partition_schema.HashSchemaPerPartitionDebugString : (partitions[10], range_hash_schema[2].second, schema)); : EXPECT_EQ("RANGE (a, b, c) PARTITION (3, -2147483648, -2147483648) <= " : "VALUES < (4, -2147483648, 1), HASH (a) PARTITION 0, HASH (b) PARTITION 1", : partition_schema.HashSchemaPerPartitionDebugString : (partitions[11], range_hash_schema[2].second, schema)); : EXPECT_EQ("RANGE (a, b, c) PARTITION (3, -2147483648, -2147483648) <= " : "VALUES < (4, -2147483648, 1), HASH (a) PARTITION 0, HASH (b) PARTITION 2", : partition_schema.HashSchemaPerPartitionDebugString : (partitions[12], range_hash_schema[2].second, schema)); : EXPECT_EQ("RANGE (a, b, c) PARTITION (3, -2147483648, -2147483648) <= " : "VALUES < (4, -2147483648, 1), HASH (a) PARTITION 1, HASH (b) PARTITION 0", : partition_schema.HashSchemaPerPartitionDebugString : (partitions[13], range_hash_schema[2].second, schema)); : EXPECT_EQ("RANGE (a, b, c) PARTITION (3, -2147483648, -2147483648) <= " : "VALUES < (4, -2147483648, 1), HASH (a) PARTITION 1, HASH (b) PARTITION 1", : partition_schema.HashSchemaPerPartitionDebugString : (partitions[14], range_hash_schema[2].second, schema)); : EXPECT_EQ("RANGE (a, b, c) PARTITION (3, -2147483648, -2147483648) <= " : "VALUES < (4, -2147483648, 1), HASH (a) PARTITION 1, HASH (b) PARTITION 2", : partition_schema.HashSchemaPerPartitionDebugString : (partitions[15], range_hash_schema[2].second, schema)); While this is pretty exhaustive, it does seem a bit excessive. Could we instead just iterate through 'partitions' and check that the partitions of specific ranges have the expected hash bucket schemas? I'd prefer a test that validated Partition directly, so there's no chance that HashSchemaPerPartitionDebugString is hiding issues. HashSchemaPerPartitionDebugString is also brand new code and if we want to keep it, it should have smaller, more targeted tests. I'm not convinced it's worth keeping given it's used in test only right now though. http://gerrit.cloudera.org:8080/#/c/16596/1/src/kudu/common/partition.h File src/kudu/common/partition.h: http://gerrit.cloudera.org:8080/#/c/16596/1/src/kudu/common/partition.h@149 PS1, Line 149: Maps Haven't looked around at how this is used, but should this actually be an unordered_map<int, vector<HashBucketSchema>> or somesuch? http://gerrit.cloudera.org:8080/#/c/16596/1/src/kudu/common/partition.h@149 PS1, Line 149: integer What is this integer? How do we use it? http://gerrit.cloudera.org:8080/#/c/16596/1/src/kudu/common/partition.h@177 PS1, Line 177: range_hash_schema nit: add a comment on how developers should use this, what invariants exist with respect to 'split_rows' and 'range_bounds', how it affects the creation of partitions, etc. http://gerrit.cloudera.org:8080/#/c/16596/1/src/kudu/common/partition.h@389 PS1, Line 389: std::vector<HashBucketSchema> nit: I wonder if it makes sense to typedef vector<HashBucketSchema> as some HashBucketSchemas (plural!) or somesuch, that would depict all hashes associated with a given range. Then there would be a default HashBucketSpec for a table, and we would pass around a vector<HashBucketSpec> when defining the hashes for ranges in CreatePartitions(). http://gerrit.cloudera.org:8080/#/c/16596/1/src/kudu/common/partition.cc File src/kudu/common/partition.cc: http://gerrit.cloudera.org:8080/#/c/16596/1/src/kudu/common/partition.cc@366 PS1, Line 366: partitions nit: not your fault, but IMO it makes it more difficult to read to use 'partitions' for a bunch of different things as we are here. Could we instead make a local variable vector<Partition> along the lines of: vector<Partition> default_hash_partitions(1); for (... In general, unless there's good reason to, I'm not a huge fan of using the output variable as a replacement local variable. How about instead, defining appropriate local variables (e.g. default_hash_partitions, hashed_range_partitions, etc.), and only once we're done and know we are going to return OK, set 'partitions'. http://gerrit.cloudera.org:8080/#/c/16596/1/src/kudu/common/partition.cc@398 PS1, Line 398: RETURN_NOT_OK(EncodeRangeBounds(range_bounds, schema, &bounds)); : RETURN_NOT_OK(EncodeRangeSplits(split_rows, schema, &splits)); : RETURN_NOT_OK(SplitRangeBounds(schema, std::move(splits), &bounds)); If 'bounds' gets populated here, how are callers expected to know how many elements 'range_hash_schema' should have? I think the answer is that it depends on whether we have 'split_rows' or not. If so, it'd be nice to comment here or add context somewhere to further discussion. http://gerrit.cloudera.org:8080/#/c/16596/1/src/kudu/common/partition.cc@403 PS1, Line 403: // Hash schema per range cannot be applied to split rows (doing bare minimum first) nit: some of these sentences are fragments. Mind filling them out and adding punctuation? http://gerrit.cloudera.org:8080/#/c/16596/1/src/kudu/common/partition.cc@405 PS1, Line 405: DCHECK(range_hash_schema.size() == bounds.size()); nit: you can use DCHECK_EQ(expected, actual) for a more useful error message upon failure (it'll print what the values were) http://gerrit.cloudera.org:8080/#/c/16596/1/src/kudu/common/partition.cc@413 PS1, Line 413: second Is 'first' ever used? If not, why not just have this be vector<vector<HashBucketSchema>>? http://gerrit.cloudera.org:8080/#/c/16596/1/src/kudu/common/partition.cc@417 PS1, Line 417: RangeHashSchema nit: variable names should be snake_case unless they're const static http://gerrit.cloudera.org:8080/#/c/16596/1/src/kudu/common/partition.cc@419 PS1, Line 419: : nit: not sure it's explicitly mentioned in the style guide, but most examples, and most of this codebase abides by having a space after the element's variable name, i.e. for (const auto& foo : foos) { ... Same elsewhere. http://gerrit.cloudera.org:8080/#/c/16596/1/src/kudu/common/partition.cc@419 PS1, Line 419: for (const HashBucketSchema& bucket_schema: RangeHashSchema) { : vector<Partition> new_partitions; : // Goes over existing partitions for current bound to generate all combinations if : // multiple schemas exist : for (const Partition& base_partition: current_bound_hash_partitions) { : for (int32_t bucket = 0; bucket < bucket_schema.num_buckets; bucket++) { : Partition partition = base_partition; : partition.hash_buckets_.push_back(bucket); : hash_encoder.Encode(&bucket, &partition.partition_key_start_); : hash_encoder.Encode(&bucket, &partition.partition_key_end_); : new_partitions.push_back(partition); : } : } : current_bound_hash_partitions.swap(new_partitions); : } nit: this seems to be a copy of the logic at L367, and it seems we define some reusable GenerateHashPartitions() function that takes in a vector<HashBucketSchema> and returns a vector<Partition>. -- 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: 1 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-Comment-Date: Wed, 14 Oct 2020 02:30:12 +0000 Gerrit-HasComments: Yes
