Zoltan Borok-Nagy has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/14811 )

Change subject: WIP: IMPALA-8046: Support CREATE TABLE from an ORC file
......................................................................


Patch Set 7:

(11 comments)

Found a few nits, other than that lgtm

http://gerrit.cloudera.org:8080/#/c/14811/3//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/14811/3//COMMIT_MSG@19
PS3, Line 19: CREATE TABLE tablename LIKE ORC '/path/to/file'
> CREATE TABLE LIKE  'path/to/file' is not a valid query and the file format
without specifying a file format it creates a table from another table, i.e.:

 CREATE TABLE <new_table> LIKE <source_table>


http://gerrit.cloudera.org:8080/#/c/14811/3/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/3/fe/src/main/java/org/apache/impala/analysis/ORCSchemeExtractor.java@35
PS3, Line 35: import org.apache.impala.util.FileAnalysisUtil;
            : import org.apache.orc.OrcFile;
            : import org.apache.orc.OrcFile.ReaderOptions;
            : import org.apache.orc.Reader;
            : import org.apache.orc.TypeDescription;
> Currently ORC comes from Hive, CDH Hive exports it as its own module.
I'd vote for adding it as a direct dependency for Impala since we are using it 
directly.


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: ORC
nit: use camel case for acronyms, e.g. OrcSchemaExtractor


http://gerrit.cloudera.org:8080/#/c/14811/7/fe/src/main/java/org/apache/impala/analysis/ORCSchemeExtractor.java@49
PS7, Line 49: Scheme
nit: Schema


http://gerrit.cloudera.org:8080/#/c/14811/7/fe/src/main/java/org/apache/impala/analysis/ORCSchemeExtractor.java@57
PS7, Line 57: ORC
nit: Orc


http://gerrit.cloudera.org:8080/#/c/14811/7/fe/src/main/java/org/apache/impala/analysis/ORCSchemeExtractor.java@74
PS7, Line 74: ORC
nit: Orc


http://gerrit.cloudera.org:8080/#/c/14811/7/fe/src/main/java/org/apache/impala/analysis/ORCSchemeExtractor.java@75
PS7, Line 75: throws AnalysisException
Can it really throw AnalysisException?


http://gerrit.cloudera.org:8080/#/c/14811/7/fe/src/main/java/org/apache/impala/analysis/ORCSchemeExtractor.java@148
PS7, Line 148: ORC
nit: Orc


http://gerrit.cloudera.org:8080/#/c/14811/7/fe/src/main/java/org/apache/impala/analysis/ORCSchemeExtractor.java@170
PS7, Line 170: ORC
nit: Orc


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: Scheme
nit: Schema


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: @Test
nit: put @Test into a separate line



--
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: 7
Gerrit-Owner: Norbert Luksa <[email protected]>
Gerrit-Reviewer: Csaba Ringhofer <[email protected]>
Gerrit-Reviewer: Daniel Becker <[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: Tue, 03 Dec 2019 14:45:12 +0000
Gerrit-HasComments: Yes

Reply via email to