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
Message-Id: <9377f9ad2fea406e9c8eb1d462fd8d18@git.apache.org>
Subject: kudu git commit: KUDU-1652 (part 2): Filter IS NOT NULL predicates
 from scan on client
Date: Tue,  4 Oct 2016 19:56:08 +0000 (UTC)
archived-at: Tue, 04 Oct 2016 19:56:10 -0000

Repository: kudu
Updated Branches:
  refs/heads/branch-1.0.x 40637ef8f -> d2ebf5aa7


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>
Reviewed-on: http://gerrit.cloudera.org:8080/4619
Reviewed-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/d2ebf5aa
Tree: http://git-wip-us.apache.org/repos/asf/kudu/tree/d2ebf5aa
Diff: http://git-wip-us.apache.org/repos/asf/kudu/diff/d2ebf5aa

Branch: refs/heads/branch-1.0.x
Commit: d2ebf5aa7e5165ffd12c3e692547455c7888363a
Parents: 40637ef
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:53:02 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/d2ebf5aa/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 17bc66d..7e913cf 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/d2ebf5aa/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 1bfa404..756afe5 100644
--- a/src/kudu/common/scan_spec-test.cc
+++ b/src/kudu/common/scan_spec-test.cc
@@ -103,7 +103,8 @@ class CompositeIntKeysTest : public TestScanSpec {
     TestScanSpec(
         Schema({ ColumnSchema("a", INT8),
                  ColumnSchema("b", INT8),
-                 ColumnSchema("c", INT8) },
+                 ColumnSchema("c", INT8),
+                 ColumnSchema("d", INT8, true) },
                3)) {
   }
 };
@@ -309,15 +310,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_));
 }
 
 // Tests that a scan spec without primary key bounds will not have predicates

http://git-wip-us.apache.org/repos/asf/kudu/blob/d2ebf5aa/src/kudu/common/scan_spec.cc
----------------------------------------------------------------------
diff --git a/src/kudu/common/scan_spec.cc b/src/kudu/common/scan_spec.cc
index d8469e0..7655035 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,