IMPALA-4379: Fix and test Kudu table type checking, follow up The first fix for IMPALA-4379 went in before all comments were addressed. First commit: 9b507b6.
This addresses some follow-up comments about how to handling ALTER TABLE setting the storage_handler table property, which doesn't really make sense to ever allow. Change-Id: I93d04a04483af598b392c28874363e3b0202e1f3 Reviewed-on: http://gerrit.cloudera.org:8080/4894 Reviewed-by: Matthew Jacobs <[email protected]> Tested-by: Internal Jenkins Project: http://git-wip-us.apache.org/repos/asf/incubator-impala/repo Commit: http://git-wip-us.apache.org/repos/asf/incubator-impala/commit/32294220 Tree: http://git-wip-us.apache.org/repos/asf/incubator-impala/tree/32294220 Diff: http://git-wip-us.apache.org/repos/asf/incubator-impala/diff/32294220 Branch: refs/heads/master Commit: 32294220c4a0a7a159a70cf8ef313622b7190303 Parents: c5f49ec Author: Matthew Jacobs <[email protected]> Authored: Mon Oct 31 16:19:22 2016 -0700 Committer: Internal Jenkins <[email protected]> Committed: Fri Nov 4 06:54:18 2016 +0000 ---------------------------------------------------------------------- .../impala/analysis/AlterTableSetTblProperties.java | 14 +++++++++++--- .../org/apache/impala/service/CatalogOpExecutor.java | 3 +-- .../apache/impala/service/KuduCatalogOpExecutor.java | 14 ++++---------- .../org/apache/impala/analysis/AnalyzeDDLTest.java | 8 ++++++++ .../queries/QueryTest/kudu_alter.test | 5 ----- 5 files changed, 24 insertions(+), 20 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/32294220/fe/src/main/java/org/apache/impala/analysis/AlterTableSetTblProperties.java ---------------------------------------------------------------------- diff --git a/fe/src/main/java/org/apache/impala/analysis/AlterTableSetTblProperties.java b/fe/src/main/java/org/apache/impala/analysis/AlterTableSetTblProperties.java index 1ff744d..f2a81cd 100644 --- a/fe/src/main/java/org/apache/impala/analysis/AlterTableSetTblProperties.java +++ b/fe/src/main/java/org/apache/impala/analysis/AlterTableSetTblProperties.java @@ -22,13 +22,15 @@ import java.util.List; import java.util.Map; import org.apache.avro.SchemaParseException; +import org.apache.hadoop.hive.metastore.api.hive_metastoreConstants; import org.apache.hadoop.hive.serde2.avro.AvroSerdeUtils; - -import org.apache.impala.catalog.HdfsFileFormat; import org.apache.impala.catalog.HdfsTable; import org.apache.impala.catalog.Table; import org.apache.impala.common.AnalysisException; -import org.apache.impala.thrift.*; +import org.apache.impala.thrift.TAlterTableParams; +import org.apache.impala.thrift.TAlterTableSetTblPropertiesParams; +import org.apache.impala.thrift.TAlterTableType; +import org.apache.impala.thrift.TTablePropertyType; import org.apache.impala.util.AvroSchemaParser; import org.apache.impala.util.AvroSchemaUtils; import org.apache.impala.util.MetaStoreUtil; @@ -78,6 +80,12 @@ public class AlterTableSetTblProperties extends AlterTableSetStmt { MetaStoreUtil.checkShortPropertyMap("Property", tblProperties_); + if (tblProperties_.containsKey(hive_metastoreConstants.META_TABLE_STORAGE)) { + throw new AnalysisException(String.format("Changing the '%s' table property is " + + "not supported to protect against metadata corruption.", + hive_metastoreConstants.META_TABLE_STORAGE)); + } + // Check avro schema when it is set in avro.schema.url or avro.schema.literal to // avoid potential metadata corruption (see IMPALA-2042). // If both properties are set then only check avro.schema.literal and ignore http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/32294220/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java ---------------------------------------------------------------------- diff --git a/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java b/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java index f5ffb59..ce5a1b2 100644 --- a/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java +++ b/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java @@ -2082,9 +2082,8 @@ public class CatalogOpExecutor { tbl.getMetaStoreTable().deepCopy(); switch (params.getTarget()) { case TBL_PROPERTY: - boolean isKuduTable = KuduTable.isKuduTable(msTbl); msTbl.getParameters().putAll(properties); - if (isKuduTable) { + if (KuduTable.isKuduTable(msTbl)) { KuduCatalogOpExecutor.validateKuduTblExists(msTbl); } break; http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/32294220/fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java ---------------------------------------------------------------------- diff --git a/fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java b/fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java index ad4f2a9..7f51717 100644 --- a/fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java +++ b/fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java @@ -221,21 +221,15 @@ public class KuduCatalogOpExecutor { } /** - * Validates the table properties of a Kudu table. It checks that the msTbl represents - * a Kudu table (indicated by the Kudu storage handler), that the master - * addresses point to valid Kudu masters, and that the table exists. + * Validates the table properties of a Kudu table. It checks that the master + * addresses point to valid Kudu masters and that the table exists. * Throws an ImpalaRuntimeException if this is not the case. */ public static void validateKuduTblExists( org.apache.hadoop.hive.metastore.api.Table msTbl) throws ImpalaRuntimeException { - Map<String, String> properties = msTbl.getParameters(); - if (!KuduTable.isKuduTable(msTbl)) { - throw new ImpalaRuntimeException(String.format("Table '%s' does not represent a " + - "Kudu table. Expected storage_handler '%s' but found '%s'", - msTbl.getTableName(), KuduTable.KUDU_STORAGE_HANDLER, - properties.get(KuduTable.KEY_STORAGE_HANDLER))); - } + Preconditions.checkArgument(KuduTable.isKuduTable(msTbl)); + Map<String, String> properties = msTbl.getParameters(); String masterHosts = properties.get(KuduTable.KEY_MASTER_HOSTS); Preconditions.checkState(!Strings.isNullOrEmpty(masterHosts)); String kuduTableName = properties.get(KuduTable.KEY_TABLE_NAME); http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/32294220/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java ---------------------------------------------------------------------- diff --git a/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java b/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java index 730ca92..1f718d5 100644 --- a/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java +++ b/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java @@ -408,6 +408,11 @@ public class AnalyzeDDLTest extends FrontendTestBase { + "serdeproperties ('key'='" + long_property_value + "')", "Property value length must be <= " + MetaStoreUtil.MAX_PROPERTY_VALUE_LENGTH + ": " + (MetaStoreUtil.MAX_PROPERTY_VALUE_LENGTH + 1)); + + AnalysisError( + "alter table functional.alltypes set tblproperties('storage_handler'='1')", + "Changing the 'storage_handler' table property is not supported to protect " + + "against metadata corruption."); } // Arbitrary exprs as partition key values. Constant exprs are ok. @@ -1361,6 +1366,9 @@ public class AnalyzeDDLTest extends FrontendTestBase { " stored as kudu as select vc from functional.chars_tiny", "Cannot create table 't': Type VARCHAR(32) is not supported in Kudu"); AnalysisError("create table t primary key (id) distribute by hash into 3 buckets" + + " stored as kudu as select c1 as id from functional.decimal_tiny", + "Cannot create table 't': Type DECIMAL(10,4) is not supported in Kudu"); + AnalysisError("create table t primary key (id) distribute by hash into 3 buckets" + " stored as kudu as select id, s from functional.complextypes_fileformat", "Expr 's' in select list returns a complex type 'STRUCT<f1:STRING,f2:INT>'.\n" + "Only scalar types are allowed in the select list."); http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/32294220/testdata/workloads/functional-query/queries/QueryTest/kudu_alter.test ---------------------------------------------------------------------- diff --git a/testdata/workloads/functional-query/queries/QueryTest/kudu_alter.test b/testdata/workloads/functional-query/queries/QueryTest/kudu_alter.test index ad49c59..e6814e1 100644 --- a/testdata/workloads/functional-query/queries/QueryTest/kudu_alter.test +++ b/testdata/workloads/functional-query/queries/QueryTest/kudu_alter.test @@ -40,8 +40,3 @@ select count(*) from simple_new; ---- TYPES BIGINT ==== ----- QUERY -alter table simple_new set tblproperties ('storage_handler'='foo') ----- CATCH -ImpalaRuntimeException: Table 'simple_new' does not represent a Kudu table. Expected storage_handler 'com.cloudera.kudu.hive.KuduStorageHandler' but found 'foo' -====
