Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/13558 )
Change subject: IMPALA-8593: Prohibit write operations for bucketed tables ...................................................................... Patch Set 3: (14 comments) http://gerrit.cloudera.org:8080/#/c/13558/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/13558/3//COMMIT_MSG@9 PS3, Line 9: nit: extra space http://gerrit.cloudera.org:8080/#/c/13558/3/fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java File fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java: http://gerrit.cloudera.org:8080/#/c/13558/3/fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java@60 PS3, Line 60: import org.apache.impala.common.Id; I didn't find where we use this. http://gerrit.cloudera.org:8080/#/c/13558/3/fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java@64 PS3, Line 64: import org.apache.impala.service.BackendConfig; This is only mentioned in comment, but is not used. http://gerrit.cloudera.org:8080/#/c/13558/3/fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java@438 PS3, Line 438: * Impala supports external table read/write : * insert-only Acid table read : * virtual view read : * materialized view read Can you make a bullet list from this? e.g: * Impala supports: * - external table read/write * - insert-only Acid table read * - virtual view read * - materialized view read http://gerrit.cloudera.org:8080/#/c/13558/3/fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java@445 PS3, Line 445: version Unused variable. http://gerrit.cloudera.org:8080/#/c/13558/3/fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java@451 PS3, Line 451: // BackendConfig.INSTANCE.getImpalaBuildVersion() is NULL or : // BackendConfig.INSTANCE is NULL when the method is called. : // Use MAJOR_VERSION I do not understand the intention here. http://gerrit.cloudera.org:8080/#/c/13558/3/fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java@454 PS3, Line 454: String impalaId = String.format("Impala%s@%s", : MAJOR_VERSION, hostName); nit: one line http://gerrit.cloudera.org:8080/#/c/13558/3/fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java@466 PS3, Line 466: // number -1. It makes clients unable to know it a bucketed table. nit: missing "is" http://gerrit.cloudera.org:8080/#/c/13558/3/fe/src/main/java/org/apache/impala/analysis/Analyzer.java File fe/src/main/java/org/apache/impala/analysis/Analyzer.java: http://gerrit.cloudera.org:8080/#/c/13558/3/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@300 PS3, Line 300: Check if support the table type or not nit: "Check if the table type is supported" would be better. http://gerrit.cloudera.org:8080/#/c/13558/3/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@311 PS3, Line 311: // the operations not supported, we will generate error messages nit: missing "are" http://gerrit.cloudera.org:8080/#/c/13558/3/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@319 PS3, Line 319: ensureTableNotFullAcid(table); Does this make sense in the MetastoreShim.getMajorVersion() == 2 case? http://gerrit.cloudera.org:8080/#/c/13558/3/fe/src/main/java/org/apache/impala/analysis/ComputeStatsStmt.java File fe/src/main/java/org/apache/impala/analysis/ComputeStatsStmt.java: http://gerrit.cloudera.org:8080/#/c/13558/3/fe/src/main/java/org/apache/impala/analysis/ComputeStatsStmt.java@365 PS3, Line 365: analyzer.checkTableCapability(table_, Analyzer.OperationType.WRITE); I also mentioned in AnalyzerTest.java that prohibiting COMPUTE STATS is strange for me. Impala can generally do COMPUTE STATS for tables it cannot write, e.g. Avro tables. I think that Transactional tables were excluded because Hive uses the stats for them to answer some queries, and we are afraid that differences in stats by Impala can lead to incorrect results in Hive. http://gerrit.cloudera.org:8080/#/c/13558/3/fe/src/test/java/org/apache/impala/analysis/AnalyzerTest.java File fe/src/test/java/org/apache/impala/analysis/AnalyzerTest.java: http://gerrit.cloudera.org:8080/#/c/13558/3/fe/src/test/java/org/apache/impala/analysis/AnalyzerTest.java@897 PS3, Line 897: AnalysisError("compute stats functional.bucketed_table", errorMsgBucketed); It is strange for me that DROP STATS is allowed, while COMPUTE STATS is not for bucketed tables. Is this intentional? http://gerrit.cloudera.org:8080/#/c/13558/3/testdata/datasets/functional/functional_schema_template.sql File testdata/datasets/functional/functional_schema_template.sql: http://gerrit.cloudera.org:8080/#/c/13558/3/testdata/datasets/functional/functional_schema_template.sql@2162 PS3, Line 2162: STORED AS ORC Does this have to be an Orc? If not, then STORED AS {file_format} could be used instead. -- To view, visit http://gerrit.cloudera.org:8080/13558 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia08d01168660830b6e0d08b55a95eac129889cec Gerrit-Change-Number: 13558 Gerrit-PatchSet: 3 Gerrit-Owner: Yongzhi Chen <yc...@cloudera.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Joe McDonnell <joemcdonn...@cloudera.com> Gerrit-Reviewer: Sahil Takiar <stak...@cloudera.com> Gerrit-Reviewer: Sudhanshu Arora <sudhan...@cloudera.com> Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Reviewer: Vihang Karajgaonkar <vih...@cloudera.com> Gerrit-Reviewer: Yongzhi Chen <yc...@cloudera.com> Gerrit-Reviewer: Zoltan Borok-Nagy <borokna...@cloudera.com> Gerrit-Comment-Date: Fri, 28 Jun 2019 15:52:41 +0000 Gerrit-HasComments: Yes
