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 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 t removed for now 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? all the partition keys are different are due to hash values, imo it's more clear this way without 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 comm removed comment, will add WIP context in commit message 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, > +1 changed to static 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. Done 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 ea Done 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 variable Done 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 removed this 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: Done 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 funct Done 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? added Status::InvalidArg() if both are populated 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 Done 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? Done -- 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: Sat, 24 Oct 2020 00:37:31 +0000 Gerrit-HasComments: Yes
