KUDU-1599. Reject invalid type/encoding pairs on table creation Change-Id: Ib508dbfbfd574c7c8ccc90ef8c3a41dcbfea058f Reviewed-on: http://gerrit.cloudera.org:8080/4984 Reviewed-by: Adar Dembo <[email protected]> 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/c7090f9c Tree: http://git-wip-us.apache.org/repos/asf/kudu/tree/c7090f9c Diff: http://git-wip-us.apache.org/repos/asf/kudu/diff/c7090f9c Branch: refs/heads/master Commit: c7090f9c00cbaf08ed871d40cd002fa7c70c96de Parents: f65924d Author: Todd Lipcon <[email protected]> Authored: Mon Nov 7 17:10:44 2016 -0800 Committer: Todd Lipcon <[email protected]> Committed: Tue Nov 8 02:07:49 2016 +0000 ---------------------------------------------------------------------- src/kudu/cfile/type_encodings.cc | 2 +- src/kudu/client/client-test.cc | 19 ++++++++++++++++++- src/kudu/master/catalog_manager.cc | 26 ++++++++++++++++++++------ 3 files changed, 39 insertions(+), 8 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/kudu/blob/c7090f9c/src/kudu/cfile/type_encodings.cc ---------------------------------------------------------------------- diff --git a/src/kudu/cfile/type_encodings.cc b/src/kudu/cfile/type_encodings.cc index 63bb0ed..dd249ac 100644 --- a/src/kudu/cfile/type_encodings.cc +++ b/src/kudu/cfile/type_encodings.cc @@ -238,7 +238,7 @@ class TypeEncodingResolver { const TypeEncodingInfo *type_info = mapping_[make_pair(t, e)].get(); if (PREDICT_FALSE(type_info == nullptr)) { return Status::NotSupported( - strings::Substitute("Unsupported type/encoding pair: $0, $1", + strings::Substitute("encoding $1 not supported for type $0", DataType_Name(t), EncodingType_Name(e))); } http://git-wip-us.apache.org/repos/asf/kudu/blob/c7090f9c/src/kudu/client/client-test.cc ---------------------------------------------------------------------- diff --git a/src/kudu/client/client-test.cc b/src/kudu/client/client-test.cc index 2919280..d7a8a5e 100644 --- a/src/kudu/client/client-test.cc +++ b/src/kudu/client/client-test.cc @@ -3033,7 +3033,7 @@ TEST_F(ClientTest, TestBasicAlterOperations) { ->Encoding(KuduColumnStorageAttributes::GROUP_VARINT); Status s = table_alterer->Alter(); ASSERT_TRUE(s.IsNotSupported()); - ASSERT_STR_CONTAINS(s.ToString(), "Unsupported type/encoding pair"); + ASSERT_STR_CONTAINS(s.ToString(), "encoding GROUP_VARINT not supported for type BINARY"); ASSERT_EQ(1, tablet_peer->tablet()->metadata()->schema_version()); } @@ -3637,6 +3637,23 @@ TEST_F(ClientTest, TestCreateTableWithTooManyReplicas) { "replication factor 3. 1 tablet servers are alive"); } +TEST_F(ClientTest, TestCreateTableWithInvalidEncodings) { + unique_ptr<KuduTableCreator> table_creator(client_->NewTableCreator()); + KuduSchema schema; + KuduSchemaBuilder schema_builder; + schema_builder.AddColumn("key")->Type(KuduColumnSchema::INT32)->NotNull()->PrimaryKey() + ->Encoding(KuduColumnStorageAttributes::DICT_ENCODING); + ASSERT_OK(schema_builder.Build(&schema)); + Status s = table_creator->table_name("foobar") + .schema(&schema) + .set_range_partition_columns({ "key" }) + .Create(); + ASSERT_TRUE(s.IsNotSupported()) << s.ToString(); + ASSERT_STR_CONTAINS(s.ToString(), + "invalid encoding for column 'key': encoding " + "DICT_ENCODING not supported for type INT32"); +} + TEST_F(ClientTest, TestLatestObservedTimestamp) { // Check that a write updates the latest observed timestamp. uint64_t ts0 = client_->GetLatestObservedTimestamp(); http://git-wip-us.apache.org/repos/asf/kudu/blob/c7090f9c/src/kudu/master/catalog_manager.cc ---------------------------------------------------------------------- diff --git a/src/kudu/master/catalog_manager.cc b/src/kudu/master/catalog_manager.cc index 7730ea6..bdbf767 100644 --- a/src/kudu/master/catalog_manager.cc +++ b/src/kudu/master/catalog_manager.cc @@ -804,10 +804,23 @@ Status CatalogManager::CreateTable(const CreateTableRequestPB* orig_req, } for (int i = 0; i < client_schema.num_key_columns(); i++) { if (!IsTypeAllowableInKey(client_schema.column(i).type_info())) { - Status s = Status::InvalidArgument( - "Key column may not have type of BOOL, FLOAT, or DOUBLE"); - SetupError(resp->mutable_error(), MasterErrorPB::INVALID_SCHEMA, s); - return s; + Status s = Status::InvalidArgument( + "Key column may not have type of BOOL, FLOAT, or DOUBLE"); + SetupError(resp->mutable_error(), MasterErrorPB::INVALID_SCHEMA, s); + return s; + } + } + // Check that the encodings are valid for the specified types. + for (int i = 0; i < client_schema.num_columns(); i++) { + const auto& col = client_schema.column(i); + const TypeEncodingInfo *dummy; + Status s = TypeEncodingInfo::Get(col.type_info(), + col.attributes().encoding, + &dummy); + if (!s.ok()) { + s = s.CloneAndPrepend(Substitute("invalid encoding for column '$0'", col.name())); + SetupError(resp->mutable_error(), MasterErrorPB::INVALID_SCHEMA, s); + return s; } } Schema schema = client_schema.CopyWithColumnIds(); @@ -1175,13 +1188,14 @@ Status CatalogManager::ApplyAlterSchemaSteps(const SysTablesEntryPB& current_pb, return Status::InvalidArgument("ADD_COLUMN missing column info"); } - // Verify that encoding is appropriate for the new column's - // type ColumnSchemaPB new_col_pb = step.add_column().schema(); if (new_col_pb.has_id()) { return Status::InvalidArgument("column $0: client should not specify column ID", new_col_pb.ShortDebugString()); } + + // Verify that encoding is appropriate for the new column's + // type ColumnSchema new_col = ColumnSchemaFromPB(new_col_pb); const TypeEncodingInfo *dummy; RETURN_NOT_OK(TypeEncodingInfo::Get(new_col.type_info(),
