Taras Bobrovytsky 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? Done 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's what I did initially, but equals does not work for decimal types. 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 cov Done Line 304: AnalyzesOk("alter table functional.alltypes change column int_col int_col tinyint"); > add tests here Done Line 2471: @Test > add tests here for completeness Done 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 Done 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. So the type centric way would mean all tests that deal with unsupported types would be in this method (instead of in different places), is this correct? I like that idea, keeps things simple. -- 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
