kudu-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From danburk...@apache.org
Subject kudu git commit: KUDU-1881: Deserializing scan token should check nullability of column
Date Fri, 17 Feb 2017 08:00:22 GMT
Repository: kudu
Updated Branches:
  refs/heads/master b73a714b9 -> 0b0781bee


KUDU-1881: Deserializing scan token should check nullability of column

Adds test cases for mismatched types and nullability on columns, and
changes the error return to InvalidStatus, since the mismatches are
indicative of changed state, and not necessarily an invalid argument.

Change-Id: I6b4c835c948723eb9bc47131e894971b8e1a2c44
Reviewed-on: http://gerrit.cloudera.org:8080/6040
Reviewed-by: Adar Dembo <adar@cloudera.com>
Tested-by: Kudu Jenkins


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

Branch: refs/heads/master
Commit: 0b0781beeb7a15adf060ca46141ecabe70737f72
Parents: b73a714
Author: Dan Burkert <danburkert@apache.org>
Authored: Thu Feb 16 13:00:55 2017 -0800
Committer: Dan Burkert <danburkert@apache.org>
Committed: Fri Feb 17 06:16:26 2017 +0000

----------------------------------------------------------------------
 .../org/apache/kudu/client/KuduScanToken.java   | 17 ++++-
 .../org/apache/kudu/client/TestKuduClient.java  | 74 ++++++++++++++++++
 src/kudu/client/scan_token-internal.cc          | 22 ++++--
 src/kudu/client/scan_token-test.cc              | 79 ++++++++++++++++++++
 4 files changed, 180 insertions(+), 12 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/kudu/blob/0b0781be/java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanToken.java
