Mailing-List: contact commits-help@kudu.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@kudu.apache.org
Content-Type: text/plain; charset="us-ascii"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
From: danburkert@apache.org
To: commits@kudu.apache.org
Date: Tue, 04 Oct 2016 19:43:35 -0000
Message-Id: <0e7df68ce8c14b219e1e70458b0c5297@git.apache.org>
Subject: [1/2] kudu git commit: KUDU-1652 (part 2): Filter IS NOT NULL
 predicates from scan on client
archived-at: Tue, 04 Oct 2016 19:43:37 -0000

Repository: kudu
Updated Branches:
  refs/heads/master 6d696bcf7 -> 1a60ed7d2


KUDU-1652 (part 2): Filter IS NOT NULL predicates from scan on client

Part 1 fixed the C++ client/server and Java client so that specifying a
an IS NOT NULL predicate on a non-nullable column would not result in a
CHECK failure or exception. This commit filters all such predicates from
new scan requests on the client, in order to avoid crashing server
versions without the part 1 fix (1.0.0 and below).

This is a difficult thing to test, since the underlying issue has
already been fixed, and this just prevents older server version from
crashing. I've manually verified the change by testing this patch with
the additional tests in part 1, but without the fix from part 1.

Change-Id: I542a735ca337ec9ed4eb3c989f8e6fa4ee0cc1da
Reviewed-on: http://gerrit.cloudera.org:8080/4612
Reviewed-by: Jean-Daniel Cryans <jdcryans@apache.org>
Reviewed-by: Adar Dembo <adar@cloudera.com>
Tested-by: Dan Burkert <dan@cloudera.com>


Project: http://git-wip-us.apache.org/repos/asf/kudu/repo
Commit: http://git-wip-us.apache.org/repos/asf/kudu/commit/624a88b1
Tree: http://git-wip-us.apache.org/repos/asf/kudu/tree/624a88b1
Diff: http://git-wip-us.apache.org/repos/asf/kudu/diff/624a88b1

Branch: refs/heads/master
Commit: 624a88b18718ea0abc39e32923bb48339398a429
Parents: 6d696bc
Author: Dan Burkert <dan@cloudera.com>
Authored: Mon Oct 3 16:06:34 2016 -0700
Committer: Dan Burkert <dan@cloudera.com>
Committed: Tue Oct 4 19:42:54 2016 +0000

----------------------------------------------------------------------
 .../kudu/client/AbstractKuduScannerBuilder.java       | 14 ++++++++++----
 src/kudu/common/scan_spec-test.cc                     | 10 ++++++----
 src/kudu/common/scan_spec.cc                          | 10 ++++++++++
 3 files changed, 26 insertions(+), 8 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/kudu/blob/624a88b1/java/kudu-client/src/main/java/org/apache/kudu/client/AbstractKuduScannerBuilder.java
----------------------------------------------------------------------
diff --git a/java/kudu-client/src/main/java/org/apache/kudu/client/AbstractKuduScannerBuilder.java b/java/kudu-client/src/main/java/org/apache/kudu/client/AbstractKuduScannerBuilder.java
index ae7b5d4..c23e07e 100644
--- a/java/kudu-client/src/main/java/org/apache/kudu/client/AbstractKuduScannerBuilder.java
+++ b/java/kudu-client/src/main/java/org/apache/kudu/client/AbstractKuduScannerBuilder.java
@@ -126,11 +126,17 @@ public abstract class AbstractKuduScannerBuilder
   public S addPredicate(KuduPredicate predicate) {
     String columnName = predicate.getColumn().getName();
     KuduPredicate existing = predicates.get(columnName);
-    if (existing == null) {
-      predicates.put(columnName, predicate);
-    } else {
-      predicates.put(columnName, existing.merge(predicate));
+    if (existing != null) {
+      predicate = existing.merge(predicate);
+    }
+
+    // KUDU-1652: Do not send an IS NOT NULL predicate to the server for a non-nullable column.
+    if (!predicate.getColumn().isNullable() &&
+        predicate.getType() == KuduPredicate.PredicateType.IS_NOT_NULL) {
+      return (S) this;
     }
+
+    predicates.put(columnName, predicate);
     return (S) this;
   }
 

http://git-wip-us.apache.org/repos/asf/kudu/blob/624a88b1/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 79f6486..10ac42f 100644
--- a/src/kudu/common/scan_spec-test.cc
+++ b/src/kudu/common/scan_spec-test.cc
@@ -118,7 +118,8 @@ class CompositeIntKeysTest : public TestScanSpec {
     TestScanSpec(
         Schema({ ColumnSchema("a", INT8),
                  ColumnSchema("b", INT8),
-                 ColumnSchema("c", INT8) },
+                 ColumnSchema("c", INT8),
+                 ColumnSchema("d", INT8, true) },
                3)) {
   }
 };
@@ -324,15 +325,16 @@ 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
+// Test that IS NOT NULL predicates do *not* get filtered from non-nullable
+// columns. 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)));
+  spec.AddPredicate(ColumnPredicate::IsNotNull(schema_.column(3)));
   SCOPED_TRACE(spec.ToString(schema_));
   spec.OptimizeScan(schema_, &arena_, &pool_, true);
-  EXPECT_EQ("`a` IS NOT NULL", spec.ToString(schema_));
+  EXPECT_EQ("`d` IS NOT NULL", spec.ToString(schema_));
 }
 
 // Test that IN list predicates get pushed into the primary key bounds.

http://git-wip-us.apache.org/repos/asf/kudu/blob/624a88b1/src/kudu/common/scan_spec.cc
----------------------------------------------------------------------
diff --git a/src/kudu/common/scan_spec.cc b/src/kudu/common/scan_spec.cc
index 7beea46..3f3ad07 100644
--- a/src/kudu/common/scan_spec.cc
+++ b/src/kudu/common/scan_spec.cc
@@ -124,6 +124,16 @@ void ScanSpec::OptimizeScan(const Schema& schema,
     LiftPrimaryKeyBounds(schema, arena);
     PushPredicatesIntoPrimaryKeyBounds(schema, arena, pool, remove_pushed_predicates);
   }
+
+  // KUDU-1652: Filter IS NOT NULL predicates for non-nullable columns.
+  for (auto itr = predicates_.begin(); itr != predicates_.end(); ) {
+    if (!itr->second.column().is_nullable() &&
+        itr->second.predicate_type() == PredicateType::IsNotNull) {
+      itr = predicates_.erase(itr);
+    } else {
+      itr = std::next(itr);
+    }
+  }
 }
 
 void ScanSpec::PushPredicatesIntoPrimaryKeyBounds(const Schema& schema,