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 2: (13 comments) http://gerrit.cloudera.org:8080/#/c/16596/2/src/kudu/common/partition-test.cc File src/kudu/common/partition-test.cc: http://gerrit.cloudera.org:8080/#/c/16596/2/src/kudu/common/partition-test.cc@920 PS2, Line 920: {"first bound", hash_schema_4_buckets}, : {"second bound", vector<PartitionSchema::HashBucketSchema>()}, : {"third bound", hash_schema_2_buckets_by_3} If this patch is not going to use the string bounds, and it's unclear how they will be used in the future, I think we're better off removing them from this patch. http://gerrit.cloudera.org:8080/#/c/16596/2/src/kudu/common/partition-test.cc@931 PS2, Line 931: EXPECT_EQ(0, partitions[0].hash_buckets()[0]); : EXPECT_EQ(string("\0\0\0\0" "a1\0\0\0\0c1", 12),partitions[0].partition_key_start()); : EXPECT_EQ(string("\0\0\0\0" "a2\0\0\0\0c2", 12),partitions[0].partition_key_end()); : : EXPECT_EQ(1, partitions[1].hash_buckets()[0]); : EXPECT_EQ(string("\0\0\0\1" "a1\0\0\0\0c1", 12),partitions[1].partition_key_start()); : EXPECT_EQ(string("\0\0\0\1" "a2\0\0\0\0c2", 12),partitions[1].partition_key_end()); : : EXPECT_EQ(2, partitions[2].hash_buckets()[0]); : EXPECT_EQ(string("\0\0\0\2" "a1\0\0\0\0c1", 12),partitions[2].partition_key_start()); : EXPECT_EQ(string("\0\0\0\2" "a2\0\0\0\0c2", 12),partitions[2].partition_key_end()); : : EXPECT_EQ(3, partitions[3].hash_buckets()[0]); : EXPECT_EQ(string("\0\0\0\3" "a1\0\0\0\0c1", 12),partitions[3].partition_key_start()); : EXPECT_EQ(string("\0\0\0\3" "a2\0\0\0\0c2", 12),partitions[3].partition_key_end()); : : EXPECT_EQ(0, partitions[4].hash_buckets()[0]); : EXPECT_EQ(0, partitions[4].hash_buckets()[1]); : EXPECT_EQ(string("\0\0\0\0" "\0\0\0\0" "a3\0\0b3\0\0", 16),partitions[4].partition_key_start()); : EXPECT_EQ(string("\0\0\0\0" "\0\0\0\0" "a4\0\0b4\0\0", 16),partitions[4].partition_key_end()); : : EXPECT_EQ(0, partitions[5].hash_buckets()[0]); : EXPECT_EQ(1, partitions[5].hash_buckets()[1]); : EXPECT_EQ(string("\0\0\0\0" "\0\0\0\1" "a3\0\0b3\0\0", 16),partitions[5].partition_key_start()); : EXPECT_EQ(string("\0\0\0\0" "\0\0\0\1" "a4\0\0b4\0\0", 16),partitions[5].partition_key_end()); : : EXPECT_EQ(1, partitions[6].hash_buckets()[0]); : EXPECT_EQ(0, partitions[6].hash_buckets()[1]); : EXPECT_EQ(string("\0\0\0\1" "\0\0\0\0" "a3\0\0b3\0\0", 16),partitions[6].partition_key_start()); : EXPECT_EQ(string("\0\0\0\1" "\0\0\0\0" "a4\0\0b4\0\0", 16),partitions[6].partition_key_end()); : : EXPECT_EQ(1, partitions[7].hash_buckets()[0]); : EXPECT_EQ(1, partitions[7].hash_buckets()[1]); : EXPECT_EQ(string("\0\0\0\1" "\0\0\0\1" "a3\0\0b3\0\0", 16),partitions[7].partition_key_start()); : EXPECT_EQ(string("\0\0\0\1" "\0\0\0\1" "a4\0\0b4\0\0", 16),partitions[7].partition_key_end()); : : EXPECT_EQ(2, partitions[8].hash_buckets()[0]); : EXPECT_EQ(0, partitions[8].hash_buckets()[1]); : EXPECT_EQ(string("\0\0\0\2" "\0\0\0\0" "a3\0\0b3\0\0", 16),partitions[8].partition_key_start()); : EXPECT_EQ(string("\0\0\0\2" "\0\0\0\0" "a4\0\0b4\0\0", 16),partitions[8].partition_key_end()); : : EXPECT_EQ(2, partitions[9].hash_buckets()[0]); : EXPECT_EQ(1, partitions[9].hash_buckets()[1]); : EXPECT_EQ(string("\0\0\0\2" "\0\0\0\1" "a3\0\0b3\0\0", 16),partitions[9].partition_key_start()); : EXPECT_EQ(string("\0\0\0\2" "\0\0\0\1" "a4\0\0b4\0\0", 16),partitions[9].partition_key_end()); : : EXPECT_EQ(0, partitions[10].hash_buckets()[0]); : EXPECT_EQ(0, partitions[10].hash_buckets()[1]); : EXPECT_EQ(string("\0\0\0\0" "\0\0\0\0" "a5\0\0b5\0\0", 16),partitions[10].partition_key_start()); : EXPECT_EQ(string("\0\0\0\0" "\0\0\0\0" "a6\0\0\0\0c6", 16),partitions[10].partition_key_end()); : : EXPECT_EQ(0, partitions[11].hash_buckets()[0]); : EXPECT_EQ(1, partitions[11].hash_buckets()[1]); : EXPECT_EQ(string("\0\0\0\0" "\0\0\0\1" "a5\0\0b5\0\0", 16),partitions[11].partition_key_start()); : EXPECT_EQ(string("\0\0\0\0" "\0\0\0\1" "a6\0\0\0\0c6", 16),partitions[11].partition_key_end()); : : EXPECT_EQ(0, partitions[12].hash_buckets()[0]); : EXPECT_EQ(2, partitions[12].hash_buckets()[1]); : EXPECT_EQ(string("\0\0\0\0" "\0\0\0\2" "a5\0\0b5\0\0", 16),partitions[12].partition_key_start()); : EXPECT_EQ(string("\0\0\0\0" "\0\0\0\2" "a6\0\0\0\0c6", 16),partitions[12].partition_key_end()); : : EXPECT_EQ(1, partitions[13].hash_buckets()[0]); : EXPECT_EQ(0, partitions[13].hash_buckets()[1]); : EXPECT_EQ(string("\0\0\0\1" "\0\0\0\0" "a5\0\0b5\0\0", 16),partitions[13].partition_key_start()); : EXPECT_EQ(string("\0\0\0\1" "\0\0\0\0" "a6\0\0\0\0c6", 16),partitions[13].partition_key_end()); : : EXPECT_EQ(1, partitions[14].hash_buckets()[0]); : EXPECT_EQ(1, partitions[14].hash_buckets()[1]); : EXPECT_EQ(string("\0\0\0\1" "\0\0\0\1" "a5\0\0b5\0\0", 16),partitions[14].partition_key_start()); : EXPECT_EQ(string("\0\0\0\1" "\0\0\0\1" "a6\0\0\0\0c6", 16),partitions[14].partition_key_end()); : : EXPECT_EQ(1, partitions[15].hash_buckets()[0]); : EXPECT_EQ(2, partitions[15].hash_buckets()[1]); : EXPECT_EQ(string("\0\0\0\1" "\0\0\0\2" "a5\0\0b5\0\0", 16),partitions[15].partition_key_start()); : EXPECT_EQ(string("\0\0\0\1" "\0\0\0\2" "a6\0\0\0\0c6", 16),partitions[15].partition_key_end()); nit: could this be more succinct with some loops? http://gerrit.cloudera.org:8080/#/c/16596/2/src/kudu/common/partition.h File src/kudu/common/partition.h: http://gerrit.cloudera.org:8080/#/c/16596/2/src/kudu/common/partition.h@149 PS2, Line 149: still working on best way to : // include this bound. nit: If this is the case, could you mention this patch is a WIP in the commit message, and mention what uncertainties there are with this approach? That'd set expectations a bit better for the reviewers and direct our attention better Otherwise, remove this from the comment. http://gerrit.cloudera.org:8080/#/c/16596/2/src/kudu/common/partition.cc File src/kudu/common/partition.cc: http://gerrit.cloudera.org:8080/#/c/16596/2/src/kudu/common/partition.cc@357 PS2, Line 357: Status PartitionSchema::GenerateHashPartitions(const std::vector<HashBucketSchema>& hash_schemas, > warning: method 'GenerateHashPartitions' can be made static [readability-co +1 This could be put in an anonymous namespace http://gerrit.cloudera.org:8080/#/c/16596/2/src/kudu/common/partition.cc@359 PS2, Line 359: std:: nit: don't need "std::" on account of the "using" above. http://gerrit.cloudera.org:8080/#/c/16596/2/src/kudu/common/partition.cc@361 PS2, Line 361: DCHECK(hash_partitions->size() == 1); How about creating the initial vector in this method? That would make it easier to use this function (callers could just pass an empty vector). http://gerrit.cloudera.org:8080/#/c/16596/2/src/kudu/common/partition.cc@369 PS2, Line 369: *hash_partitions nit: see my earlier feedback about using local variables vs output variables. This could probably be a local variable like 'hash_partitions_so_far' or something http://gerrit.cloudera.org:8080/#/c/16596/2/src/kudu/common/partition.cc@378 PS2, Line 378: expected_partitions *= bucket_schema.num_buckets; nit: not sure how valuable this check is, given you can validate it fairly easily by reading the code. I'd feel a bit differently if the code were more complicated, but this seems reasonably easy to check? http://gerrit.cloudera.org:8080/#/c/16596/2/src/kudu/common/partition.cc@379 PS2, Line 379: hash_partitions->swap(new_partitions); nit: we've more recently begun using: *hash_partitions = std::move(new_partitions); instead of swap() http://gerrit.cloudera.org:8080/#/c/16596/2/src/kudu/common/partition.cc@383 PS2, Line 383: return Status::OK(); nit: How about just return the vector<Partition>? or make this a void function, given this never returns an error http://gerrit.cloudera.org:8080/#/c/16596/2/src/kudu/common/partition.cc@386 PS2, Line 386: const vector<KuduPartialRow>& split_rows, Shouldn't we return an error if both this _and_ range_hash_schema are set? http://gerrit.cloudera.org:8080/#/c/16596/2/src/kudu/common/partition.cc@424 PS2, Line 424: [i].second; If range_hash_schema[i].first isn't used, why does it exist at all? Why not just pass in a vector<vector<HashBucketSchema>>? http://gerrit.cloudera.org:8080/#/c/16596/2/src/kudu/common/partition.cc@668 PS2, Line 668: string PartitionSchema::HashSchemaPerPartitionDebugString( nit: Since this is no longer used, could we get rid of it? -- 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: 2 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: Fri, 23 Oct 2020 18:03:32 +0000 Gerrit-HasComments: Yes
