impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Matthew Jacobs (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-4379: Fix and test Kudu table type checking
Date Thu, 27 Oct 2016 23:17:01 GMT
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 <mj@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dtsirogiannis@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message