Norbert Luksa has posted comments on this change. ( http://gerrit.cloudera.org:8080/14811 )
Change subject: IMPALA-8046: Support CREATE TABLE from an ORC file ...................................................................... Patch Set 8: (16 comments) http://gerrit.cloudera.org:8080/#/c/14811/7/fe/src/main/java/org/apache/impala/analysis/CreateTableLikeFileStmt.java File fe/src/main/java/org/apache/impala/analysis/CreateTableLikeFileStmt.java: http://gerrit.cloudera.org:8080/#/c/14811/7/fe/src/main/java/org/apache/impala/analysis/CreateTableLikeFileStmt.java@74 PS7, Line 74: Schema > nit: Isn't this meant to be Schema Done http://gerrit.cloudera.org:8080/#/c/14811/7/fe/src/main/java/org/apache/impala/analysis/CreateTableLikeFileStmt.java@77 PS7, Line 77: Schema > nit: same as above Done http://gerrit.cloudera.org:8080/#/c/14811/7/fe/src/main/java/org/apache/impala/analysis/ORCSchemeExtractor.java File fe/src/main/java/org/apache/impala/analysis/ORCSchemeExtractor.java: http://gerrit.cloudera.org:8080/#/c/14811/7/fe/src/main/java/org/apache/impala/analysis/ORCSchemeExtractor.java@49 PS7, Line 49: > nit: Schema Done http://gerrit.cloudera.org:8080/#/c/14811/7/fe/src/main/java/org/apache/impala/analysis/ORCSchemeExtractor.java@49 PS7, Line 49: > nit: use camel case for acronyms, e.g. OrcSchemaExtractor Done http://gerrit.cloudera.org:8080/#/c/14811/7/fe/src/main/java/org/apache/impala/analysis/ORCSchemeExtractor.java@50 PS7, Line 50: > I haven't seen any tests for this error msg. Did I miss something? Tried to create an ORC file with a non-primitive map key, but did not find an easy way to do so with the available tools. http://gerrit.cloudera.org:8080/#/c/14811/7/fe/src/main/java/org/apache/impala/analysis/ORCSchemeExtractor.java@74 PS7, Line 74: > nit: Orc Done http://gerrit.cloudera.org:8080/#/c/14811/7/fe/src/main/java/org/apache/impala/analysis/ORCSchemeExtractor.java@75 PS7, Line 75: > Can it really throw AnalysisException? Done http://gerrit.cloudera.org:8080/#/c/14811/7/fe/src/main/java/org/apache/impala/analysis/ORCSchemeExtractor.java@139 PS7, Line 139: > Might worth a DCHECK here as well that the size of fieldNames equals to the Done http://gerrit.cloudera.org:8080/#/c/14811/7/fe/src/main/java/org/apache/impala/analysis/ORCSchemeExtractor.java@148 PS7, Line 148: > nit: Orc Done http://gerrit.cloudera.org:8080/#/c/14811/7/fe/src/main/java/org/apache/impala/analysis/ORCSchemeExtractor.java@170 PS7, Line 170: > nit: Orc Done http://gerrit.cloudera.org:8080/#/c/14811/7/fe/src/main/java/org/apache/impala/analysis/ParquetSchemeExtractor.java File fe/src/main/java/org/apache/impala/analysis/ParquetSchemeExtractor.java: http://gerrit.cloudera.org:8080/#/c/14811/7/fe/src/main/java/org/apache/impala/analysis/ParquetSchemeExtractor.java@50 PS7, Line 50: > nit: Schema Done http://gerrit.cloudera.org:8080/#/c/14811/7/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java File fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java: http://gerrit.cloudera.org:8080/#/c/14811/7/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java@2029 PS7, Line 2029: > To be in line with TestCreateTableLikeFile() could you rename this to TestC Done http://gerrit.cloudera.org:8080/#/c/14811/7/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java@2029 PS7, Line 2029: > I'd also add the "file does't exist" coverage to this function. Done http://gerrit.cloudera.org:8080/#/c/14811/7/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java@2029 PS7, Line 2029: @Test > nit: put @Test into a separate line Done http://gerrit.cloudera.org:8080/#/c/14811/7/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java@2030 PS7, Line 2030: leLikeFileOrc() throws AnalysisException { > I though Apache Impala has ORC support regardless of the Hive version. I ca Right, this comment is misleading. What I meant to write was that the Java API that comes with the CDH Hive ORC fails getting the schema of the ORC file, because of a bug in ORC. This is solved however by listing ORC as an external dependency (like Hive does from version 3), instead of relying on Hive's ORC version. (The bug is fixed in later versions.) http://gerrit.cloudera.org:8080/#/c/14811/7/testdata/workloads/functional-query/queries/QueryTest/create-table-like-table.test File testdata/workloads/functional-query/queries/QueryTest/create-table-like-table.test: http://gerrit.cloudera.org:8080/#/c/14811/7/testdata/workloads/functional-query/queries/QueryTest/create-table-like-table.test@254 PS7, Line 254: --- QUERY > Why was this parquet test necessary for this ORC related patch? This test is not related to ORC support, was only at the wrong place (in create-table-like-file.test) -- To view, visit http://gerrit.cloudera.org:8080/14811 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I77cd84cda2ed86516937a67eb320fd41e3f1cf2d Gerrit-Change-Number: 14811 Gerrit-PatchSet: 8 Gerrit-Owner: Norbert Luksa <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Daniel Becker <[email protected]> Gerrit-Reviewer: Gabor Kaszab <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Norbert Luksa <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Thu, 05 Dec 2019 14:12:47 +0000 Gerrit-HasComments: Yes