----------------------------------------------------------------------
diff --git a/java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanToken.java b/java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanToken.java
index 4d225cc..2f3a93c 100644
--- a/java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanToken.java
+++ b/java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanToken.java
@@ -157,9 +157,18 @@ public class KuduScanToken implements Comparable<KuduScanToken>
{
     for (Common.ColumnSchemaPB column : message.getProjectedColumnsList()) {
       int columnIdx = table.getSchema().getColumnIndex(column.getName());
       ColumnSchema schema = table.getSchema().getColumnByIndex(columnIdx);
-      Preconditions.checkArgument(column.getType() == schema.getType().getDataType(),
-                                  String.format("Column types do not match for column %s",
-                                                column.getName()));
+      if (column.getType() != schema.getType().getDataType()) {
+        throw new IllegalStateException(String.format(
+            "invalid type %s for column '%s' in scan token, expected: %s",
+            column.getType().name(), column.getName(), schema.getType().name()));
+      }
+      if (column.getIsNullable() != schema.isNullable()) {
+        throw new IllegalStateException(String.format(
+            "invalid nullability for column '%s' in scan token, expected: %s",
+            column.getName(), column.getIsNullable() ? "NULLABLE" : "NOT NULL"));
+
+      }
+
       columns.add(columnIdx);
     }
     builder.setProjectedColumnIndexes(columns);
@@ -276,7 +285,7 @@ public class KuduScanToken implements Comparable<KuduScanToken>
{
       if (projectedColumnNames != null) {
         for (String columnName : projectedColumnNames) {
           ColumnSchema columnSchema = table.getSchema().getColumn(columnName);
-          Preconditions.checkArgument(columnSchema != null, "unknown column %s", columnName);
+          Preconditions.checkArgument(columnSchema != null, "unknown column i%s", columnName);
           ProtobufHelper.columnToPb(proto.addProjectedColumnsBuilder(), columnSchema);
         }
       } else if (projectedColumnIndexes != null) {

http://git-wip-us.apache.org/repos/asf/kudu/blob/0b0781be/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 7e5a07a..f5435f1 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
@@ -37,12 +37,14 @@ import java.util.concurrent.atomic.AtomicInteger;
 
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.Iterators;
+import org.junit.Assert;
 import org.junit.Before;
 import org.junit.Test;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
 import org.apache.kudu.ColumnSchema;
+import org.apache.kudu.Common;
 import org.apache.kudu.Schema;
 import org.apache.kudu.Type;
 
@@ -632,6 +634,78 @@ public class TestKuduClient extends BaseKuduTest {
   }
 
   /**
+   * Tests the results of creating scan tokens, altering the columns being
+   * scanned, and then executing the scan tokens.
+   */
+  @Test
+  public void testScanTokensConcurrentAlterTable() throws Exception {
+    Schema schema = new Schema(ImmutableList.of(
+        new ColumnSchema.ColumnSchemaBuilder("key", Type.INT64).nullable(false).key(true).build(),
+        new ColumnSchema.ColumnSchemaBuilder("a", Type.INT64).nullable(false).key(false).build()
+    ));
+    CreateTableOptions createOptions = new CreateTableOptions();
+    createOptions.setRangePartitionColumns(ImmutableList.<String>of());
+    createOptions.setNumReplicas(1);
+    syncClient.createTable(tableName, schema, createOptions);
+
+    KuduTable table = syncClient.openTable(tableName);
+
+    KuduScanToken.KuduScanTokenBuilder tokenBuilder = syncClient.newScanTokenBuilder(table);
+    List<KuduScanToken> tokens = tokenBuilder.build();
+    assertEquals(1, tokens.size());
+    KuduScanToken token = tokens.get(0);
+
+    // Drop a column
+    syncClient.alterTable(tableName, new AlterTableOptions().dropColumn("a"));
+    assertTrue(syncClient.isAlterTableDone(tableName));
+    try {
+      token.intoScanner(syncClient);
+      fail();
+    } catch (IllegalArgumentException e) {
+      assertTrue(e.getMessage().contains("Unknown column"));
+    }
+
+    // Add back the column with the wrong type.
+    syncClient.alterTable(
+        tableName,
+        new AlterTableOptions().addColumn(
+            new ColumnSchema.ColumnSchemaBuilder("a", Type.STRING).nullable(true).build()));
+    assertTrue(syncClient.isAlterTableDone(tableName));
+    try {
+      token.intoScanner(syncClient);
+      fail();
+    } catch (IllegalStateException e) {
+      assertTrue(e.getMessage().contains(
+          "invalid type INT64 for column 'a' in scan token, expected: STRING"));
+    }
+
+    // Add the column with the wrong nullability.
+    syncClient.alterTable(
+        tableName,
+        new AlterTableOptions().dropColumn("a")
+                               .addColumn(new ColumnSchema.ColumnSchemaBuilder("a", Type.INT64)
+                                                          .nullable(true).build()));
+    assertTrue(syncClient.isAlterTableDone(tableName));
+    try {
+      token.intoScanner(syncClient);
+      fail();
+    } catch (IllegalStateException e) {
+      assertTrue(e.getMessage().contains(
+          "invalid nullability for column 'a' in scan token, expected: NOT NULL"));
+    }
+
+    // Add the column with the correct type and nullability.
+    syncClient.alterTable(
+        tableName,
+        new AlterTableOptions().dropColumn("a")
+                               .addColumn(new ColumnSchema.ColumnSchemaBuilder("a", Type.INT64)
+                                                          .nullable(false)
+                                                          .defaultValue(0L).build()));
+    assertTrue(syncClient.isAlterTableDone(tableName));
+    token.intoScanner(syncClient);
+  }
+
+  /**
    * Counts the rows in a table between two optional bounds.
    * @param table the table to scan, must have the basic schema
    * @param lowerBound an optional lower bound key

http://git-wip-us.apache.org/repos/asf/kudu/blob/0b0781be/src/kudu/client/scan_token-internal.cc
----------------------------------------------------------------------
diff --git a/src/kudu/client/scan_token-internal.cc b/src/kudu/client/scan_token-internal.cc
index 31b22f5..e1b0a22 100644
--- a/src/kudu/client/scan_token-internal.cc
+++ b/src/kudu/client/scan_token-internal.cc
@@ -90,17 +90,23 @@ Status KuduScanToken::Data::PBIntoScanner(KuduClient* client,
 
   vector<int> column_indexes;
   for (const ColumnSchemaPB& column : message.projected_columns()) {
-    int columnIdx = schema->find_column(column.name());
-    if (columnIdx == Schema::kColumnNotFound) {
-      return Status::InvalidArgument("unknown column in scan token", column.name());
+    int column_idx = schema->find_column(column.name());
+    if (column_idx == Schema::kColumnNotFound) {
+      return Status::IllegalState("unknown column in scan token", column.name());
     }
-    DataType expectedType = schema->column(columnIdx).type_info()->type();
-    if (column.type() != expectedType) {
-      return Status::InvalidArgument(Substitute(
+    DataType expected_type = schema->column(column_idx).type_info()->type();
+    if (column.type() != expected_type) {
+      return Status::IllegalState(Substitute(
             "invalid type $0 for column '$1' in scan token, expected: $2",
-            column.type(), column.name(), expectedType));
+            DataType_Name(column.type()), column.name(), DataType_Name(expected_type)));
     }
-    column_indexes.push_back(columnIdx);
+    bool expected_is_nullable = schema->column(column_idx).is_nullable();
+    if (column.is_nullable() != expected_is_nullable) {
+      return Status::IllegalState(Substitute(
+            "invalid nullability for column '$0' in scan token, expected: $1",
+            column.name(), expected_is_nullable ? "NULLABLE" : "NOT NULL"));
+    }
+    column_indexes.push_back(column_idx);
   }
   RETURN_NOT_OK(scan_builder->SetProjectedColumnIndexes(column_indexes));
 

http://git-wip-us.apache.org/repos/asf/kudu/blob/0b0781be/src/kudu/client/scan_token-test.cc
----------------------------------------------------------------------
diff --git a/src/kudu/client/scan_token-test.cc b/src/kudu/client/scan_token-test.cc
index 42028e3..521b752 100644
--- a/src/kudu/client/scan_token-test.cc
+++ b/src/kudu/client/scan_token-test.cc
@@ -454,5 +454,84 @@ TEST_F(ScanTokenTest, TestTimestampPropagation) {
   }
 }
 
+// Tests the results of creating scan tokens, altering the columns being
+// scanned, and then executing the scan tokens.
+TEST_F(ScanTokenTest, TestConcurrentAlterTable) {
+  const char* kTableName = "scan-token-alter";
+  // Create schema
+  KuduSchema schema;
+  {
+    KuduSchemaBuilder builder;
+    builder.AddColumn("key")->NotNull()->Type(KuduColumnSchema::INT64)->PrimaryKey();
+    builder.AddColumn("a")->NotNull()->Type(KuduColumnSchema::INT64);
+    ASSERT_OK(builder.Build(&schema));
+  }
+
+  // Create table
+  shared_ptr<KuduTable> table;
+  {
+    unique_ptr<client::KuduTableCreator> table_creator(client_->NewTableCreator());
+    ASSERT_OK(table_creator->table_name(kTableName)
+                            .schema(&schema)
+                            .set_range_partition_columns({})
+                            .num_replicas(1)
+                            .Create());
+    ASSERT_OK(client_->OpenTable(kTableName, &table));
+  }
+
+  vector<KuduScanToken*> tokens;
+  ASSERT_OK(KuduScanTokenBuilder(table.get()).Build(&tokens));
+  ASSERT_EQ(1, tokens.size());
+  unique_ptr<KuduScanToken> token(tokens[0]);
+
+  // Drop a column.
+  {
+    unique_ptr<KuduTableAlterer> table_alterer(client_->NewTableAlterer(kTableName));
+    table_alterer->DropColumn("a");
+    ASSERT_OK(table_alterer->Alter());
+  }
+
+  KuduScanner* scanner_ptr;
+  Status s = token->IntoKuduScanner(&scanner_ptr);
+  ASSERT_EQ("Illegal state: unknown column in scan token: a", s.ToString());
+
+  // Add back the column with the wrong type.
+  {
+    unique_ptr<KuduTableAlterer> table_alterer(client_->NewTableAlterer(kTableName));
+    table_alterer->AddColumn("a")->Type(KuduColumnSchema::STRING);
+    ASSERT_OK(table_alterer->Alter());
+  }
+
+  s = token->IntoKuduScanner(&scanner_ptr);
+  ASSERT_EQ("Illegal state: invalid type INT64 for column 'a' in scan token, expected: STRING",
+            s.ToString());
+
+  // Add back the column with the wrong nullability.
+  {
+    unique_ptr<KuduTableAlterer> table_alterer(client_->NewTableAlterer(kTableName));
+    table_alterer->DropColumn("a");
+    table_alterer->AddColumn("a")->Type(KuduColumnSchema::INT64)->Nullable();
+    ASSERT_OK(table_alterer->Alter());
+  }
+
+  s = token->IntoKuduScanner(&scanner_ptr);
+  ASSERT_EQ("Illegal state: invalid nullability for column 'a' in scan token, expected: NULLABLE",
+            s.ToString());
+
+  // Add the column with the correct type and nullability.
+  {
+    unique_ptr<KuduTableAlterer> table_alterer(client_->NewTableAlterer(kTableName));
+    table_alterer->DropColumn("a");
+    table_alterer->AddColumn("a")
+                 ->Type(KuduColumnSchema::INT64)
+                 ->NotNull()
+                 ->Default(KuduValue::FromInt(0));
+    ASSERT_OK(table_alterer->Alter());
+  }
+
+  ASSERT_OK(token->IntoKuduScanner(&scanner_ptr));
+  delete scanner_ptr;
+}
+
 } // namespace client
 } // namespace kudu


Mime
View raw message