Yongzhi Chen has posted comments on this change. ( http://gerrit.cloudera.org:8080/13558 )
Change subject: IMPALA-8593: Prohibit write operations for bucketed tables ...................................................................... Patch Set 4: (14 comments) Patch set 4 addresses the issue and passed core tests for Hive2 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 Done 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.Imp > I didn't find where we use this. removed 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.MetadataOp; > This is only mentioned in comment, but is not used. removed http://gerrit.cloudera.org:8080/#/c/13558/3/fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java@438 PS3, Line 438: * - insert-only Acid table read : * - virtual view read : * - materialized view read : */ > Can you make a bullet list from this? Done http://gerrit.cloudera.org:8080/#/c/13558/3/fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java@445 PS3, Line 445: ame = I > Unused variable. Done http://gerrit.cloudera.org:8080/#/c/13558/3/fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java@451 PS3, Line 451: // Use MAJOR_VERSION for now : // TODO: Change to impala build version when the functions are fixed. : String impalaId = St > I do not understand the intention here. Explain why I am using major version not impala version for impala id: The function available in Impala does not work for catalog front end and causes for troubles. For the issue does not affect the feature(any name will be fine), comment here, we can change the name after the related functions fixed. 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[] capabilities = new String[] { : EXTWRITE, // External tab > nit: one line Done http://gerrit.cloudera.org:8080/#/c/13558/3/fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java@466 PS3, Line 466: }; > nit: missing "is" I think the "is" can be omitted, but I will add it anyway. 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 the table type is supported > nit: "Check if the table type is supported" would be better. Done http://gerrit.cloudera.org:8080/#/c/13558/3/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@311 PS3, Line 311: // the operations are not supported, we will generate error messages > nit: missing "are" This can be omitted too. http://gerrit.cloudera.org:8080/#/c/13558/3/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@319 PS3, Line 319: ensureTableNotTransactional(ta > Does this make sense in the MetastoreShim.getMajorVersion() == 2 case? It is just a direct translation of the original code where ensureTableNotFullAcid(table) used, for version 2, need check if it is transactional. I will change to ensureTableNotTransactional(table) And this kind of check is needed for transactional table use table properties, it can go into version 2 DB. 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.ensureTableNotTransactional(table_); > I also mentioned in AnalyzerTest.java that prohibiting COMPUTE STATS is str Done, change back to the original implementation. I thought we cannot compute accurate stats for the tables that we do not know how to write. 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: "is a bucketed table. Only read operations are supported on such tables > It is strange for me that DROP STATS is allowed, while COMPUTE STATS is not Fixed. 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: ---- DATASET > Does this have to be an Orc? If not, then STORED AS {file_format} could be Yes, it is a transactional bucketed table which very limited file formats are satisfied. I have a non-transactional bucketed table test cover all of the supported bucketed types. -- 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: 4 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: Sat, 29 Jun 2019 11:37:32 +0000 Gerrit-HasComments: Yes
