Matthew Jacobs has posted comments on this change. Change subject: IMPALA-4379: Fix and test Kudu table type checking ......................................................................
Patch Set 2: (4 comments) http://gerrit.cloudera.org:8080/#/c/4857/2/fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java File fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java: Line 232: if (!KuduTable.isKuduTable(msTbl)) { > Is the intention here to forbid changing the storage_handler? If so why not Yeah, good point. I followed up with your comment in the test. If you're OK with me disallowing changing the storage_descriptor, then I'll change this to just disallow changing it in the CatalogOpExecutor handling of alter tableproperties. I also don't think this is critical so I can leave it out if you prefer. http://gerrit.cloudera.org:8080/#/c/4857/2/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java File fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java: PS2, Line 1362: VARCHAR(32) > Maybe add one for decimal as well? Thanks, I missed that one. PS2, Line 1363: 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."); : AnalysisError("create table t primary key (id) distribute by hash into 3 buckets" + : " stored as kudu as select id, m from functional.complextypes_fileformat", : "Expr 'm' in select list returns a complex type 'MAP<STRING,BIGINT>'.\n" + : "Only scalar types are allowed in the select list."); : AnalysisError("create table t primary key (id) distribute by hash into 3 buckets" + : " stored as kudu as select id, a from functional.complextypes_fileformat", : "Expr 'a' in select list returns a complex type 'ARRAY<INT>'.\n" + : "Only scalar types are allowed in the select list."); > I am not sure this exercises any Kudu-related code path. This error probabl Yeah, you're right it's not Kudu specific, but I want these tests to be here to make sure it fails even if that code changes. We can always update the test but the expectation is CTAS for these types should fail. http://gerrit.cloudera.org:8080/#/c/4857/2/testdata/workloads/functional-query/queries/QueryTest/kudu_alter.test File testdata/workloads/functional-query/queries/QueryTest/kudu_alter.test: Line 46: ImpalaRuntimeException: Table 'simple_new' does not represent a Kudu table. Expected storage_handler 'com.cloudera.kudu.hive.KuduStorageHandler' but found 'foo' > This behavior seems a little strange because 'simple_new' is actually a Kud Well, I was thinking it might not make sense to change the storage handler of a table. AFAIK, storage handlers specify an external storage engine in Hive, so it seems unnecessary to allow someone to change a table from pointing at kudu to hive, or something else. However, the bigger concern I have is that once these tables have a junk storage_handler it's really hard to do anything with them in Hive or Impala. Both Hive and Impala won't even allow me to drop a table once it is in this state. While these tables should be drop-able, it seems weird to even allow users to shoot themselves in the foot in the first place. -- To view, visit http://gerrit.cloudera.org:8080/4857 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I475273cbbf4110db8d0f78ddf9a56abfc6221e3e Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Matthew Jacobs <m...@cloudera.com> Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com> Gerrit-Reviewer: Dimitris Tsirogiannis <dtsirogian...@cloudera.com> Gerrit-Reviewer: Matthew Jacobs <m...@cloudera.com> Gerrit-HasComments: Yes
