Alex Behm has posted comments on this change. Change subject: IMPALA-3270 & IMPALA-3237: Improve handling of unsupported data types ......................................................................
Patch Set 3: (7 comments) http://gerrit.cloudera.org:8080/#/c/2742/3/fe/src/main/java/com/cloudera/impala/analysis/SelectStmt.java File fe/src/main/java/com/cloudera/impala/analysis/SelectStmt.java: Line 510: if (!p.destType().isSupported()) { The check in line 210 should be sufficient, is it not? http://gerrit.cloudera.org:8080/#/c/2742/3/fe/src/main/java/com/cloudera/impala/catalog/ScalarType.java File fe/src/main/java/com/cloudera/impala/catalog/ScalarType.java: Line 267: if (matchesType(t)) return true; let's use equals() because matchesType() has a slightly different meaning that is not necessary to use here http://gerrit.cloudera.org:8080/#/c/2742/3/fe/src/test/java/com/cloudera/impala/analysis/AnalyzeDDLTest.java File fe/src/test/java/com/cloudera/impala/analysis/AnalyzeDDLTest.java: Line 221: AnalysisError("alter table functional.alltypes add columns (new_col binary)", Can you use a loop here over the unsupported types? That way we'll have coverage if we ever add more unsupported data types (for example, if we want to reserve keywords like BLOB, CLOB, etc.) (use loop elsewhere also) Line 304: AnalyzesOk("alter table functional.alltypes change column int_col int_col tinyint"); add tests here Line 2471: @Test add tests here for completeness http://gerrit.cloudera.org:8080/#/c/2742/3/fe/src/test/java/com/cloudera/impala/analysis/AnalyzeExprsTest.java File fe/src/test/java/com/cloudera/impala/analysis/AnalyzeExprsTest.java: Line 467: AnalysisError("select cast(1 as array<int>)", move these into your new unsupported casts test http://gerrit.cloudera.org:8080/#/c/2742/3/fe/src/test/java/com/cloudera/impala/analysis/AnalyzerTest.java File fe/src/test/java/com/cloudera/impala/analysis/AnalyzerTest.java: Line 556: public void TestUnsupportedTypes() { The current structure of the testing unsupported data type can be improved. We have: 1. tests that focus on the stmt/expr type and check for unsupported types there 2. this type-centric test that checks the behavior for unsupported types in various stmts Ideally, we'd pick one way or the other: stmt/expr centric or type centric. For example, in the type centric solution, we would add all those tests in here. I tend to prefer the type-centric solution because then we have an overview of what works and does not work with these unsupported types. What do you think? -- To view, visit http://gerrit.cloudera.org:8080/2742 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I0acb57d1749978143a3559ae676b882d4b018d58 Gerrit-PatchSet: 3 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Taras Bobrovytsky <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Taras Bobrovytsky <[email protected]> Gerrit-HasComments: Yes
