Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/17643 )
Change subject: [pruning] wip KUDU-2671: Modifies pruning logic to include per range hash schemas. ...................................................................... Patch Set 1: (20 comments) Only briefly looked at the tests to suggest some stylistic changes. http://gerrit.cloudera.org:8080/#/c/17643/1/src/kudu/common/partition.h File src/kudu/common/partition.h: http://gerrit.cloudera.org:8080/#/c/17643/1/src/kudu/common/partition.h@286 PS1, Line 286: nit: spacing http://gerrit.cloudera.org:8080/#/c/17643/1/src/kudu/common/partition.cc File src/kudu/common/partition.cc: http://gerrit.cloudera.org:8080/#/c/17643/1/src/kudu/common/partition.cc@1108 PS1, Line 1108: : bool PartitionSchema::EqualHashBucketSchemas(const HashBucketSchemas& hb_schemas, : const HashBucketSchemas& rhs_hb_schemas) { Seems like this could be separated out from the pruning-related bits of this patch into its own patch with its own tests. http://gerrit.cloudera.org:8080/#/c/17643/1/src/kudu/common/partition.cc@1149 PS1, Line 1149: rh nit: spacing http://gerrit.cloudera.org:8080/#/c/17643/1/src/kudu/common/partition_pruner-test.cc File src/kudu/common/partition_pruner-test.cc: http://gerrit.cloudera.org:8080/#/c/17643/1/src/kudu/common/partition_pruner-test.cc@1057 PS1, Line 1057: // CREATE TABLE t : // (a INT8, b INT8, c STRING) : // PRIMARY KEY (a, b, c) : // PARTITION BY RANGE (c) : // DISTRIBUTE BY HASH(a) INTO 2 BUCKETS : // HASH(b) INTO 2 BUCKETS; : Schema schema({ ColumnSchema("a", INT8), : ColumnSchema("b", INT8), : ColumnSchema("c", STRING) }, : { ColumnId(0), ColumnId(1), ColumnId(2) }, : 3); : Arena arena(1024); : : PartitionSchema partition_schema; : auto pb = PartitionSchemaPB(); : auto range_schema = pb.mutable_range_schema(); : range_schema->add_columns()->set_name("c"); : : auto hash_schema_component = pb.add_hash_bucket_schemas(); : hash_schema_component->add_columns()->set_name("a"); : hash_schema_component->set_num_buckets(2); : auto hash_schema_component_1 = pb.add_hash_bucket_schemas(); : hash_schema_component_1->add_columns()->set_name("b"); : hash_schema_component_1->set_num_buckets(2); nit: just looking for ways to reuse some code. May be worth defining a helper function like: typedef pair<string, int> HashColumnAndNumBuckets; typedef vector<string> RangeBoundValues; Status CreatePartitionSchema( { ColumnSchema("a", INT8), ColumnSchema("b", INT8), ColumnSchema("c", STRING) }, /*range_columns*/{ "c" }, /*table_hash_schema*/{ { "a", 2 }, { "b", 2 } }, // vector<HashColumnAndNumBuckets> &schema, &partition_schema_pb); That way, we get to reuse a bunch of code across these tests, and when CreatePartitions() is updated, we can focus on updating that method once, rather than updating each of the tests. http://gerrit.cloudera.org:8080/#/c/17643/1/src/kudu/common/partition_pruner-test.cc@1095 PS1, Line 1095: ASSERT_OK(lower.SetStringCopy("c", "a")); : ASSERT_OK(upper.SetStringCopy("c", "c")); nit: to avoid ambiguity in reading this, could we perhaps uppercase the column names? Or use 'col1', 'col2', 'col3', etc.? http://gerrit.cloudera.org:8080/#/c/17643/1/src/kudu/common/partition_pruner-test.cc@1155 PS1, Line 1155: RowOperationsPBEncoder encoder(pb.add_range_bounds()); : KuduPartialRow lower(&schema); : KuduPartialRow upper(&schema); : ASSERT_OK(lower.SetStringCopy("c", "k")); : ASSERT_OK(upper.SetStringCopy("c", "m")); : encoder.Add(RowOperationsPB::RANGE_LOWER_BOUND, lower); : encoder.Add(RowOperationsPB::RANGE_UPPER_BOUND, upper); : : auto range_hash_component = pb.add_range_hash_schemas(); : auto hash_component = range_hash_component->add_hash_schemas(); : hash_component->add_columns()->set_name("b"); : hash_component->set_num_buckets(2); : PartitionSchema::HashBucketSchemas hash_schema_2_buckets = {{{ColumnId(1)}, 2, 0}}; nit: may be worth adding some helper to modify a partition schema? Maybe something like: void AddRangePartitionWithSchema(schema, /*lower_string_cols*/{ { "c", "k" } }, /*upper_string_cols*/{ { "c", "m" } }, /*lower_int_cols*/ {}, /*upper_int_cols*/ {}, /*hash_schemas*/ { { "b", 2 } }, &partition_schema_pb); Suggesting because I think a shorter declaration of the ranges would it'd make it easier to go through the test cases below and understand what the bounds are. http://gerrit.cloudera.org:8080/#/c/17643/1/src/kudu/common/partition_pruner-test.cc@1183 PS1, Line 1183: const string& lower_bound_partition_key, : const string& upper_bound_partition_key, Just curious, is setting these different from setting range predicates on the partition key? http://gerrit.cloudera.org:8080/#/c/17643/1/src/kudu/common/partition_pruner-test.cc@1211 PS1, Line 1211: 1 nit: spacing http://gerrit.cloudera.org:8080/#/c/17643/1/src/kudu/common/partition_pruner-test.cc@1312 PS1, Line 1312: Arena arena(1024); nit: move this down closer to where it's used. That way it's less distracting. http://gerrit.cloudera.org:8080/#/c/17643/1/src/kudu/common/partition_pruner.h File src/kudu/common/partition_pruner.h: http://gerrit.cloudera.org:8080/#/c/17643/1/src/kudu/common/partition_pruner.h@99 PS1, Line 99: std::vector<std::pair< : std::pair<std::string, std::string>, : std::vector<std::tuple<std::string, std::string>>>> nit: this might be easier to parse as: typedef pair<string, string> LowerUpperRangeBounds; typedef tuple<string, string> LowerUpperPartitionKeyBounds; typedef pair<LowerUpperRangeBounds, vector<LowerUpperPartitionKeyBounds>> RangeBoundsAndPartitionKeyBounds; vector<RangeBoundsAndPartitionKeyBounds> partition_key_ranges; Some of these could even be used in ConstructPartitionKeyRanges(), and throughout partition_pruner.cc e.g. void ConstructPartitionKeyRanges( const Schema& schema, const PartitionSchema& partition_schema, const ScanSpec& scan_spec, const LowerUpperRangeBounds& range_bounds, vector<LowerUpperPartitionKeyBounds>* partition_key_bounds); http://gerrit.cloudera.org:8080/#/c/17643/1/src/kudu/common/partition_pruner.cc File src/kudu/common/partition_pruner.cc: http://gerrit.cloudera.org:8080/#/c/17643/1/src/kudu/common/partition_pruner.cc@412 PS1, Line 412: // Step 1: Build the range portion of the partition key. nit: either through a comment or a variable name change (e.g. scan_range_lower_bound), could you make it clear that what we're doing here is setting the upper and lower bounds as specified by the scan? It takes a bit of context pick-up to realize that, and it'd be nice to make it a bit more obvious. http://gerrit.cloudera.org:8080/#/c/17643/1/src/kudu/common/partition_pruner.cc@433 PS1, Line 433: ty()) { nit: would be nice to add a comment describing this block, e.g. something like: "Collect partitions that are relevant to the range bounds specified by predicates in 'scan_spec', keeping track of each range's corresponding hash schemas as appropriate." http://gerrit.cloudera.org:8080/#/c/17643/1/src/kudu/common/partition_pruner.cc@434 PS1, Line 434: vector<const PartitionSchema::HashBucketSchemas *> : hash_schemas nit: when splitting a long declaration, I find it more convenient as a reader to have the type and the variable name on the same line, and the constructor args to be separate. That way, when skimming, all I need is to look at the first line to get the type of the variable. http://gerrit.cloudera.org:8080/#/c/17643/1/src/kudu/common/partition_pruner.cc@435 PS1, Line 435: hash_schemas nit: We seem to be conflating the plurality of hash_schema and hash_schemas, which makes it a bit trickier to read through this code. How about we call this 'hash_schemas_per_range', and be consistent with what it means to refer to something as schemas vs schema? http://gerrit.cloudera.org:8080/#/c/17643/1/src/kudu/common/partition_pruner.cc@438 PS1, Line 438: const PartitionSchema::HashBucketSchemas* hash_schema = &range.hash_schemas; : if (range.hash_schemas.empty()) { : hash_schema = &partition_schema.hash_bucket_schemas_; : } nit: we could simplify this as const auto* hash_schemas = range.hash_schemas.empty() ? &partition_schema.hash_bucket_schemas_ : &range.hash_schemas; ... hash_schemas_per_range[i++] = hash_schemas; Regardless, let's maintain plurality of the name and call it hash_schemas, as it is elsewhere. http://gerrit.cloudera.org:8080/#/c/17643/1/src/kudu/common/partition_pruner.cc@444 PS1, Line 444: [hash_schemas_size++] Why can't we use emplace_back for 'hash_schemas'? http://gerrit.cloudera.org:8080/#/c/17643/1/src/kudu/common/partition_pruner.cc@473 PS1, Line 473: // If both are bounded, and lower is already found and upper not found yet, add hash schema. : if (range_lower_bound < range.lower) { : range_bounds.emplace_back(range.lower, range.upper); : hash_schemas[hash_schemas_size++] = hash_schema; : } I don't quite follow this case. If the scan's lower bound is below the range's lower bound, and the scan's upper bound is also below the range's lower bound, won't this be a false positive? The entire scan falls below the range's lower bound, no? http://gerrit.cloudera.org:8080/#/c/17643/1/src/kudu/common/partition_pruner.cc@463 PS1, Line 463: // Both lower and upper ranges are bounded. : if (range_lower_bound >= range.lower && range_lower_bound < range.upper) { : range_bounds.emplace_back(range.lower, range.upper); : hash_schemas[hash_schemas_size++] = hash_schema; : } : if (range_upper_bound >= range.lower && range_upper_bound < range.upper) { : range_bounds.emplace_back(range.lower, range.upper); : hash_schemas[hash_schemas_size++] = hash_schema; : break; // Assumes ranges are sorted. : } : // If both are bounded, and lower is already found and upper not found yet, add hash schema. : if (range_lower_bound < range.lower) { : range_bounds.emplace_back(range.lower, range.upper); : hash_schemas[hash_schemas_size++] = hash_schema; : } Maybe I'm missing something, but it seems like this code should just be checking whether [scan_lower_bound, scan_upper_bound) intersects with [range.lower, range.upper). Is that the case? If so, I think we could simplify this as: if (range.upper < scan_lower_bound && range.lower > scan_upper_bound) { range_bounds.emplace_back(range.lower, range.upper); hash_schemas[hash_schemas_size++] = hash_schema; } If it's written this way so we can break from the loop, I wonder if we can instead install a check earlier in the loop that checks the scan's lower bound vs the range's upper bound and breaks early or somesuch. http://gerrit.cloudera.org:8080/#/c/17643/1/src/kudu/common/partition_pruner.cc@502 PS1, Line 502: // Step 5: nit: IMO these step counts aren't super useful and are a bit confusing now that the steps are strewn about multiple methods. Maybe just remove them? http://gerrit.cloudera.org:8080/#/c/17643/1/src/kudu/common/partition_pruner.cc@586 PS1, Line 586: get<0>(*range).empty() ? "<start>" : : partition_schema.PartitionKeyDebugString(get<0>(*range), schema), nit: individual values that span more than one line should be indented such that the start of each value aligns. E.g. (with lazy, incorrect syntax) Substitute( "[($0), ($1))", range<0>.empty() ? "start" : PartitionKeyDebugString(range<0>), range<1>.empty() ? "end" : PartitionKeyDebugString(range<1>) That way it's clear to readers what lines are starts of new values vs continuations of the previous lines. -- To view, visit http://gerrit.cloudera.org:8080/17643 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I05c37495430f61a2c6f6012c72251138aee465b7 Gerrit-Change-Number: 17643 Gerrit-PatchSet: 1 Gerrit-Owner: Mahesh Reddy <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy <[email protected]> Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Mon, 05 Jul 2021 02:47:46 +0000 Gerrit-HasComments: Yes
