KUDU-1766: Java client partition pruning with MAX_VALUE equality predicate fails
The bug orginated in a mistransliteration of the c++ partition pruning algorithm; the original version has always had the missing if statement: https://github.com/apache/kudu/blob/53e67e9eb7b4fc940a14a17119041871e80bcc3f/src/kudu/common/key_util.cc#L152-L155 Change-Id: Iad6d00d9f401c510e14709323e7686bfa6d1b449 Reviewed-on: http://gerrit.cloudera.org:8080/5266 Tested-by: Kudu Jenkins Reviewed-by: Adar Dembo <[email protected]> Project: http://git-wip-us.apache.org/repos/asf/kudu/repo Commit: http://git-wip-us.apache.org/repos/asf/kudu/commit/f441b45b Tree: http://git-wip-us.apache.org/repos/asf/kudu/tree/f441b45b Diff: http://git-wip-us.apache.org/repos/asf/kudu/diff/f441b45b Branch: refs/heads/master Commit: f441b45bfb6f34aec7757f3e0fb143c4f9063fe5 Parents: 5de4837 Author: Dan Burkert <[email protected]> Authored: Tue Nov 29 15:24:23 2016 -0800 Committer: Adar Dembo <[email protected]> Committed: Wed Nov 30 02:19:56 2016 +0000 ---------------------------------------------------------------------- .../org/apache/kudu/client/PartitionPruner.java | 5 +++- .../apache/kudu/client/TestPartitionPruner.java | 24 ++++++++++++++++++-- src/kudu/common/partition_pruner-test.cc | 21 +++++++++++++++++ 3 files changed, 47 insertions(+), 3 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/kudu/blob/f441b45b/java/kudu-client/src/main/java/org/apache/kudu/client/PartitionPruner.java ---------------------------------------------------------------------- diff --git a/java/kudu-client/src/main/java/org/apache/kudu/client/PartitionPruner.java b/java/kudu-client/src/main/java/org/apache/kudu/client/PartitionPruner.java index 1535173..e5786f3 100644 --- a/java/kudu-client/src/main/java/org/apache/kudu/client/PartitionPruner.java +++ b/java/kudu-client/src/main/java/org/apache/kudu/client/PartitionPruner.java @@ -476,7 +476,10 @@ public class PartitionPruner { // key to convert it to an exclusive upper bound. if (finalPredicate.getType() == KuduPredicate.PredicateType.EQUALITY || finalPredicate.getType() == KuduPredicate.PredicateType.IN_LIST) { - incrementKey(row, rangePartitionColumnIdxs.subList(0, pushedPredicates)); + // If the increment fails then this bound is is not constraining the keyspace. + if (!incrementKey(row, rangePartitionColumnIdxs.subList(0, pushedPredicates))) { + return AsyncKuduClient.EMPTY_ARRAY; + } } // Step 3: Fill the remaining columns without predicates with the min value. http://git-wip-us.apache.org/repos/asf/kudu/blob/f441b45b/java/kudu-client/src/test/java/org/apache/kudu/client/TestPartitionPruner.java ---------------------------------------------------------------------- diff --git a/java/kudu-client/src/test/java/org/apache/kudu/client/TestPartitionPruner.java b/java/kudu-client/src/test/java/org/apache/kudu/client/TestPartitionPruner.java index c05083b..457457e 100644 --- a/java/kudu-client/src/test/java/org/apache/kudu/client/TestPartitionPruner.java +++ b/java/kudu-client/src/test/java/org/apache/kudu/client/TestPartitionPruner.java @@ -246,8 +246,7 @@ public class TestPartitionPruner extends BaseKuduTest { // CREATE TABLE t // (a INT8, b STRING, c INT8) // PRIMARY KEY (a, b, c)) - // DISTRIBUTE BY RANGE(c, b); - // PARTITION BY RANGE (a, b, c) + // PARTITION BY RANGE (c, b) // (PARTITION VALUES < (0, "m"), // PARTITION (0, "m") <= VALUES < (10, "r") // PARTITION (10, "r") <= VALUES); @@ -292,6 +291,13 @@ public class TestPartitionPruner extends BaseKuduTest { assertEquals(3, countPartitions(table, partitions, KuduPredicate.newComparisonPredicate(c, ComparisonOp.LESS, 100))); + // c < MIN + // TODO(dan): this should prune all partitions. + assertEquals(1, countPartitions(table, partitions, + KuduPredicate.newComparisonPredicate(c, ComparisonOp.LESS, Byte.MIN_VALUE))); + // c < MAX + assertEquals(3, countPartitions(table, partitions, + KuduPredicate.newComparisonPredicate(c, ComparisonOp.LESS, Byte.MAX_VALUE))); // c >= -10 assertEquals(3, countPartitions(table, partitions, @@ -313,6 +319,13 @@ public class TestPartitionPruner extends BaseKuduTest { assertEquals(1, countPartitions(table, partitions, KuduPredicate.newComparisonPredicate(c, ComparisonOp.GREATER_EQUAL, 100))); + // c >= MIN + assertEquals(3, countPartitions(table, partitions, + KuduPredicate.newComparisonPredicate(c, ComparisonOp.GREATER_EQUAL, Byte.MIN_VALUE))); + // c >= MAX + assertEquals(1, countPartitions(table, partitions, + KuduPredicate.newComparisonPredicate(c, ComparisonOp.GREATER_EQUAL, Byte.MAX_VALUE))); + // c >= -10 // c < 0 assertEquals(1, countPartitions(table, partitions, @@ -397,6 +410,13 @@ public class TestPartitionPruner extends BaseKuduTest { KuduPredicate.newComparisonPredicate(c, ComparisonOp.EQUAL, 0), KuduPredicate.newComparisonPredicate(c, ComparisonOp.EQUAL, 2))); + // c = MIN + assertEquals(1, countPartitions(table, partitions, + KuduPredicate.newComparisonPredicate(c, ComparisonOp.EQUAL, Byte.MIN_VALUE))); + // c = MAX + assertEquals(1, countPartitions(table, partitions, + KuduPredicate.newComparisonPredicate(c, ComparisonOp.EQUAL, Byte.MAX_VALUE))); + // a IN (1, 2) assertEquals(1, countPartitions(table, partitions, KuduPredicate.newInListPredicate(c, ImmutableList.of((byte) 1, (byte) 2)))); http://git-wip-us.apache.org/repos/asf/kudu/blob/f441b45b/src/kudu/common/partition_pruner-test.cc ---------------------------------------------------------------------- diff --git a/src/kudu/common/partition_pruner-test.cc b/src/kudu/common/partition_pruner-test.cc index 9c85420..b9c5fa2 100644 --- a/src/kudu/common/partition_pruner-test.cc +++ b/src/kudu/common/partition_pruner-test.cc @@ -327,6 +327,8 @@ TEST(TestPartitionPruner, TestRangePruning) { int8_t five = 5; int8_t ten = 10; int8_t hundred = 100; + int8_t min = INT8_MIN; + int8_t max = INT8_MAX; Slice empty = ""; Slice a = "a"; @@ -350,6 +352,13 @@ TEST(TestPartitionPruner, TestRangePruning) { // c < 100 Check({ ColumnPredicate::Range(schema.column(2), nullptr, &hundred) }, 3); + // c < MIN + // TODO(dan): this should prune all partitions. + Check({ ColumnPredicate::Range(schema.column(2), nullptr, &min) }, 1); + + // c < MAX + Check({ ColumnPredicate::Range(schema.column(2), nullptr, &max) }, 3); + // c >= -10 Check({ ColumnPredicate::Range(schema.column(0), &neg_ten, nullptr) }, 3); @@ -365,6 +374,18 @@ TEST(TestPartitionPruner, TestRangePruning) { // c >= 100 Check({ ColumnPredicate::Range(schema.column(2), &hundred, nullptr) }, 1); + // c >= MIN + Check({ ColumnPredicate::Range(schema.column(2), &min, nullptr) }, 3); + + // c >= MAX + Check({ ColumnPredicate::Range(schema.column(2), &max, nullptr) }, 1); + + // c = MIN + Check({ ColumnPredicate::Equality(schema.column(2), &min) }, 1); + + // c = MAX + Check({ ColumnPredicate::Equality(schema.column(2), &max) }, 1); + // c >= -10 // c < 0 Check({ ColumnPredicate::Range(schema.column(2), &neg_ten, &zero) }, 1);
