Repository: kudu Updated Branches: refs/heads/master ed048e83a -> 56cc69226
KUDU-1652: Partition pruning fails with IS NOT NULL predicate on PK column Scan optimization could also fail in certain cases, but the fix is the same. Change-Id: Icba2defeffff74e86b0e266e668974bce0ad9b0e Reviewed-on: http://gerrit.cloudera.org:8080/4541 Reviewed-by: Adar Dembo <[email protected]> Tested-by: Kudu Jenkins Reviewed-by: Jean-Daniel Cryans <[email protected]> Project: http://git-wip-us.apache.org/repos/asf/kudu/repo Commit: http://git-wip-us.apache.org/repos/asf/kudu/commit/ce17a9c4 Tree: http://git-wip-us.apache.org/repos/asf/kudu/tree/ce17a9c4 Diff: http://git-wip-us.apache.org/repos/asf/kudu/diff/ce17a9c4 Branch: refs/heads/master Commit: ce17a9c4eb34dcfe63c8d4321d38d18a0cb8c5c2 Parents: ed048e8 Author: Dan Burkert <[email protected]> Authored: Fri Sep 23 16:12:01 2016 -0700 Committer: Dan Burkert <[email protected]> Committed: Tue Sep 27 17:33:14 2016 +0000 ---------------------------------------------------------------------- .../org/apache/kudu/client/KuduPredicate.java | 11 ++-- .../org/apache/kudu/client/PartitionPruner.java | 63 ++++++++++--------- .../org/apache/kudu/client/TestKuduClient.java | 6 ++ src/kudu/common/column_predicate.cc | 1 - src/kudu/common/key_util.cc | 65 ++++++++++++-------- src/kudu/common/partition_pruner-test.cc | 3 + src/kudu/common/scan_spec-test.cc | 11 ++++ 7 files changed, 99 insertions(+), 61 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/kudu/blob/ce17a9c4/java/kudu-client/src/main/java/org/apache/kudu/client/KuduPredicate.java ---------------------------------------------------------------------- diff --git a/java/kudu-client/src/main/java/org/apache/kudu/client/KuduPredicate.java b/java/kudu-client/src/main/java/org/apache/kudu/client/KuduPredicate.java index 76c7594..03a6004 100644 --- a/java/kudu-client/src/main/java/org/apache/kudu/client/KuduPredicate.java +++ b/java/kudu-client/src/main/java/org/apache/kudu/client/KuduPredicate.java @@ -22,6 +22,7 @@ import com.google.common.base.Objects; import com.google.common.base.Preconditions; import com.google.common.primitives.UnsignedBytes; import com.google.protobuf.ByteString; + import org.apache.kudu.ColumnSchema; import org.apache.kudu.Common; import org.apache.kudu.Schema; @@ -467,18 +468,19 @@ public class KuduPredicate { public static KuduPredicate fromPB(Schema schema, Common.ColumnPredicatePB pb) { ColumnSchema column = schema.getColumn(pb.getColumn()); switch (pb.getPredicateCase()) { - case EQUALITY: { + case EQUALITY: return new KuduPredicate(PredicateType.EQUALITY, column, pb.getEquality().getValue().toByteArray(), null); - - } case RANGE: { Common.ColumnPredicatePB.Range range = pb.getRange(); return new KuduPredicate(PredicateType.RANGE, column, range.hasLower() ? range.getLower().toByteArray() : null, range.hasUpper() ? range.getUpper().toByteArray() : null); } - default: throw new IllegalArgumentException("unknown predicate type"); + case IS_NOT_NULL: + return newIsNotNullPredicate(column); + default: + throw new IllegalArgumentException("unknown predicate type"); } } @@ -614,7 +616,6 @@ public class KuduPredicate { } } - /** * Checks that the column is one of the expected types. * @param column the column being checked http://git-wip-us.apache.org/repos/asf/kudu/blob/ce17a9c4/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 2ab5adf..910083f 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 @@ -360,21 +360,24 @@ public class PartitionPruner { // Copy predicates into the row in range partition key column order, // stopping after the first missing predicate. - for (int idx : rangePartitionColumnIdxs) { + loop: for (int idx : rangePartitionColumnIdxs) { ColumnSchema column = schema.getColumnByIndex(idx); KuduPredicate predicate = predicates.get(column.getName()); if (predicate == null) break; - if (predicate.getType() != KuduPredicate.PredicateType.EQUALITY && - predicate.getType() != KuduPredicate.PredicateType.RANGE) { - throw new IllegalArgumentException( - String.format("unexpected predicate type can not be pushed into key: %s", predicate)); + switch (predicate.getType()) { + case RANGE: + if (predicate.getLower() == null) break loop; + // fall through + case EQUALITY: + row.setRaw(idx, predicate.getLower()); + pushedPredicates++; + break; + case IS_NOT_NULL: break loop; + default: + throw new IllegalArgumentException( + String.format("unexpected predicate type can not be pushed into key: %s", predicate)); } - - byte[] value = predicate.getLower(); - if (value == null) break; - row.setRaw(idx, value); - pushedPredicates++; } // If no predicates were pushed, no need to do any more work. @@ -407,32 +410,34 @@ public class PartitionPruner { // Step 1: copy predicates into the row in range partition key column order, stopping after // the first missing predicate. - for (int idx : rangePartitionColumnIdxs) { + loop: for (int idx : rangePartitionColumnIdxs) { ColumnSchema column = schema.getColumnByIndex(idx); KuduPredicate predicate = predicates.get(column.getName()); if (predicate == null) break; - if (predicate.getType() == KuduPredicate.PredicateType.EQUALITY) { - byte[] value = predicate.getLower(); - row.setRaw(idx, value); - pushedPredicates++; - finalPredicate = predicate; - } else if (predicate.getType() == KuduPredicate.PredicateType.RANGE) { - - if (predicate.getUpper() != null) { - row.setRaw(idx, predicate.getUpper()); + switch (predicate.getType()) { + case EQUALITY: + row.setRaw(idx, predicate.getLower()); pushedPredicates++; finalPredicate = predicate; - } + break; + case RANGE: + if (predicate.getUpper() != null) { + row.setRaw(idx, predicate.getUpper()); + pushedPredicates++; + finalPredicate = predicate; + } - // After the first column with a range constraint we stop pushing - // constraints into the upper bound. Instead, we push minimum values - // to the remaining columns (below), which is the maximally tight - // constraint. - break; - } else { - throw new IllegalArgumentException( - String.format("unexpected predicate type can not be pushed into key: %s", predicate)); + // After the first column with a range constraint we stop pushing + // constraints into the upper bound. Instead, we push minimum values + // to the remaining columns (below), which is the maximally tight + // constraint. + break loop; + case IS_NOT_NULL: + break loop; + default: + throw new IllegalArgumentException( + String.format("unexpected predicate type can not be pushed into key: %s", predicate)); } } http://git-wip-us.apache.org/repos/asf/kudu/blob/ce17a9c4/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java ---------------------------------------------------------------------- diff --git a/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java b/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java index 0ae3727..e59ea8e 100644 --- a/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java +++ b/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java @@ -331,6 +331,12 @@ public class TestKuduClient extends BaseKuduTest { KuduPredicate.newComparisonPredicate(schema.getColumn("c2"), GREATER, "c2_30"), KuduPredicate.newComparisonPredicate(schema.getColumn("c2"), LESS, "c2_20") ).size()); + + // IS NOT NULL + assertEquals(100, scanTableToStrings(table, + KuduPredicate.newIsNotNullPredicate(schema.getColumn("c2")), + KuduPredicate.newIsNotNullPredicate(schema.getColumn("key")) + ).size()); } /** http://git-wip-us.apache.org/repos/asf/kudu/blob/ce17a9c4/src/kudu/common/column_predicate.cc ---------------------------------------------------------------------- diff --git a/src/kudu/common/column_predicate.cc b/src/kudu/common/column_predicate.cc index 5dfedbe..229dcd6 100644 --- a/src/kudu/common/column_predicate.cc +++ b/src/kudu/common/column_predicate.cc @@ -108,7 +108,6 @@ ColumnPredicate ColumnPredicate::ExclusiveRange(ColumnSchema column, } ColumnPredicate ColumnPredicate::IsNotNull(ColumnSchema column) { - CHECK(column.is_nullable()); return ColumnPredicate(PredicateType::IsNotNull, move(column), nullptr, nullptr); } http://git-wip-us.apache.org/repos/asf/kudu/blob/ce17a9c4/src/kudu/common/key_util.cc ---------------------------------------------------------------------- diff --git a/src/kudu/common/key_util.cc b/src/kudu/common/key_util.cc index f62caba..357744e 100644 --- a/src/kudu/common/key_util.cc +++ b/src/kudu/common/key_util.cc @@ -146,28 +146,35 @@ int PushUpperBoundKeyPredicates(ColIdxIter first, // Step 1: copy predicates into the row in key column order, stopping after // the first range predicate. - for (auto col_idx_it = first; col_idx_it < last; std::advance(col_idx_it, 1)) { + bool break_loop = false; + for (auto col_idx_it = first; !break_loop && col_idx_it < last; std::advance(col_idx_it, 1)) { const ColumnSchema& column = schema.column(*col_idx_it); const ColumnPredicate* predicate = FindOrNull(predicates, column.name()); if (predicate == nullptr) break; size_t size = column.type_info()->size(); - if (predicate->predicate_type() == PredicateType::Equality) { - memcpy(row->mutable_cell_ptr(*col_idx_it), predicate->raw_lower(), size); - pushed_predicates++; - final_predicate = predicate; - } else if (predicate->predicate_type() == PredicateType::Range) { - if (predicate->raw_upper() != nullptr) { - memcpy(row->mutable_cell_ptr(*col_idx_it), predicate->raw_upper(), size); + switch (predicate->predicate_type()) { + case PredicateType::Equality: + memcpy(row->mutable_cell_ptr(*col_idx_it), predicate->raw_lower(), size); pushed_predicates++; final_predicate = predicate; - } - // After the first column with a range constraint we stop pushing - // constraints into the upper bound. Instead, we push minimum values - // to the remaining columns (below), which is the maximally tight - // constraint. - break; - } else { - LOG(FATAL) << "unexpected predicate type can not be pushed into key"; + break; + case PredicateType::Range: + if (predicate->raw_upper() != nullptr) { + memcpy(row->mutable_cell_ptr(*col_idx_it), predicate->raw_upper(), size); + pushed_predicates++; + final_predicate = predicate; + } + // After the first column with a range constraint we stop pushing + // constraints into the upper bound. Instead, we push minimum values + // to the remaining columns (below), which is the maximally tight + // constraint. + break_loop = true; + break; + case PredicateType::IsNotNull: + break_loop = true; + break; + case PredicateType::None: + LOG(FATAL) << "NONE predicate can not be pushed into key"; } } @@ -200,23 +207,29 @@ int PushLowerBoundKeyPredicates(ColIdxIter first, // Step 1: copy predicates into the row in key column order, stopping after // the first missing predicate. - for (auto col_idx_it = first; col_idx_it < last; std::advance(col_idx_it, 1)) { + bool break_loop = false; + for (auto col_idx_it = first; !break_loop && col_idx_it < last; std::advance(col_idx_it, 1)) { const ColumnSchema& column = schema.column(*col_idx_it); const ColumnPredicate* predicate = FindOrNull(predicates, column.name()); if (predicate == nullptr) break; size_t size = column.type_info()->size(); - if (predicate->predicate_type() == PredicateType::Equality) { - memcpy(row->mutable_cell_ptr(*col_idx_it), predicate->raw_lower(), size); - pushed_predicates++; - } else if (predicate->predicate_type() == PredicateType::Range) { - if (predicate->raw_lower() != nullptr) { + + switch (predicate->predicate_type()) { + case PredicateType::Range: + if (predicate->raw_lower() == nullptr) { + break_loop = true; + break; + } + // Fall through. + case PredicateType::Equality: memcpy(row->mutable_cell_ptr(*col_idx_it), predicate->raw_lower(), size); pushed_predicates++; - } else { break; - } - } else { - LOG(FATAL) << "unexpected predicate type can not be pushed into key"; + case PredicateType::IsNotNull: + break_loop = true; + break; + case PredicateType::None: + LOG(FATAL) << "NONE predicate can not be pushed into key"; } } http://git-wip-us.apache.org/repos/asf/kudu/blob/ce17a9c4/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 889ebaa..9c85420 100644 --- a/src/kudu/common/partition_pruner-test.cc +++ b/src/kudu/common/partition_pruner-test.cc @@ -433,6 +433,9 @@ TEST(TestPartitionPruner, TestRangePruning) { Check({ ColumnPredicate::Equality(schema.column(2), &zero), ColumnPredicate::Range(schema.column(1), nullptr, &m0) }, 2); + + // c IS NOT NULL + Check({ ColumnPredicate::IsNotNull(schema.column(2)) }, 3); } TEST(TestPartitionPruner, TestHashPruning) { http://git-wip-us.apache.org/repos/asf/kudu/blob/ce17a9c4/src/kudu/common/scan_spec-test.cc ---------------------------------------------------------------------- diff --git a/src/kudu/common/scan_spec-test.cc b/src/kudu/common/scan_spec-test.cc index 428d9cd..1bfa404 100644 --- a/src/kudu/common/scan_spec-test.cc +++ b/src/kudu/common/scan_spec-test.cc @@ -309,6 +309,17 @@ TEST_F(CompositeIntKeysTest, TestPredicateOrderDoesntMatter) { spec.ToString(schema_)); } +// Test that IS NOT NULL predicates do *not* get pushed into the primary key +// bounds. This is a regression test for KUDU-1652, where previously attempting +// to push an IS NOT NULL predicate would cause a CHECK failure. +TEST_F(CompositeIntKeysTest, TestIsNotNullPushdown) { + ScanSpec spec; + spec.AddPredicate(ColumnPredicate::IsNotNull(schema_.column(0))); + SCOPED_TRACE(spec.ToString(schema_)); + spec.OptimizeScan(schema_, &arena_, &pool_, true); + EXPECT_EQ("`a` IS NOT NULL", spec.ToString(schema_)); +} + // Tests that a scan spec without primary key bounds will not have predicates // after optimization. TEST_F(CompositeIntKeysTest, TestLiftPrimaryKeyBounds_NoBounds) {
