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 1: (16 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 us Done 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 Done 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. Done 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 adj Done 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 ins Makes sense, I'll just iterate through 'partitions' and check each individual partition 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 u Maybe I shouldn't have said "maps", but I suppose if the vector<HashBucketSchema> is mapped to the range's upper and lower bounds then we could use an unordered_map. 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? right now it's useless, but the idea was to replace the integer with the upper and lower bound for a specific range 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 Done 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 'par makes sense, I can change that 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 If we don't have any 'split_rows', then the number of bounds would be known before this point, they just get encoded here. It gets tricky if we include both 'splits_rows' and 'bounds' for range specific hash partitioning because then it's more unclear to define how many elements 'range_hash_schema' should have and what bounds to map the schemas to. 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 addin Done 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 messag Done 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<HashB answered in definition of 'range_hash_schema' in header file 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 Done 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 exampl Done 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 s yep I noticed that as well, I can refactor this by creating a helper function. -- 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 06:02:52 +0000 Gerrit-HasComments: Yes
