You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Mahesh Reddy (Code Review)" <ge...@cloudera.org> on 2021/07/03 01:01:41 UTC
[kudu-CR] [pruning] wip KUDU-2671: Modifies pruning logic to include per range hash schemas.
Mahesh Reddy has uploaded this change for review. ( http://gerrit.cloudera.org:8080/17643
Change subject: [pruning] wip KUDU-2671: Modifies pruning logic to include per range hash schemas.
......................................................................
[pruning] wip KUDU-2671: Modifies pruning logic to include per range hash schemas.
The PartitionPruner class is responsible for constructing partition key ranges
from the predicates and PK bounds provided by the scan. Currently, when the ranges
are constructed, the same set of hash bucket schemas is used. With this patch,
that will no longer be the case as now ranges are constructed using
PartitionSchema's new field of per range hash schemas.
WIP patch because the functionality of pruning given partition keys
as part of the scan is incomplete. However, according to the client API,
this is unstable and for internal use only.
PartitionPruner::RemovePartitionKeyRange removes ranges using a partition key
so I'll leave this patch as a WIP for the time being as it is a critical function.
Change-Id: I05c37495430f61a2c6f6012c72251138aee465b7
---
M src/kudu/common/partial_row.h
M src/kudu/common/partition.cc
M src/kudu/common/partition.h
M src/kudu/common/partition_pruner-test.cc
M src/kudu/common/partition_pruner.cc
M src/kudu/common/partition_pruner.h
6 files changed, 840 insertions(+), 154 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/43/17643/1
--
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: newchange
Gerrit-Change-Id: I05c37495430f61a2c6f6012c72251138aee465b7
Gerrit-Change-Number: 17643
Gerrit-PatchSet: 1
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
[kudu-CR] [pruning] KUDU-2671: Pruning compatible with custom hash schemas.
Posted by "Mahesh Reddy (Code Review)" <ge...@cloudera.org>.
Mahesh Reddy has posted comments on this change. ( http://gerrit.cloudera.org:8080/17643 )
Change subject: [pruning] KUDU-2671: Pruning compatible with custom hash schemas.
......................................................................
Patch Set 9:
> Uploaded patch set 9.
Addressing build failures from previous patch.
--
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: 9
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Tue, 03 Aug 2021 18:41:21 +0000
Gerrit-HasComments: No
[kudu-CR] [pruning] KUDU-2671: Pruning compatible with custom hash schemas.
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/17643 )
Change subject: [pruning] KUDU-2671: Pruning compatible with custom hash schemas.
......................................................................
Patch Set 6: Verified+1
Test failures were unrelated to this patch.
--
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: 6
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Sat, 31 Jul 2021 00:06:01 +0000
Gerrit-HasComments: No
[kudu-CR] [pruning] KUDU-2671: Pruning compatible with custom hash schemas.
Posted by "Mahesh Reddy (Code Review)" <ge...@cloudera.org>.
Mahesh Reddy has posted comments on this change. ( http://gerrit.cloudera.org:8080/17643 )
Change subject: [pruning] KUDU-2671: Pruning compatible with custom hash schemas.
......................................................................
Patch Set 4:
(23 comments)
http://gerrit.cloudera.org:8080/#/c/17643/3//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/17643/3//COMMIT_MSG@13
PS3, Line 13:
: - Adding predicates (e.g. range/equality)
: - Setting lower and upper bound primary keys
: - Setting lower and upper bound partition keys
:
: This patch introduces changes that make the first two methods
:
> nit: remove the extra newlines between each bullet?
Done
http://gerrit.cloudera.org:8080/#/c/17643/3/src/kudu/common/partition.cc
File src/kudu/common/partition.cc:
http://gerrit.cloudera.org:8080/#/c/17643/3/src/kudu/common/partition.cc@1109
PS3, Line 1109: y_end(),
> nit: parameter's name is different between the declaration and the definiti
Done
http://gerrit.cloudera.org:8080/#/c/17643/3/src/kudu/common/partition.cc@1147
PS3, Line 1147: Status PartitionSchema::EncodeColumns(const ConstContiguousRow& row,
: const vector<ColumnId>& column_ids,
: string* buf) {
: for (int i = 0; i < column_ids.size(); i++) {
: ColumnId column_id = column_ids[i];
:
> +1
Will move these changes to a separate patch.
http://gerrit.cloudera.org:8080/#/c/17643/3/src/kudu/common/partition_pruner-test.cc
File src/kudu/common/partition_pruner-test.cc:
http://gerrit.cloudera.org:8080/#/c/17643/3/src/kudu/common/partition_pruner-test.cc@1056
PS3, Line 1056: .column(0), &a),
> nit: consider a typedef for this? Maybe ColumnNamesNumBucketsAndSeed or som
Done
http://gerrit.cloudera.org:8080/#/c/17643/3/src/kudu/common/partition_pruner-test.cc@1079
PS3, Line 1079: check({ ColumnPredicate::Equality
> nit: the indent for this line is off
Done
http://gerrit.cloudera.org:8080/#/c/17643/3/src/kudu/common/partition_pruner-test.cc@1100
PS3, Line 1100: Part
> nit: for pointers, prefer being explicit and use auto*
Done
http://gerrit.cloudera.org:8080/#/c/17643/3/src/kudu/common/partition_pruner-test.cc@1103
PS3, Line 1103: nge_
> nit: same here
Done
http://gerrit.cloudera.org:8080/#/c/17643/3/src/kudu/common/partition_pruner-test.cc@1103
PS3, Line 1103: chema->add_col
> nit: to avoid any ambiguity with hash_bucket_schemas, maybe call this hash_
Done
http://gerrit.cloudera.org:8080/#/c/17643/3/src/kudu/common/partition_pruner-test.cc@1304
PS3, Line 1304:
: // C < "a"
: check({ ColumnPredicate::Range(schema.column(2), nullptr, &a)}, "", "", 0, 0);
:
: // C >= "m"
: check({ ColumnPredicate::Range(schema.column(2), &m, nullptr)}, "", "", 0, 0);
:
: // partition key >= (hash=1, hash=0)
: check({}, string("\0\0\0\1\0\0\0\0", 8), "", 8, 8);
:
: // partition key
> Do you mind adding a scenario for a scheme like this but with custom hash b
Done
http://gerrit.cloudera.org:8080/#/c/17643/3/src/kudu/common/partition_pruner-test.cc@1326
PS3, Line 1326:
> nit here and below where std::make_pair() is used: would it be enough to ha
Done
http://gerrit.cloudera.org:8080/#/c/17643/3/src/kudu/common/partition_pruner-test.cc@1355
PS3, Line 1355:
> nit: add 'const' and turn into all-lowercase
Done
http://gerrit.cloudera.org:8080/#/c/17643/3/src/kudu/common/partition_pruner-test.cc@1366
PS3, Line 1366:
> Why not to use ASSERT_OK() here?
I just reused the existing test code that uses CHECK_OK(). What's the difference between two?
http://gerrit.cloudera.org:8080/#/c/17643/3/src/kudu/common/partition_pruner-test.cc@1381
PS3, Line 1381: size_
> nit: add NO_FATALS() here?
Done
http://gerrit.cloudera.org:8080/#/c/17643/3/src/kudu/common/partition_pruner-test.cc@1411
PS3, Line 1411: // PK < (2, 2, min)
> Do you mind adding a scenario where the range for the primary key should le
Done
http://gerrit.cloudera.org:8080/#/c/17643/3/src/kudu/common/partition_pruner-test.cc@1428
PS3, Line 1428: int8_t>(4, 4, 0), 7, 7);
> nit: would {columns, 3, 0} be enough here?
nope, have to explicitly use `make_tuple`.
http://gerrit.cloudera.org:8080/#/c/17643/3/src/kudu/common/partition_pruner-test.cc@1478
PS3, Line 1478:
: ASSERT_OK(PartitionSchema::FromPB(pb, schema, &partition_schema));
:
> nit: make these constexpr?
Done
http://gerrit.cloudera.org:8080/#/c/17643/3/src/kudu/common/partition_pruner.h
File src/kudu/common/partition_pruner.h:
http://gerrit.cloudera.org:8080/#/c/17643/3/src/kudu/common/partition_pruner.h@49
PS3, Line 49:
> nit: it should be 4 spaces here per style guide
Done
http://gerrit.cloudera.org:8080/#/c/17643/3/src/kudu/common/partition_pruner.cc
File src/kudu/common/partition_pruner.cc:
http://gerrit.cloudera.org:8080/#/c/17643/3/src/kudu/common/partition_pruner.cc@187
PS3, Line 187: vector<bool> PartitionPruner::PruneHashComponent(
> Can this and ConstructPartitionKeyRanges() be made static? Or put into the
Done
http://gerrit.cloudera.org:8080/#/c/17643/3/src/kudu/common/partition_pruner.cc@229
PS3, Line 229: const Schema& schema,
: const ScanSpec& scan_spec,
: const PartitionSchema::HashBucketSchemas& hash_bucket_schemas,
: const LowerUpperRangeBounds& range_bounds,
: vector<PartitionKeyRange>* partition_key_ranges) {
: // Create the hash bucket portion of the partition key.
> nit: fix the indent misalignment for parameters
Done
http://gerrit.cloudera.org:8080/#/c/17643/3/src/kudu/common/partition_pruner.cc@271
PS3, Line 271: find_if(hash_bucket_bitsets.rbegin(),
> warning: narrowing conversion from 'unsigned long' to signed type 'int' is
Done
http://gerrit.cloudera.org:8080/#/c/17643/3/src/kudu/common/partition_pruner.cc@411
PS3, Line 411: ing sc
> nit: let's stick with imperative tense for in-line code comments. I.e. "Bui
Done
http://gerrit.cloudera.org:8080/#/c/17643/3/src/kudu/common/partition_pruner.cc@435
PS3, Line 435: ConstructPartitionKeyRanges(schema, scan_spec, hash_bu
> nit: could you swap the predicate and corresponding code blocks? It would
Done
http://gerrit.cloudera.org:8080/#/c/17643/3/src/kudu/common/partition_pruner.cc@509
PS3, Line 509: er::NextPa
> nit: since this isn't only used in tests, remove the "ForTests()" suffix
Done
--
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: 4
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Fri, 30 Jul 2021 06:26:29 +0000
Gerrit-HasComments: Yes
[kudu-CR] [pruning] KUDU-2671: Pruning compatible with custom hash schemas.
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/17643 )
Change subject: [pruning] KUDU-2671: Pruning compatible with custom hash schemas.
......................................................................
Patch Set 3:
(9 comments)
http://gerrit.cloudera.org:8080/#/c/17643/3//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/17643/3//COMMIT_MSG@13
PS3, Line 13:
: - Adding predicates (e.g. range/equality)
:
: - Setting lower and upper bound primary keys
:
: - Setting lower and upper bound partition keys
:
nit: remove the extra newlines between each bullet?
http://gerrit.cloudera.org:8080/#/c/17643/3/src/kudu/common/partition.cc
File src/kudu/common/partition.cc:
http://gerrit.cloudera.org:8080/#/c/17643/3/src/kudu/common/partition.cc@1147
PS3, Line 1147: for (int i = 0; i < ranges_with_hash_schemas_.size(); i++) {
: if (!EqualHashBucketSchemas(ranges_with_hash_schemas_[i].hash_schemas,
: rhs.ranges_with_hash_schemas_[i].hash_schemas)) {
: return false;
: }
: }
> In addition, does it make sense to compare the lower and the upper boundari
+1
http://gerrit.cloudera.org:8080/#/c/17643/3/src/kudu/common/partition_pruner-test.cc
File src/kudu/common/partition_pruner-test.cc:
http://gerrit.cloudera.org:8080/#/c/17643/3/src/kudu/common/partition_pruner-test.cc@1056
PS3, Line 1056: tuple<vector<string>, int32_t, uint32_t>
nit: consider a typedef for this? Maybe ColumnNamesNumBucketsAndSeed or something
http://gerrit.cloudera.org:8080/#/c/17643/3/src/kudu/common/partition_pruner-test.cc@1100
PS3, Line 1100: auto
nit: for pointers, prefer being explicit and use auto*
http://gerrit.cloudera.org:8080/#/c/17643/3/src/kudu/common/partition_pruner-test.cc@1103
PS3, Line 1103: hash_component
nit: to avoid any ambiguity with hash_bucket_schemas, maybe call this hash_component_pb so readers can easily infer that one is a protobuf field
http://gerrit.cloudera.org:8080/#/c/17643/3/src/kudu/common/partition_pruner-test.cc@1103
PS3, Line 1103: auto
nit: same here
http://gerrit.cloudera.org:8080/#/c/17643/3/src/kudu/common/partition_pruner.cc
File src/kudu/common/partition_pruner.cc:
http://gerrit.cloudera.org:8080/#/c/17643/3/src/kudu/common/partition_pruner.cc@187
PS3, Line 187: vector<bool> PartitionPruner::PruneHashComponent(
Can this and ConstructPartitionKeyRanges() be made static? Or put into the anonymous namespace?
http://gerrit.cloudera.org:8080/#/c/17643/3/src/kudu/common/partition_pruner.cc@411
PS3, Line 411: Builds
nit: let's stick with imperative tense for in-line code comments. I.e. "Build" instead of "Builds", "Store" instead of "Stores", etc.
http://gerrit.cloudera.org:8080/#/c/17643/3/src/kudu/common/partition_pruner.cc@509
PS3, Line 509: ForTests()
nit: since this isn't only used in tests, remove the "ForTests()" suffix
--
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: 3
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Tue, 27 Jul 2021 23:03:44 +0000
Gerrit-HasComments: Yes
[kudu-CR] [pruning] wip KUDU-2671: Modifies pruning logic to include per range hash schemas.
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
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 <mr...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Mon, 05 Jul 2021 02:47:46 +0000
Gerrit-HasComments: Yes
[kudu-CR] [pruning] KUDU-2671: Pruning compatible with custom hash schemas.
Posted by "Mahesh Reddy (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/17643
to look at the new patch set (#5).
Change subject: [pruning] KUDU-2671: Pruning compatible with custom hash schemas.
......................................................................
[pruning] KUDU-2671: Pruning compatible with custom hash schemas.
This patch introduces changes to the PartitionPruner class
to be compatible with custom hash bucket schemas per range.
There are three ways to set bounds on a scan.
- Adding predicates (e.g. range/equality)
- Setting lower and upper bound primary keys
- Setting lower and upper bound partition keys
This patch introduces changes that make the first two methods
of setting bounds on a scan compatible with custom hash bucket
schemas per range. The last way using partition keys is unstable
and for internal use only. While it's not necessary for the last
way to be compatible with per range hash bucket schemas, the
entire pruning functionality will not be complete until
PartitionPruner::RemovePartitionKeyRange() is modified.
That work will be done in a follow up patch.
Change-Id: I05c37495430f61a2c6f6012c72251138aee465b7
---
M src/kudu/common/partial_row.h
M src/kudu/common/partition_pruner-test.cc
M src/kudu/common/partition_pruner.cc
M src/kudu/common/partition_pruner.h
4 files changed, 918 insertions(+), 373 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/43/17643/5
--
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: newpatchset
Gerrit-Change-Id: I05c37495430f61a2c6f6012c72251138aee465b7
Gerrit-Change-Number: 17643
Gerrit-PatchSet: 5
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [pruning] wip KUDU-2671: Modifies pruning logic to include per range hash schemas.
Posted by "Mahesh Reddy (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/17643
to look at the new patch set (#2).
Change subject: [pruning] wip KUDU-2671: Modifies pruning logic to include per range hash schemas.
......................................................................
[pruning] wip KUDU-2671: Modifies pruning logic to include per range hash schemas.
The PartitionPruner class is responsible for constructing
partition key ranges from the predicates and PK bounds
provided by the scan. Currently, when the ranges are constructed,
the same table-wide set of hash bucket schemas is used for every range.
With this patch, that will no longer be the case as now ranges are
constructed using PartitionSchema's new field of per range hash schemas.
WIP patch because the functionality of pruning given partition keys
as part of the scan is incomplete. However, according to the client API,
this is unstable and for internal use only.
PartitionPruner::RemovePartitionKeyRange removes ranges using a partition key
so I'll leave this patch as a WIP for the time being as it is a critical function.
Change-Id: I05c37495430f61a2c6f6012c72251138aee465b7
---
M src/kudu/common/partial_row.h
M src/kudu/common/partition.cc
M src/kudu/common/partition.h
M src/kudu/common/partition_pruner-test.cc
M src/kudu/common/partition_pruner.cc
M src/kudu/common/partition_pruner.h
6 files changed, 763 insertions(+), 163 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/43/17643/2
--
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: newpatchset
Gerrit-Change-Id: I05c37495430f61a2c6f6012c72251138aee465b7
Gerrit-Change-Number: 17643
Gerrit-PatchSet: 2
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [pruning] KUDU-2671: Pruning compatible with custom hash schemas.
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/17643 )
Change subject: [pruning] KUDU-2671: Pruning compatible with custom hash schemas.
......................................................................
Patch Set 10: Code-Review+2
--
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: 10
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 04 Aug 2021 22:55:08 +0000
Gerrit-HasComments: No
[kudu-CR] [pruning] KUDU-2671: Pruning compatible with custom hash schemas.
Posted by "Mahesh Reddy (Code Review)" <ge...@cloudera.org>.
Mahesh Reddy has posted comments on this change. ( http://gerrit.cloudera.org:8080/17643 )
Change subject: [pruning] KUDU-2671: Pruning compatible with custom hash schemas.
......................................................................
Patch Set 8:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/17643/6/src/kudu/common/partition_pruner-test.cc
File src/kudu/common/partition_pruner-test.cc:
http://gerrit.cloudera.org:8080/#/c/17643/6/src/kudu/common/partition_pruner-test.cc@1490
PS6, Line 1490: check({ ColumnPredicate::Equality(schema.column(1), &zero),
> Yeah, the idea is not to exercise the paths in the ScanSpec::CanShortCircui
Makes sense, I can incorporate this behavior in existing tests.
http://gerrit.cloudera.org:8080/#/c/17643/6/src/kudu/common/partition_pruner-test.cc@1491
PS6, Line 1491: ColumnPredicate::InList(schema.column(2), &C_values) },
> My concern is how the new code works in case of having a single element in
Yep, as shown by TestHashSchemasPerRangePruning where the range schema only consists of a single element of the PK, this works as intended. I added a test anyways that also tests the boundary cases as if the range partitions were created by split rows, but this test is probably redundant.
http://gerrit.cloudera.org:8080/#/c/17643/7/src/kudu/common/partition_pruner.h
File src/kudu/common/partition_pruner.h:
http://gerrit.cloudera.org:8080/#/c/17643/7/src/kudu/common/partition_pruner.h@46
PS7, Line 46:
> nit: can just be
Done
http://gerrit.cloudera.org:8080/#/c/17643/7/src/kudu/common/partition_pruner.h@47
PS7, Line 47: std::string lower;
: std::string upper;
: };
:
: str
> nit: we shouldn't need these -- we should be able to construct via
Have to explicitly use the struct's name, thanks for the pointer.
http://gerrit.cloudera.org:8080/#/c/17643/6/src/kudu/common/partition_pruner.h
File src/kudu/common/partition_pruner.h:
http://gerrit.cloudera.org:8080/#/c/17643/6/src/kudu/common/partition_pruner.h@48
PS6, Line 48: tring upp
> +1 for making RangeBoundsAndPartitionKeyRanges a struct. I guess it's more
Ack
http://gerrit.cloudera.org:8080/#/c/17643/6/src/kudu/common/partition_pruner.h@71
PS6, Line 71: // Returns whether there are more partition key ranges to scan.
: bool HasMorePartitionKeyRanges() const;
:
: // Returns the inclusive lower bound partition key of the next tablet to scan.
: const std::string& N
> Yep, makes sense. I guess we can update the behavior as needed if we find
Ack
http://gerrit.cloudera.org:8080/#/c/17643/7/src/kudu/common/partition_pruner.cc
File src/kudu/common/partition_pruner.cc:
http://gerrit.cloudera.org:8080/#/c/17643/7/src/kudu/common/partition_pruner.cc@533
PS7, Line 533: [range_bounds, partition_key_range]
> nit: FYI starting in C++17 you can use structured bindings, e.g.
Done
--
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: 8
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Tue, 03 Aug 2021 07:18:40 +0000
Gerrit-HasComments: Yes
[kudu-CR] [pruning] KUDU-2671: Pruning compatible with custom hash schemas.
Posted by "Mahesh Reddy (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/17643
to look at the new patch set (#8).
Change subject: [pruning] KUDU-2671: Pruning compatible with custom hash schemas.
......................................................................
[pruning] KUDU-2671: Pruning compatible with custom hash schemas.
This patch introduces changes to the PartitionPruner class
to be compatible with custom hash bucket schemas per range.
There are three ways to set bounds on a scan.
- Adding predicates (e.g. range/equality)
- Setting lower and upper bound primary keys
- Setting lower and upper bound partition keys
This patch introduces changes that make the first two methods
of setting bounds on a scan compatible with custom hash bucket
schemas per range. The last way using partition keys is unstable
and for internal use only. While it's not necessary for the last
way to be compatible with per range hash bucket schemas, the
entire pruning functionality will not be complete until
PartitionPruner::RemovePartitionKeyRange() is modified.
That work will be done in a follow up patch.
Change-Id: I05c37495430f61a2c6f6012c72251138aee465b7
---
M src/kudu/common/partial_row.h
M src/kudu/common/partition_pruner-test.cc
M src/kudu/common/partition_pruner.cc
M src/kudu/common/partition_pruner.h
4 files changed, 1,056 insertions(+), 389 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/43/17643/8
--
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: newpatchset
Gerrit-Change-Id: I05c37495430f61a2c6f6012c72251138aee465b7
Gerrit-Change-Number: 17643
Gerrit-PatchSet: 8
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [pruning] KUDU-2671: Pruning compatible with custom hash schemas.
Posted by "Mahesh Reddy (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/17643
to look at the new patch set (#4).
Change subject: [pruning] KUDU-2671: Pruning compatible with custom hash schemas.
......................................................................
[pruning] KUDU-2671: Pruning compatible with custom hash schemas.
This patch introduces changes to the PartitionPruner class
to be compatible with custom hash bucket schemas per range.
There are three ways to set bounds on a scan.
- Adding predicates (e.g. range/equality)
- Setting lower and upper bound primary keys
- Setting lower and upper bound partition keys
This patch introduces changes that make the first two methods
of setting bounds on a scan compatible with custom hash bucket
schemas per range. The last way using partition keys is unstable
and for internal use only. While it's not necessary for the last
way to be compatible with per range hash bucket schemas, the
entire pruning functionality will not be complete until
PartitionPruner::RemovePartitionKeyRange() is modified.
That work will be done in a follow up patch.
Change-Id: I05c37495430f61a2c6f6012c72251138aee465b7
---
M src/kudu/common/partial_row.h
M src/kudu/common/partition_pruner-test.cc
M src/kudu/common/partition_pruner.cc
M src/kudu/common/partition_pruner.h
4 files changed, 913 insertions(+), 315 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/43/17643/4
--
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: newpatchset
Gerrit-Change-Id: I05c37495430f61a2c6f6012c72251138aee465b7
Gerrit-Change-Number: 17643
Gerrit-PatchSet: 4
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [pruning] wip KUDU-2671: Modifies pruning logic to include per range hash schemas.
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin 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:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/17643/1//COMMIT_MSG
Commit Message:
PS1:
> Took a quick look, but what specifically did you think needs to be changed
I meant a couple of formatting points outlined in https://git-scm.com/book/en/v2/Distributed-Git-Contributing-to-a-Project#_commit_guidelines
There is an example of commit message right there, with its text explaining the principles. Particularly, I meant these two items:
* Striving the summary to be 50 characters or less
* Wrap lines of the description at 72 characters: as you can see, gerrit shows vertical red dotted line at 72 characters for commit description.
--
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 <mr...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Tue, 20 Jul 2021 16:50:47 +0000
Gerrit-HasComments: Yes
[kudu-CR] [pruning] KUDU-2671: Pruning compatible with custom hash schemas.
Posted by "Mahesh Reddy (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/17643
to look at the new patch set (#9).
Change subject: [pruning] KUDU-2671: Pruning compatible with custom hash schemas.
......................................................................
[pruning] KUDU-2671: Pruning compatible with custom hash schemas.
This patch introduces changes to the PartitionPruner class
to be compatible with custom hash bucket schemas per range.
There are three ways to set bounds on a scan.
- Adding predicates (e.g. range/equality)
- Setting lower and upper bound primary keys
- Setting lower and upper bound partition keys
This patch introduces changes that make the first two methods
of setting bounds on a scan compatible with custom hash bucket
schemas per range. The last way using partition keys is unstable
and for internal use only. While it's not necessary for the last
way to be compatible with per range hash bucket schemas, the
entire pruning functionality will not be complete until
PartitionPruner::RemovePartitionKeyRange() is modified.
That work will be done in a follow up patch.
Change-Id: I05c37495430f61a2c6f6012c72251138aee465b7
---
M src/kudu/common/partial_row.h
M src/kudu/common/partition_pruner-test.cc
M src/kudu/common/partition_pruner.cc
M src/kudu/common/partition_pruner.h
4 files changed, 1,056 insertions(+), 389 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/43/17643/9
--
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: newpatchset
Gerrit-Change-Id: I05c37495430f61a2c6f6012c72251138aee465b7
Gerrit-Change-Number: 17643
Gerrit-PatchSet: 9
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [pruning] KUDU-2671: Pruning compatible with custom hash schemas.
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/17643 )
Change subject: [pruning] KUDU-2671: Pruning compatible with custom hash schemas.
......................................................................
Patch Set 7:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/17643/6/src/kudu/common/partition_pruner-test.cc
File src/kudu/common/partition_pruner-test.cc:
http://gerrit.cloudera.org:8080/#/c/17643/6/src/kudu/common/partition_pruner-test.cc@1490
PS6, Line 1490: }
> Custom hash schemas per range shouldn't change how ScanSpec::CanShortCircui
Yeah, the idea is not to exercise the paths in the ScanSpec::CanShortCircuit() function, but rather see how the rest of the code works in case when CanShortCircuit() returns true.
http://gerrit.cloudera.org:8080/#/c/17643/6/src/kudu/common/partition_pruner-test.cc@1491
PS6, Line 1491: } // namespace kudu
> TestHashSchemasPerRangePruning has a PK of (A, B, C) and is range partition
My concern is how the new code works in case of having a single element in a range. E.g., in https://gerrit.cloudera.org/#/c/17643/7/src/kudu/common/partition_pruner.cc@467 there is a code comparing the ranges with strict inequality comparisons -- do those work as intended for a single-element ranges?
http://gerrit.cloudera.org:8080/#/c/17643/6/src/kudu/common/partition_pruner.h
File src/kudu/common/partition_pruner.h:
http://gerrit.cloudera.org:8080/#/c/17643/6/src/kudu/common/partition_pruner.h@48
PS6, Line 48: ounds(std
> Nope, the pairs aren't related. Each pair contains the range bounds and the
+1 for making RangeBoundsAndPartitionKeyRanges a struct. I guess it's more or less acceptable to keep RangeBounds and PartitionKeyRange as pairs/tuples, but having them as structs is better due to the observation pointed by Andrew.
Thank you for making this update!
http://gerrit.cloudera.org:8080/#/c/17643/6/src/kudu/common/partition_pruner.h@71
PS6, Line 71:
: RangeBounds range_bounds;
: std::vector<PartitionKeyRange> partition_key_ranges;
: };
:
> The issue with using the pre-computed result is that PartitionPruner::Remov
Yep, makes sense. I guess we can update the behavior as needed if we find the computations in NumRangesRemaining() are an issue in a hot path, but as of now that would be a premature optimization, indeed.
--
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: 7
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Mon, 02 Aug 2021 22:40:38 +0000
Gerrit-HasComments: Yes
[kudu-CR] [pruning] KUDU-2671: Pruning compatible with custom hash schemas.
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/17643 )
Change subject: [pruning] KUDU-2671: Pruning compatible with custom hash schemas.
......................................................................
Patch Set 3:
(14 comments)
http://gerrit.cloudera.org:8080/#/c/17643/3/src/kudu/common/partition.cc
File src/kudu/common/partition.cc:
http://gerrit.cloudera.org:8080/#/c/17643/3/src/kudu/common/partition.cc@1109
PS3, Line 1109: hb_schemas
nit: parameter's name is different between the declaration and the definition
http://gerrit.cloudera.org:8080/#/c/17643/3/src/kudu/common/partition.cc@1147
PS3, Line 1147: for (int i = 0; i < ranges_with_hash_schemas_.size(); i++) {
: if (!EqualHashBucketSchemas(ranges_with_hash_schemas_[i].hash_schemas,
: rhs.ranges_with_hash_schemas_[i].hash_schemas)) {
: return false;
: }
: }
In addition, does it make sense to compare the lower and the upper boundaries per every pair of corresponding elements in PartitionSchema::ranges_with_hash_schemas_?
Also, it would be great to add a unit test to cover the functionality of PartitionSchema::operator==().
http://gerrit.cloudera.org:8080/#/c/17643/3/src/kudu/common/partition_pruner-test.cc
File src/kudu/common/partition_pruner-test.cc:
http://gerrit.cloudera.org:8080/#/c/17643/3/src/kudu/common/partition_pruner-test.cc@1079
PS3, Line 1079:
nit: the indent for this line is off
Overall, I guess it would be easier to read if it were formatted as:
void X(
const Y& y,
...
size_t z) {
...
}
http://gerrit.cloudera.org:8080/#/c/17643/3/src/kudu/common/partition_pruner-test.cc@1304
PS3, Line 1304: // CREATE TABLE t
: // (a INT8, b INT8, c INT8)
: // PRIMARY KEY (a, b, c)
: // PARTITION BY RANGE(a, b)
:
: // Setup the Schema
: Schema schema({ ColumnSchema("a", INT8),
: ColumnSchema("b", INT8),
: ColumnSchema("c", INT8) },
: { ColumnId(0), ColumnId(1), ColumnId(2) },
: 3);
Do you mind adding a scenario for a scheme like this but with custom hash bucket schemas per range, so it would be 2, 3, 4 buckets per range correspondingly?
http://gerrit.cloudera.org:8080/#/c/17643/3/src/kudu/common/partition_pruner-test.cc@1326
PS3, Line 1326: std::make_pair("a", 0)
nit here and below where std::make_pair() is used: would it be enough to have just {"a", 0} instead of std::make_pair("a", 0)?
http://gerrit.cloudera.org:8080/#/c/17643/3/src/kudu/common/partition_pruner-test.cc@1355
PS3, Line 1355: Check
nit: add 'const' and turn into all-lowercase
http://gerrit.cloudera.org:8080/#/c/17643/3/src/kudu/common/partition_pruner-test.cc@1366
PS3, Line 1366: CHECK_OK
Why not to use ASSERT_OK() here?
http://gerrit.cloudera.org:8080/#/c/17643/3/src/kudu/common/partition_pruner-test.cc@1381
PS3, Line 1381: CheckPrunedPartitions
nit: add NO_FATALS() here?
http://gerrit.cloudera.org:8080/#/c/17643/3/src/kudu/common/partition_pruner-test.cc@1411
PS3, Line 1411: }
Do you mind adding a scenario where the range for the primary key should lead to empty list of partitions to scan?
http://gerrit.cloudera.org:8080/#/c/17643/3/src/kudu/common/partition_pruner-test.cc@1428
PS3, Line 1428: make_tuple(columns, 3, 0)
nit: would {columns, 3, 0} be enough here?
http://gerrit.cloudera.org:8080/#/c/17643/3/src/kudu/common/partition_pruner-test.cc@1478
PS3, Line 1478: int8_t zero = 0;
: int8_t one = 1;
: int8_t eight = 8;
nit: make these constexpr?
http://gerrit.cloudera.org:8080/#/c/17643/3/src/kudu/common/partition_pruner.h
File src/kudu/common/partition_pruner.h:
http://gerrit.cloudera.org:8080/#/c/17643/3/src/kudu/common/partition_pruner.h@49
PS3, Line 49:
nit: it should be 4 spaces here per style guide
http://gerrit.cloudera.org:8080/#/c/17643/3/src/kudu/common/partition_pruner.cc
File src/kudu/common/partition_pruner.cc:
http://gerrit.cloudera.org:8080/#/c/17643/3/src/kudu/common/partition_pruner.cc@229
PS3, Line 229: void PartitionPruner::ConstructPartitionKeyRanges(const Schema& schema,
: const PartitionSchema& partition_schema,
: const ScanSpec& scan_spec,
: const PartitionSchema::HashBucketSchemas& hash_bucket_schemas,
: const LowerUpperRangeBounds& range_bounds,
: vector<PartitionKeyRange>* partition_key_ranges) {
nit: fix the indent misalignment for parameters
http://gerrit.cloudera.org:8080/#/c/17643/3/src/kudu/common/partition_pruner.cc@435
PS3, Line 435: if (!partition_schema.ranges_with_hash_schemas_.empty())
nit: could you swap the predicate and corresponding code blocks? It would be more readable the other way:
if (partition_schema.ranges_with_hash_schemas_.empty()) {
...
}
...
}
--
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: 3
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Fri, 23 Jul 2021 19:27:56 +0000
Gerrit-HasComments: Yes
[kudu-CR] [pruning] KUDU-2671: Pruning compatible with custom hash schemas.
Posted by "Mahesh Reddy (Code Review)" <ge...@cloudera.org>.
Mahesh Reddy has posted comments on this change. ( http://gerrit.cloudera.org:8080/17643 )
Change subject: [pruning] KUDU-2671: Pruning compatible with custom hash schemas.
......................................................................
Patch Set 5:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/17643/3/src/kudu/common/partition_pruner-test.cc
File src/kudu/common/partition_pruner-test.cc:
http://gerrit.cloudera.org:8080/#/c/17643/3/src/kudu/common/partition_pruner-test.cc@1366
PS3, Line 1366:
> Bad asserts result in the test being marked as failed, while a CHECK failur
Thanks for the clarification.
http://gerrit.cloudera.org:8080/#/c/17643/3/src/kudu/common/partition_pruner-test.cc@1428
PS3, Line 1428:
> Just to clarify the suggestion is:
Done
http://gerrit.cloudera.org:8080/#/c/17643/4/src/kudu/common/partition_pruner.cc
File src/kudu/common/partition_pruner.cc:
http://gerrit.cloudera.org:8080/#/c/17643/4/src/kudu/common/partition_pruner.cc@481
PS4, Line 481: auto&
> nit: ampersand should be right after "auto"
Done
--
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: 5
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Fri, 30 Jul 2021 20:25:52 +0000
Gerrit-HasComments: Yes
[kudu-CR] [pruning] KUDU-2671: Pruning compatible with custom hash schemas.
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/17643 )
Change subject: [pruning] KUDU-2671: Pruning compatible with custom hash schemas.
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/17643/2/src/kudu/common/partition_pruner.cc
File src/kudu/common/partition_pruner.cc:
http://gerrit.cloudera.org:8080/#/c/17643/2/src/kudu/common/partition_pruner.cc@435
PS2, Line 435: partition_schema.ranges_with_hash_schemas_.empty()
> Talked about this offline with Alexey a bit, but will put thoughts here for
Yep, the idea is to have a registry of different hash bucket schemas (say, keep the registry as hash dictionary, keying with an integer index, for example), and store the index in the hash bucket schema per partition range.
--
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: 2
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Fri, 23 Jul 2021 17:41:24 +0000
Gerrit-HasComments: Yes
[kudu-CR] [pruning] KUDU-2671: Pruning compatible with custom hash schemas.
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/17643 )
Change subject: [pruning] KUDU-2671: Pruning compatible with custom hash schemas.
......................................................................
Patch Set 6:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/17643/6/src/kudu/common/partition_pruner-test.cc
File src/kudu/common/partition_pruner-test.cc:
http://gerrit.cloudera.org:8080/#/c/17643/6/src/kudu/common/partition_pruner-test.cc@1491
PS6, Line 1491: } // namespace kudu
Also, does it make sense to add a few test cases to cover situations when range partitions consist just of a single element in one of its PK dimension? E.g., for PK (int32, string) have a table range partitioned by int32, and hash bucketed in different number of buckets based on string per range?
--
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: 6
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Sat, 31 Jul 2021 02:23:36 +0000
Gerrit-HasComments: Yes
[kudu-CR] [pruning] KUDU-2671: Pruning compatible with custom hash schemas.
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/17643 )
Change subject: [pruning] KUDU-2671: Pruning compatible with custom hash schemas.
......................................................................
Patch Set 4:
I'll look into the test -- it seems pretty flaky (the test dashboard says 11%!).
--
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: 4
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Fri, 30 Jul 2021 18:59:53 +0000
Gerrit-HasComments: No
[kudu-CR] [pruning] KUDU-2671: Pruning compatible with custom hash schemas.
Posted by "Mahesh Reddy (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/17643
to look at the new patch set (#10).
Change subject: [pruning] KUDU-2671: Pruning compatible with custom hash schemas.
......................................................................
[pruning] KUDU-2671: Pruning compatible with custom hash schemas.
This patch introduces changes to the PartitionPruner class
to be compatible with custom hash bucket schemas per range.
There are three ways to set bounds on a scan.
- Adding predicates (e.g. range/equality)
- Setting lower and upper bound primary keys
- Setting lower and upper bound partition keys
This patch introduces changes that make the first two methods
of setting bounds on a scan compatible with custom hash bucket
schemas per range. The last way using partition keys is unstable
and for internal use only. While it's not necessary for the last
way to be compatible with per range hash bucket schemas, the
entire pruning functionality will not be complete until
PartitionPruner::RemovePartitionKeyRange() is modified.
That work will be done in a follow up patch.
Change-Id: I05c37495430f61a2c6f6012c72251138aee465b7
---
M src/kudu/common/partial_row.h
M src/kudu/common/partition_pruner-test.cc
M src/kudu/common/partition_pruner.cc
M src/kudu/common/partition_pruner.h
4 files changed, 1,051 insertions(+), 389 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/43/17643/10
--
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: newpatchset
Gerrit-Change-Id: I05c37495430f61a2c6f6012c72251138aee465b7
Gerrit-Change-Number: 17643
Gerrit-PatchSet: 10
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [pruning] KUDU-2671: Pruning compatible with custom hash schemas.
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/17643 )
Change subject: [pruning] KUDU-2671: Pruning compatible with custom hash schemas.
......................................................................
Patch Set 9: Code-Review+1
(1 comment)
http://gerrit.cloudera.org:8080/#/c/17643/6/src/kudu/common/partition_pruner-test.cc
File src/kudu/common/partition_pruner-test.cc:
http://gerrit.cloudera.org:8080/#/c/17643/6/src/kudu/common/partition_pruner-test.cc@1491
PS6, Line 1491: ColumnPredicate::InList(schema.column(2), &C_values) },
> Yep, as shown by TestHashSchemasPerRangePruning where the range schema only
Thank you for adding these scenarios!
I apologize for not being crystal clear: I meant to see a test scenario with the following ranges for the range-partion part of the PK (assuming the upper range is exclusive): [0, 1), [1, 2), [2, 3) for INT range key and ['a', 'b'), ['b', 'c'), ['c', 'd') for STRING range key
Do we have a coverage for such an edge case already?
--
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: 9
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 04 Aug 2021 00:49:17 +0000
Gerrit-HasComments: Yes
[kudu-CR] [pruning] KUDU-2671: Pruning compatible with custom hash schemas.
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/17643 )
Change subject: [pruning] KUDU-2671: Pruning compatible with custom hash schemas.
......................................................................
Patch Set 6:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/17643/3/src/kudu/common/partition_pruner-test.cc
File src/kudu/common/partition_pruner-test.cc:
http://gerrit.cloudera.org:8080/#/c/17643/3/src/kudu/common/partition_pruner-test.cc@1366
PS3, Line 1366:
> Bad asserts result in the test being marked as failed, while a CHECK failur
Yep, I agree that ASSERT_OK() is a better choice here.
AFAIK, the only case where resorting to CHECK_OK() is a good option is if ASSERT_OK() isn't working as expected if the code is run by other than the main test thread.
http://gerrit.cloudera.org:8080/#/c/17643/6/src/kudu/common/partition_pruner.h
File src/kudu/common/partition_pruner.h:
http://gerrit.cloudera.org:8080/#/c/17643/6/src/kudu/common/partition_pruner.h@46
PS6, Line 46: LowerUpperRangeBounds
nit: why not just RangeBounds -- could there be any other than lower and upper bounds for a range?
http://gerrit.cloudera.org:8080/#/c/17643/6/src/kudu/common/partition_pruner.h@48
PS6, Line 48: std::pair
What is semantic significance of using std::pair instead of struct here? Does the first pair's component somehow complements the second and vice versa?
http://gerrit.cloudera.org:8080/#/c/17643/6/src/kudu/common/partition_pruner.h@71
PS6, Line 71: size_t num_ranges = 0;
: for (const auto& range: partition_key_ranges_) {
: num_ranges += range.second.size();
: }
: return num_ranges;
I see this method is indirectly called in NextPartitionKey() method, and that would result in n^2 complexity while building, scan tokens. Is it possible to compute this once in Init() method and then use the pre-computed result elsewhere?
--
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: 6
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Sat, 31 Jul 2021 01:18:04 +0000
Gerrit-HasComments: Yes
[kudu-CR] [pruning] KUDU-2671: Pruning compatible with custom hash schemas.
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has removed a vote on this change.
Change subject: [pruning] KUDU-2671: Pruning compatible with custom hash schemas.
......................................................................
Removed Verified-1 by Kudu Jenkins (120)
--
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: deleteVote
Gerrit-Change-Id: I05c37495430f61a2c6f6012c72251138aee465b7
Gerrit-Change-Number: 17643
Gerrit-PatchSet: 4
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [pruning] wip KUDU-2671: Modifies pruning logic to include per range hash schemas.
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin 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:
(10 comments)
Did a quick first pass
http://gerrit.cloudera.org:8080/#/c/17643/1//COMMIT_MSG
Commit Message:
PS1:
nit: it would be nice to reformat this commit description to follow the guideline outlined at https://git-scm.com/book/en/v2/Distributed-Git-Contributing-to-a-Project#_commit_guidelines (refereed from https://kudu.apache.org/docs/contributing.html#_submitting_patches)
http://gerrit.cloudera.org:8080/#/c/17643/1//COMMIT_MSG@11
PS1, Line 11: same set of hash bucket schemas is used
nit: same table-wide set of hash bucket schemas is used for every range
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@285
PS1, Line 285: hb_schemas
nit: if the other parameter named 'rhs_hb_schemas', maybe name this one as 'lhs_hb_schemas'?
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 thi
+1
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@66
PS1, Line 66: ranges
nit: num_ranges ?
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:
+1
I'm also curious why both tuple and pair types is used? Any significance to use std::pair instead of std::tuple for particular types?
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@518
PS1, Line 518: NumRangesRemainingForTests() != 0;
nit: this looks a bit strange to re-use test-only method as a base for regular, non-test-only API
http://gerrit.cloudera.org:8080/#/c/17643/1/src/kudu/common/partition_pruner.cc@523
PS1, Line 523: partition_key_ranges_.size() - 1
Could it happen that partition_key_ranges_ is empty? Maybe, add corresponding CHECK() here?
http://gerrit.cloudera.org:8080/#/c/17643/1/src/kudu/common/partition_pruner.cc@524
PS1, Line 524: partition_key_ranges_[last_range]
nit: BTW, there is std::vector::back() method
http://gerrit.cloudera.org:8080/#/c/17643/1/src/kudu/common/partition_pruner.cc@550
PS1, Line 550: partition.range_key_start();
Would it be possible to use partition_key_start() without wrapping it into a Slice?
--
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 <mr...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Mon, 12 Jul 2021 15:05:38 +0000
Gerrit-HasComments: Yes
[kudu-CR] [pruning] KUDU-2671: Pruning compatible with custom hash schemas.
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/17643 )
Change subject: [pruning] KUDU-2671: Pruning compatible with custom hash schemas.
......................................................................
Patch Set 6:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/17643/6/src/kudu/common/partition_pruner-test.cc
File src/kudu/common/partition_pruner-test.cc:
http://gerrit.cloudera.org:8080/#/c/17643/6/src/kudu/common/partition_pruner-test.cc@1490
PS6, Line 1490: }
I'm not sure whether I saw testcases to cover situations when scan_spec.CanShortCircuit() in the very beginning of the PartitionPruner::Init() method would return 'true' in case of using custom hash bucket schemas per range partition. Does it make sense to add a few to cover the cases to cover various cases when ScanSpec::CanShortCircuit() returns 'true'?
http://gerrit.cloudera.org:8080/#/c/17643/6/src/kudu/common/partition_pruner.cc
File src/kudu/common/partition_pruner.cc:
http://gerrit.cloudera.org:8080/#/c/17643/6/src/kudu/common/partition_pruner.cc@241
PS6, Line 241: ColumnId
nit: could use 'auto' here as well?
http://gerrit.cloudera.org:8080/#/c/17643/6/src/kudu/common/partition_pruner.cc@243
PS6, Line 243: ColumnPredicate *predicate
nit: per code style, the asterisk should stick to the type, not variable
http://gerrit.cloudera.org:8080/#/c/17643/6/src/kudu/common/partition_pruner.cc@521
PS6, Line 521: range
nit: for better readability, maybe name this iterator 'range_it'?
http://gerrit.cloudera.org:8080/#/c/17643/6/src/kudu/common/partition_pruner.cc@521
PS6, Line 521: partition_key_range.second
nit: could establish a reference variable to refer to partition_key_range.second for brevity:
auto& key_range = partition_key_range.second;
http://gerrit.cloudera.org:8080/#/c/17643/6/src/kudu/common/partition_pruner.cc@523
PS6, Line 523: range++
nit: prefer using prefix increment operators -- they might be faster because they don't need to store the intermediate result in a temporary variable (you could check the implementation of prefix and postfix operators for STD containers' iterators)
http://gerrit.cloudera.org:8080/#/c/17643/6/src/kudu/common/partition_pruner.cc@557
PS6, Line 557: &&
nit: move '&&' to the previous line
--
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: 6
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Sat, 31 Jul 2021 02:12:04 +0000
Gerrit-HasComments: Yes
[kudu-CR] [pruning] KUDU-2671: Pruning compatible with custom hash schemas.
Posted by "Mahesh Reddy (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/17643
to look at the new patch set (#6).
Change subject: [pruning] KUDU-2671: Pruning compatible with custom hash schemas.
......................................................................
[pruning] KUDU-2671: Pruning compatible with custom hash schemas.
This patch introduces changes to the PartitionPruner class
to be compatible with custom hash bucket schemas per range.
There are three ways to set bounds on a scan.
- Adding predicates (e.g. range/equality)
- Setting lower and upper bound primary keys
- Setting lower and upper bound partition keys
This patch introduces changes that make the first two methods
of setting bounds on a scan compatible with custom hash bucket
schemas per range. The last way using partition keys is unstable
and for internal use only. While it's not necessary for the last
way to be compatible with per range hash bucket schemas, the
entire pruning functionality will not be complete until
PartitionPruner::RemovePartitionKeyRange() is modified.
That work will be done in a follow up patch.
Change-Id: I05c37495430f61a2c6f6012c72251138aee465b7
---
M src/kudu/common/partial_row.h
M src/kudu/common/partition_pruner-test.cc
M src/kudu/common/partition_pruner.cc
M src/kudu/common/partition_pruner.h
4 files changed, 918 insertions(+), 373 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/43/17643/6
--
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: newpatchset
Gerrit-Change-Id: I05c37495430f61a2c6f6012c72251138aee465b7
Gerrit-Change-Number: 17643
Gerrit-PatchSet: 6
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [pruning] wip KUDU-2671: Modifies pruning logic to include per range hash schemas.
Posted by "Mahesh Reddy (Code Review)" <ge...@cloudera.org>.
Mahesh Reddy 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 2:
(33 comments)
http://gerrit.cloudera.org:8080/#/c/17643/1//COMMIT_MSG
Commit Message:
PS1:
> nit: it would be nice to reformat this commit description to follow the gui
Took a quick look, but what specifically did you think needs to be changed about the commit message's format? I made the lengths of the lines shorter.
http://gerrit.cloudera.org:8080/#/c/17643/1//COMMIT_MSG@11
PS1, Line 11: Currently, when the ranges are constru
> nit: same table-wide set of hash bucket schemas is used for every range
Done
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@285
PS1, Line 285: lhs_hb_sch
> nit: if the other parameter named 'rhs_hb_schemas', maybe name this one as
Done
http://gerrit.cloudera.org:8080/#/c/17643/1/src/kudu/common/partition.h@286
PS1, Line 286:
> nit: spacing
Done
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) {
> +1
It's included in this patch for the time being because I was planning on checking for duplicate hash bucket schemas when constructing partition key ranges to avoid building duplicate ranges.
http://gerrit.cloudera.org:8080/#/c/17643/1/src/kudu/common/partition.cc@1149
PS1, Line 1149: rh
> nit: spacing
Done
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: table_hash_schema,
: PartitionSchemaPB* partition_schema_pb) {
: auto range_schema = partition_schema_pb->mutable_range_schema();
: for (const auto& range_column : range_columns) {
: range_schema->add_columns()->set_name(range_column);
: }
: for (const auto& hash_schema : table_hash_schema) {
: auto hash_schema_component = partition_schema_pb->add_hash_bucket_schemas();
: for (const auto& hash_schema_columns : get<0>(hash_schema)) {
: hash_schema_component->add_columns()->set_name(hash_schema_columns);
: }
: hash_schema_component->set_num_buckets(get<1>(hash_schema));
: hash_schema_component->set_seed(get<2>(hash_schema));
: }
: }
:
: void AddRangePartitionWithSchema(const Schema& schema,
: const vector<pair<string, string>>& lower_string_cols,
: const vector<pair<string, string>>& upper_string_cols,
: const vector<pair<string, int8_t>>& lower_int_cols,
: const vector<pair<string, int8_t>>& upper_int_cols,
: const vector<tuple<vector<string>, int32_t, uint32_t>>&
: hash_schemas,
: vector<pair<K
> nit: just looking for ways to reuse some code. May be worth defining a help
Done
http://gerrit.cloudera.org:8080/#/c/17643/1/src/kudu/common/partition_pruner-test.cc@1095
PS1, Line 1095: for (const auto& bound : upper_int_cols) {
: ASSERT_OK(upper.SetInt8(bound.first, boun
> nit: to avoid ambiguity in reading this, could we perhaps uppercase the col
Done
http://gerrit.cloudera.org:8080/#/c/17643/1/src/kudu/common/partition_pruner-test.cc@1147
PS1, Line 1147: {
> warning: unused variable 'range_hash_component' [clang-diagnostic-unused-va
Done
http://gerrit.cloudera.org:8080/#/c/17643/1/src/kudu/common/partition_pruner-test.cc@1155
PS1, Line 1155: vector<string> columns = {"A"};
: vector<string> columns_1 = {"B"};
: AddRangePartitionWithSchema(schema, {std::make_pair("C", "d")}, {std::make_pair("C", "f")},{},
: {}, {make_tuple(columns, 2, 0), make_tuple(columns_1, 3, 0)},
: &bounds, &range_hash_schemas, &pb);
: }
:
: // [(_, _, h), (_, _, j))
: {
: AddRangePartitionWithSchema(schema, {std::make_pair("C", "h")}, {std::make_pair("C", "j")},
: {}, {}, {}, &bounds, &range_hash_schemas, &pb);
: }
:
> nit: may be worth adding some helper to modify a partition schema? Maybe so
Done
http://gerrit.cloudera.org:8080/#/c/17643/1/src/kudu/common/partition_pruner-test.cc@1183
PS1, Line 1183: // Applies the specified predicates to a scan and checks that the expected
: // number of partitions are pruned.
> Just curious, is setting these different from setting range predicates on t
There are three ways to set bounds on a scan. The first way being adding predicates (e.g. range/equality). The second way being setting lower and upper bound primary keys. The last way is setting lower and upper bound partition keys as shown in this test. However, setting partition keys is considered unstable and is only meant for internal use.
http://gerrit.cloudera.org:8080/#/c/17643/1/src/kudu/common/partition_pruner-test.cc@1211
PS1, Line 1211:
> nit: spacing
Done
http://gerrit.cloudera.org:8080/#/c/17643/1/src/kudu/common/partition_pruner-test.cc@1312
PS1, Line 1312: Co
> nit: move this down closer to where it's used. That way it's less distracti
Done
http://gerrit.cloudera.org:8080/#/c/17643/1/src/kudu/common/partition_pruner-test.cc@1337
PS1, Line 1337:
> warning: Value stored to 'range_hash_component' during its initialization i
Done
http://gerrit.cloudera.org:8080/#/c/17643/1/src/kudu/common/partition_pruner-test.cc@1356
PS1, Line 1356: optional<tuple<int8_t, int8_t, int8_t>> upper,
> warning: Value stored to 'range_hash_component' during its initialization i
Done
http://gerrit.cloudera.org:8080/#/c/17643/1/src/kudu/common/partition_pruner-test.cc@1375
PS1, Line 1375: CHECK_OK(upper_bound.SetInt8("b", get<1>(*upper)));
> warning: unused variable 'range_hash_component' [clang-diagnostic-unused-va
Done
http://gerrit.cloudera.org:8080/#/c/17643/1/src/kudu/common/partition_pruner-test.cc@1506
PS1, Line 1506: 7, 7);
> warning: Value stored to 'range_hash_component' during its initialization i
Done
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@66
PS1, Line 66: s true
> nit: num_ranges ?
Done
http://gerrit.cloudera.org:8080/#/c/17643/1/src/kudu/common/partition_pruner.h@99
PS1, Line 99:
: // A vector of a pair of lower and upper range bounds mapped to a
: // reverse sorted set of partition key ranges. Each
> +1
The tuple type was already in use before my changes, I added a pair of strings for the range bounds mostly because it's slightly easier to access the contents of the pair. If there's a preference for pair or tuple, I wouldn't mind only using one type. That being said, the distinction between the range bounds and partition key ranges will no longer exist if we conform to one type for both. Another alternative is using a struct but that will also be a bit redundant since for both range bounds and partition key ranges it's a pair of strings.
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: // the lower and upper bounds specified by the scan.
> nit: either through a comment or a variable name change (e.g. scan_range_lo
Done
http://gerrit.cloudera.org:8080/#/c/17643/1/src/kudu/common/partition_pruner.cc@433
PS1, Line 433: as if t
> nit: would be nice to add a comment describing this block, e.g. something l
Done
http://gerrit.cloudera.org:8080/#/c/17643/1/src/kudu/common/partition_pruner.cc@435
PS1, Line 435: artition_sch
> nit: We seem to be conflating the plurality of hash_schema and hash_schemas
Done
http://gerrit.cloudera.org:8080/#/c/17643/1/src/kudu/common/partition_pruner.cc@434
PS1, Line 434: the range bounds specified by the scan.
: if (!partition_sch
> nit: when splitting a long declaration, I find it more convenient as a read
Done
http://gerrit.cloudera.org:8080/#/c/17643/1/src/kudu/common/partition_pruner.cc@438
PS1, Line 438: const auto* hash_schemas = range.hash_schemas.empty() ?
: &partition_schema.hash_bucket_schemas_ : &range.hash_schemas;
: // Both lower and upper bounds are unbounded.
: i
> nit: we could simplify this as
Done
http://gerrit.cloudera.org:8080/#/c/17643/1/src/kudu/common/partition_pruner.cc@444
PS1, Line 444:
> Why can't we use emplace_back for 'hash_schemas'?
Done
http://gerrit.cloudera.org:8080/#/c/17643/1/src/kudu/common/partition_pruner.cc@473
PS1, Line 473: vector<PartitionKeyRange> partition_key_ranges(1);
: if (scan_range_lower_bound.empty() && scan_range_upper_bound.empty()) {
: ConstructPartitionKeyRanges(schema, partition_schema, scan_spec,
: *hash_schema, make_pair(range_bounds[i].first,
:
> I don't quite follow this case. If the scan's lower bound is below the rang
Imagine there are 3 range bounds: (0, 10), (10, 20), (20, 30). If the scan's lower range bound is 5 and the scan's upper range bound is 25, then in order to add the hash schemas from the bound (10, 20) we need the last if statement.
In regards to your assumption about the scan's upper bound being below the range's lower bound, it only falls to the last if statement if the scan's upper bound isn't fully contained within the current range we are iterating through.
All that being said, this case is merged with the new simplification of the code.
http://gerrit.cloudera.org:8080/#/c/17643/1/src/kudu/common/partition_pruner.cc@463
PS1, Line 463: range_bounds.emplace_back(range.lower, range.upper);
: hash_schemas_per_range.emplace_back(hash_schemas);
: }
: }
: partition_key_ranges_.resize(hash_schemas_per_range.size());
: DCHECK_EQ(range_bounds.size(), hash_schemas_per_range.size());
: // Constructs partition key ranges from the ranges and their respective hash schemas
: // that fell within the scan's bounds.
: for (int i = 0; i < hash_schemas_per_range.size(); i++) {
: const auto& hash_schema = hash_schemas_per_range[i];
: vector<PartitionKeyRange> partition_key_ranges(1);
: if (scan_range_lower_bound.empty() && scan_range_upper_bound.empty()) {
: ConstructPartitionKeyRanges(schema, partition_schema, scan_spec,
: *hash_schema, make_pair(range_bounds[i].first,
:
> Maybe I'm missing something, but it seems like this code should just be che
Thanks for pointing this out, the simplified code I ended up writing has different equality checks than the one you presented, but it led me down the right path.
http://gerrit.cloudera.org:8080/#/c/17643/1/src/kudu/common/partition_pruner.cc@502
PS1, Line 502: if (!scan_spe
> nit: IMO these step counts aren't super useful and are a bit confusing now
Done
http://gerrit.cloudera.org:8080/#/c/17643/1/src/kudu/common/partition_pruner.cc@518
PS1, Line 518: er_bound.empty()) {
> nit: this looks a bit strange to re-use test-only method as a base for regu
I agree, the original motivation behind the method seems to be for tests only, but I don't see why we couldn't use it for this method. The alternative is just duplicating the code from `NumRangesRemainingForTests()` for this method.
http://gerrit.cloudera.org:8080/#/c/17643/1/src/kudu/common/partition_pruner.cc@523
PS1, Line 523: _key_range : partition_key_range
> Could it happen that partition_key_ranges_ is empty? Maybe, add correspond
There's a CHECK() in the line above that does that.
http://gerrit.cloudera.org:8080/#/c/17643/1/src/kudu/common/partition_pruner.cc@524
PS1, Line 524: nge = partition_key_range.second.
> nit: BTW, there is std::vector::back() method
Done
http://gerrit.cloudera.org:8080/#/c/17643/1/src/kudu/common/partition_pruner.cc@550
PS1, Line 550: _bound(partition_key_range.s
> Would it be possible to use partition_key_start() without wrapping it into
Both the `range_key_start()` and `range_key_end()` methods within the Partition class return a Slice. Maybe I could use the data wrapped within the Slice directly. Any reason why we don't want to use a Slice here?
http://gerrit.cloudera.org:8080/#/c/17643/1/src/kudu/common/partition_pruner.cc@586
PS1, Line 586: pace kudu
:
> nit: individual values that span more than one line should be indented such
Done
--
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: 2
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 15 Jul 2021 05:46:09 +0000
Gerrit-HasComments: Yes
[kudu-CR] [pruning] wip KUDU-2671: Modifies pruning logic to include per range hash schemas.
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin 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 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/17643/2/src/kudu/common/partition_pruner.cc
File src/kudu/common/partition_pruner.cc:
http://gerrit.cloudera.org:8080/#/c/17643/2/src/kudu/common/partition_pruner.cc@435
PS2, Line 435: partition_schema.ranges_with_hash_schemas_.empty()
I see this if/else pattern for custom range bounds in multiple places which use PartitionSchema. This isn't a question related to this particular changelist, but since I started looking into addressing some technical debt accumulated in this area: Why not to switch to always using ranges_with_hash_schemas_ in the new code? In other words, why not to always populate ranges_with_hash_schemas_ with appropriate information, even if using table-wide hash schemas for all its range partitions?
--
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: 2
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Tue, 20 Jul 2021 17:30:39 +0000
Gerrit-HasComments: Yes
[kudu-CR] [pruning] KUDU-2671: Pruning compatible with custom hash schemas.
Posted by "Mahesh Reddy (Code Review)" <ge...@cloudera.org>.
Mahesh Reddy has posted comments on this change. ( http://gerrit.cloudera.org:8080/17643 )
Change subject: [pruning] KUDU-2671: Pruning compatible with custom hash schemas.
......................................................................
Patch Set 10:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/17643/6/src/kudu/common/partition_pruner-test.cc
File src/kudu/common/partition_pruner-test.cc:
http://gerrit.cloudera.org:8080/#/c/17643/6/src/kudu/common/partition_pruner-test.cc@1491
PS6, Line 1491: ColumnPredicate::InList(schema.column(2), &C_values) },
> Thank you for adding these scenarios!
Updated the test I added to include ranges consisting of just a single element.
Ex [0, 1), [1, 2), [2, 3) instead of [0, 2), [2, 4), [4, 6).
--
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: 10
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 04 Aug 2021 02:24:20 +0000
Gerrit-HasComments: Yes
[kudu-CR] [pruning] KUDU-2671: Pruning compatible with custom hash schemas.
Posted by "Mahesh Reddy (Code Review)" <ge...@cloudera.org>.
Mahesh Reddy has posted comments on this change. ( http://gerrit.cloudera.org:8080/17643 )
Change subject: [pruning] KUDU-2671: Pruning compatible with custom hash schemas.
......................................................................
Patch Set 3:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/17643/1//COMMIT_MSG
Commit Message:
PS1:
> I meant a couple of formatting points outlined in https://git-scm.com/book/
Done
http://gerrit.cloudera.org:8080/#/c/17643/2/src/kudu/common/partition_pruner.cc
File src/kudu/common/partition_pruner.cc:
http://gerrit.cloudera.org:8080/#/c/17643/2/src/kudu/common/partition_pruner.cc@435
PS2, Line 435: partition_schema.ranges_with_hash_schemas_.empty()
> I see this if/else pattern for custom range bounds in multiple places which
Talked about this offline with Alexey a bit, but will put thoughts here for all to see.
I introduced the field `ranges_with_hash_schemas_` with the intention that it would only be populated when a user defines custom hash bucket schemas per range. So in the use case where a user defines multiple range bounds and uses the table wide hash schema then `ranges_with_hash_schemas_` would not be populated as `hash_bucket_schemas` holds the table wide schema. If we were to go away from using `hash_bucket_schemas_` and make the usage of `ranges_with_hash_schemas` ubiquitous, in this use case just described each pair of range bounds would have to be stored and the table wide schema even though it's the same hash schema for each range. My main concern here is storing extraneous data.
Basically it comes down to whether or not we want to store per range hash schemas all the time (even if the same hash bucket schema is used for each range). Alexey mentioned using some sort of reference to the hash bucket schema, but I'll let him expand on that if he would like.
--
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: 3
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 22 Jul 2021 00:23:27 +0000
Gerrit-HasComments: Yes
[kudu-CR] [pruning] KUDU-2671: Pruning compatible with custom hash schemas.
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/17643 )
Change subject: [pruning] KUDU-2671: Pruning compatible with custom hash schemas.
......................................................................
Patch Set 7:
(4 comments)
Not a full review yet, just calling out some style/modernization improvements before jumping into the meat of the patch again.
http://gerrit.cloudera.org:8080/#/c/17643/7/src/kudu/common/partition_pruner.h
File src/kudu/common/partition_pruner.h:
http://gerrit.cloudera.org:8080/#/c/17643/7/src/kudu/common/partition_pruner.h@46
PS7, Line 46: RangeBounds() {}
nit: can just be
RangeBounds() = default;
https://stackoverflow.com/questions/13576055/how-is-default-different-from-for-default-constructor-and-destructor mentions some differences between a user-defined empty constructor vs using the default
http://gerrit.cloudera.org:8080/#/c/17643/7/src/kudu/common/partition_pruner.h@47
PS7, Line 47:
: RangeBounds(std::string lower, std::string upper)
: : lower(std::move(lower)),
: upper(std::move(upper)) {
: }
nit: we shouldn't need these -- we should be able to construct via
{ lower, upper }
or at worst,
RangeBounds{ lower, upper }
Same in PartitionKeyRange. Did you run into trouble not having these defined?
http://gerrit.cloudera.org:8080/#/c/17643/6/src/kudu/common/partition_pruner.h
File src/kudu/common/partition_pruner.h:
http://gerrit.cloudera.org:8080/#/c/17643/6/src/kudu/common/partition_pruner.h@48
PS6, Line 48: ounds(std
> Nope, the pairs aren't related. Each pair contains the range bounds and the
+1 for having separate types for separate concerns. Conceptually we shouldn't be able to use range bounds as a partition key bounds, so it's great if we can enforce that via the type system.
http://gerrit.cloudera.org:8080/#/c/17643/7/src/kudu/common/partition_pruner.cc
File src/kudu/common/partition_pruner.cc:
http://gerrit.cloudera.org:8080/#/c/17643/7/src/kudu/common/partition_pruner.cc@533
PS7, Line 533: range_bounds_and_partition_key_range
nit: FYI starting in C++17 you can use structured bindings, e.g.
for (const auto& [range_bounds, partition_key_range] : range_bounds_to_partition_key_ranges_) {
const auto& range_bounds_start = range_bounds.lower;
const auto& range_bounds_end = range_bounds.upper;
...
}
It can be a little finicky around unused variables[1], but it's great when both members of the binding are used.
[1] https://stackoverflow.com/a/40714311
--
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: 7
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Mon, 02 Aug 2021 18:59:54 +0000
Gerrit-HasComments: Yes
[kudu-CR] [pruning] KUDU-2671: Pruning compatible with custom hash schemas.
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has removed a vote on this change.
Change subject: [pruning] KUDU-2671: Pruning compatible with custom hash schemas.
......................................................................
Removed Verified-1 by Kudu Jenkins (120)
--
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: deleteVote
Gerrit-Change-Id: I05c37495430f61a2c6f6012c72251138aee465b7
Gerrit-Change-Number: 17643
Gerrit-PatchSet: 6
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [pruning] KUDU-2671: Pruning compatible with custom hash schemas.
Posted by "Mahesh Reddy (Code Review)" <ge...@cloudera.org>.
Mahesh Reddy has posted comments on this change. ( http://gerrit.cloudera.org:8080/17643 )
Change subject: [pruning] KUDU-2671: Pruning compatible with custom hash schemas.
......................................................................
Patch Set 3:
> Uploaded patch set 3.
Removed WIP label, will move changes needed for full functionality of pruning (mostly due to PartitionPruner::RemovePartitionKeyRange()) to a separate patch.
--
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: 3
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 22 Jul 2021 00:25:14 +0000
Gerrit-HasComments: No
[kudu-CR] [pruning] KUDU-2671: Pruning compatible with custom hash schemas.
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/17643 )
Change subject: [pruning] KUDU-2671: Pruning compatible with custom hash schemas.
......................................................................
Patch Set 10: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/17643/6/src/kudu/common/partition_pruner-test.cc
File src/kudu/common/partition_pruner-test.cc:
http://gerrit.cloudera.org:8080/#/c/17643/6/src/kudu/common/partition_pruner-test.cc@1491
PS6, Line 1491: ColumnPredicate::InList(schema.column(2), &C_values) },
> Updated the test I added to include ranges consisting of just a single elem
Cool, thanks a lot!
--
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: 10
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 04 Aug 2021 02:49:34 +0000
Gerrit-HasComments: Yes
[kudu-CR] [pruning] KUDU-2671: Pruning compatible with custom hash schemas.
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/17643 )
Change subject: [pruning] KUDU-2671: Pruning compatible with custom hash schemas.
......................................................................
[pruning] KUDU-2671: Pruning compatible with custom hash schemas.
This patch introduces changes to the PartitionPruner class
to be compatible with custom hash bucket schemas per range.
There are three ways to set bounds on a scan.
- Adding predicates (e.g. range/equality)
- Setting lower and upper bound primary keys
- Setting lower and upper bound partition keys
This patch introduces changes that make the first two methods
of setting bounds on a scan compatible with custom hash bucket
schemas per range. The last way using partition keys is unstable
and for internal use only. While it's not necessary for the last
way to be compatible with per range hash bucket schemas, the
entire pruning functionality will not be complete until
PartitionPruner::RemovePartitionKeyRange() is modified.
That work will be done in a follow up patch.
Change-Id: I05c37495430f61a2c6f6012c72251138aee465b7
Reviewed-on: http://gerrit.cloudera.org:8080/17643
Reviewed-by: Alexey Serbin <as...@cloudera.com>
Tested-by: Kudu Jenkins
Reviewed-by: Andrew Wong <aw...@cloudera.com>
---
M src/kudu/common/partial_row.h
M src/kudu/common/partition_pruner-test.cc
M src/kudu/common/partition_pruner.cc
M src/kudu/common/partition_pruner.h
4 files changed, 1,051 insertions(+), 389 deletions(-)
Approvals:
Alexey Serbin: Looks good to me, approved
Kudu Jenkins: Verified
Andrew Wong: Looks good to me, approved
--
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: merged
Gerrit-Change-Id: I05c37495430f61a2c6f6012c72251138aee465b7
Gerrit-Change-Number: 17643
Gerrit-PatchSet: 11
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [pruning] KUDU-2671: Pruning compatible with custom hash schemas.
Posted by "Mahesh Reddy (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/17643
to look at the new patch set (#7).
Change subject: [pruning] KUDU-2671: Pruning compatible with custom hash schemas.
......................................................................
[pruning] KUDU-2671: Pruning compatible with custom hash schemas.
This patch introduces changes to the PartitionPruner class
to be compatible with custom hash bucket schemas per range.
There are three ways to set bounds on a scan.
- Adding predicates (e.g. range/equality)
- Setting lower and upper bound primary keys
- Setting lower and upper bound partition keys
This patch introduces changes that make the first two methods
of setting bounds on a scan compatible with custom hash bucket
schemas per range. The last way using partition keys is unstable
and for internal use only. While it's not necessary for the last
way to be compatible with per range hash bucket schemas, the
entire pruning functionality will not be complete until
PartitionPruner::RemovePartitionKeyRange() is modified.
That work will be done in a follow up patch.
Change-Id: I05c37495430f61a2c6f6012c72251138aee465b7
---
M src/kudu/common/partial_row.h
M src/kudu/common/partition_pruner-test.cc
M src/kudu/common/partition_pruner.cc
M src/kudu/common/partition_pruner.h
4 files changed, 953 insertions(+), 386 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/43/17643/7
--
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: newpatchset
Gerrit-Change-Id: I05c37495430f61a2c6f6012c72251138aee465b7
Gerrit-Change-Number: 17643
Gerrit-PatchSet: 7
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [pruning] KUDU-2671: Pruning compatible with custom hash schemas.
Posted by "Mahesh Reddy (Code Review)" <ge...@cloudera.org>.
Mahesh Reddy has posted comments on this change. ( http://gerrit.cloudera.org:8080/17643 )
Change subject: [pruning] KUDU-2671: Pruning compatible with custom hash schemas.
......................................................................
Patch Set 6:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/17643/5/src/kudu/common/partition_pruner.cc
File src/kudu/common/partition_pruner.cc:
http://gerrit.cloudera.org:8080/#/c/17643/5/src/kudu/common/partition_pruner.cc@269
PS5, Line 269: constrained_index = hash_bucket_schemas.size() -
> warning: narrowing conversion from 'unsigned long' to signed type 'int' is
Done
--
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: 6
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Fri, 30 Jul 2021 20:36:51 +0000
Gerrit-HasComments: Yes
[kudu-CR] [pruning] KUDU-2671: Pruning compatible with custom hash schemas.
Posted by "Mahesh Reddy (Code Review)" <ge...@cloudera.org>.
Mahesh Reddy has posted comments on this change. ( http://gerrit.cloudera.org:8080/17643 )
Change subject: [pruning] KUDU-2671: Pruning compatible with custom hash schemas.
......................................................................
Patch Set 7:
(12 comments)
http://gerrit.cloudera.org:8080/#/c/17643/3/src/kudu/common/partition_pruner-test.cc
File src/kudu/common/partition_pruner-test.cc:
http://gerrit.cloudera.org:8080/#/c/17643/3/src/kudu/common/partition_pruner-test.cc@1366
PS3, Line 1366:
> Yep, I agree that ASSERT_OK() is a better choice here.
Ack
http://gerrit.cloudera.org:8080/#/c/17643/6/src/kudu/common/partition_pruner-test.cc
File src/kudu/common/partition_pruner-test.cc:
http://gerrit.cloudera.org:8080/#/c/17643/6/src/kudu/common/partition_pruner-test.cc@1490
PS6, Line 1490: }
> I'm not sure whether I saw testcases to cover situations when scan_spec.Can
Custom hash schemas per range shouldn't change how ScanSpec::CanShortCircuit() functions. When checking if it can bypass future checks, the function checks if the lower bound set is greater than the upper bound set. It also checks for empty predicates or InList predicates where no list is provided. Both of these are independent of custom hash schemas per range so IMO I don't think it's necessary to add a test to cover this case.
http://gerrit.cloudera.org:8080/#/c/17643/6/src/kudu/common/partition_pruner-test.cc@1491
PS6, Line 1491: } // namespace kudu
> Also, does it make sense to add a few test cases to cover situations when r
TestHashSchemasPerRangePruning has a PK of (A, B, C) and is range partitioned on only one column of the PK (C). Is that test case enough coverage or do you think I should add more?
http://gerrit.cloudera.org:8080/#/c/17643/6/src/kudu/common/partition_pruner.h
File src/kudu/common/partition_pruner.h:
http://gerrit.cloudera.org:8080/#/c/17643/6/src/kudu/common/partition_pruner.h@46
PS6, Line 46:
> nit: why not just RangeBounds -- could there be any other than lower and up
nope, will change this to 'RangeBounds'.
http://gerrit.cloudera.org:8080/#/c/17643/6/src/kudu/common/partition_pruner.h@48
PS6, Line 48: ounds(std
> What is semantic significance of using std::pair instead of struct here? D
Nope, the pairs aren't related. Each pair contains the range bounds and the respective partition key ranges. I changed all the typedefs to structs because I thought it made more sense. I'm acutely aware that the struct of 'RangeBounds' and 'PartitionKeyRange' can me merged into one if the field names match, but I'm not sure if it's better to have a separation of concerns or merge them. Please let me know your thoughts on this.
FYI, I needed the default constructors so the newly named field 'range_bounds_to_partition_key_ranges' can be resized.
http://gerrit.cloudera.org:8080/#/c/17643/6/src/kudu/common/partition_pruner.h@71
PS6, Line 71:
: RangeBounds range_bounds;
: std::vector<PartitionKeyRange> partition_key_ranges;
: };
:
> I see this method is indirectly called in NextPartitionKey() method, and th
The issue with using the pre-computed result is that PartitionPruner::RemovePartitionKeyRange potentially modifies the size of the field every iteration, so we need to constantly be checking the size of this field.
http://gerrit.cloudera.org:8080/#/c/17643/6/src/kudu/common/partition_pruner.cc
File src/kudu/common/partition_pruner.cc:
http://gerrit.cloudera.org:8080/#/c/17643/6/src/kudu/common/partition_pruner.cc@241
PS6, Line 241: redicate
> nit: could use 'auto' here as well?
Done
http://gerrit.cloudera.org:8080/#/c/17643/6/src/kudu/common/partition_pruner.cc@243
PS6, Line 243: k;
> nit: per code style, the asterisk should stick to the type, not variable
Done
http://gerrit.cloudera.org:8080/#/c/17643/6/src/kudu/common/partition_pruner.cc@521
PS6, Line 521: ge_it
> nit: for better readability, maybe name this iterator 'range_it'?
Done
http://gerrit.cloudera.org:8080/#/c/17643/6/src/kudu/common/partition_pruner.cc@521
PS6, Line 521:
> nit: could establish a reference variable to refer to partition_key_range.s
Done
http://gerrit.cloudera.org:8080/#/c/17643/6/src/kudu/common/partition_pruner.cc@523
PS6, Line 523: ((*rang
> nit: prefer using prefix increment operators -- they might be faster becaus
Done
http://gerrit.cloudera.org:8080/#/c/17643/6/src/kudu/common/partition_pruner.cc@557
PS6, Line 557:
> nit: move '&&' to the previous line
Done
--
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: 7
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Sat, 31 Jul 2021 09:02:36 +0000
Gerrit-HasComments: Yes
[kudu-CR] [pruning] KUDU-2671: Pruning compatible with custom hash schemas.
Posted by "Mahesh Reddy (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/17643
to look at the new patch set (#3).
Change subject: [pruning] KUDU-2671: Pruning compatible with custom hash schemas.
......................................................................
[pruning] KUDU-2671: Pruning compatible with custom hash schemas.
This patch introduces changes to the PartitionPruner class
to be compatible with custom hash bucket schemas per range.
There are three ways to set bounds on a scan.
- Adding predicates (e.g. range/equality)
- Setting lower and upper bound primary keys
- Setting lower and upper bound partition keys
This patch introduces changes that make the first two methods
of setting bounds on a scan compatible with custom hash bucket
schemas per range. The last way using partition keys is unstable
and for internal use only. While it's not necessary for the last
way to be compatible with per range hash bucket schemas, the
entire pruning functionality will not be complete until
PartitionPruner::RemovePartitionKeyRange() is modified.
That work will be done in a follow up patch.
Change-Id: I05c37495430f61a2c6f6012c72251138aee465b7
---
M src/kudu/common/partial_row.h
M src/kudu/common/partition.cc
M src/kudu/common/partition.h
M src/kudu/common/partition_pruner-test.cc
M src/kudu/common/partition_pruner.cc
M src/kudu/common/partition_pruner.h
6 files changed, 763 insertions(+), 163 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/43/17643/3
--
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: newpatchset
Gerrit-Change-Id: I05c37495430f61a2c6f6012c72251138aee465b7
Gerrit-Change-Number: 17643
Gerrit-PatchSet: 3
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [pruning] KUDU-2671: Pruning compatible with custom hash schemas.
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/17643 )
Change subject: [pruning] KUDU-2671: Pruning compatible with custom hash schemas.
......................................................................
Patch Set 4: Verified+1
(3 comments)
http://gerrit.cloudera.org:8080/#/c/17643/3/src/kudu/common/partition_pruner-test.cc
File src/kudu/common/partition_pruner-test.cc:
http://gerrit.cloudera.org:8080/#/c/17643/3/src/kudu/common/partition_pruner-test.cc@1366
PS3, Line 1366:
> I just reused the existing test code that uses CHECK_OK(). What's the diffe
Bad asserts result in the test being marked as failed, while a CHECK failure results in the process crashing. A failed test is much easier to diagnose and report compared to a crash. And depending on the test, e.g. in a multi-process test like one that uses an ExternalMiniCluster, a crash may not even result in a test failure without explicit checking for crashed processes.
http://gerrit.cloudera.org:8080/#/c/17643/3/src/kudu/common/partition_pruner-test.cc@1428
PS3, Line 1428: int8_t>(4, 4, 0), 7, 7);
> nope, have to explicitly use `make_tuple`.
Just to clarify the suggestion is:
CreatePartitionSchemaPB({"A"}, { { columns, 3, 0 } }, &pb);
or
CreatePartitionSchemaPB({"A"}, { { { "A", "B" }, 3, 0 } }, &pb);
which _should_ work. I think compilers have issues with inferring types for optionals (e.g. in the check() lambdas), but that shouldn't be the case here or with AddRangePartitionWithSchema().
http://gerrit.cloudera.org:8080/#/c/17643/4/src/kudu/common/partition_pruner.cc
File src/kudu/common/partition_pruner.cc:
http://gerrit.cloudera.org:8080/#/c/17643/4/src/kudu/common/partition_pruner.cc@481
PS4, Line 481: auto &
nit: ampersand should be right after "auto"
--
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: 4
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Fri, 30 Jul 2021 18:57:22 +0000
Gerrit-HasComments: Yes