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 <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy <[email protected]> Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Fri, 30 Jul 2021 06:26:29 +0000 Gerrit-HasComments: Yes
