[Impala-ASF-CR] IMPALA-8593: Prohibit write operations for bucketed tables
Impala Public Jenkins 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: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/3790/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- 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 Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Sudhanshu Arora Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Vihang Karajgaonkar Gerrit-Reviewer: Yongzhi Chen Gerrit-Reviewer: Zoltan Borok-Nagy Gerrit-Comment-Date: Sat, 29 Jun 2019 12:12:50 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8593: Prohibit write operations for bucketed tables
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.
[Impala-ASF-CR] IMPALA-8593: Prohibit write operations for bucketed tables
Impala Public Jenkins 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: (4 comments) http://gerrit.cloudera.org:8080/#/c/13558/4/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/4/fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java@20 PS4, Line 20: import static org.apache.hadoop.hive.metastore.api.hive_metastoreConstants.ACCESSTYPE_NONE; line too long (91 > 90) http://gerrit.cloudera.org:8080/#/c/13558/4/fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java@21 PS4, Line 21: import static org.apache.hadoop.hive.metastore.api.hive_metastoreConstants.ACCESSTYPE_READONLY; line too long (95 > 90) http://gerrit.cloudera.org:8080/#/c/13558/4/fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java@22 PS4, Line 22: import static org.apache.hadoop.hive.metastore.api.hive_metastoreConstants.ACCESSTYPE_READWRITE; line too long (96 > 90) http://gerrit.cloudera.org:8080/#/c/13558/4/fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java@23 PS4, Line 23: import static org.apache.hadoop.hive.metastore.api.hive_metastoreConstants.ACCESSTYPE_WRITEONLY; line too long (96 > 90) -- 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 Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Sudhanshu Arora Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Vihang Karajgaonkar Gerrit-Reviewer: Yongzhi Chen Gerrit-Reviewer: Zoltan Borok-Nagy Gerrit-Comment-Date: Sat, 29 Jun 2019 11:33:21 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8593: Prohibit write operations for bucketed tables
Hello Vihang Karajgaonkar, Sudhanshu Arora, Zoltan Borok-Nagy, Sahil Takiar, Todd Lipcon, Joe McDonnell, Csaba Ringhofer, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/13558 to look at the new patch set (#4). Change subject: IMPALA-8593: Prohibit write operations for bucketed tables .. IMPALA-8593: Prohibit write operations for bucketed tables This patch adds a method to check if a table bucketed. For Hive 3, integrates with HMS translation layer for capabilities checks. Implements methods ensureTableWriteSupported and ensureTableReadSupported. Tests: Added unit tests to ParserTest and AnalyzerTest. Added bucketed tables which are required by IMPALA-8439. Ran core tests. ToDo: Integrate checking bucketed tables capabilities and creating error messages with HMS translation after Hive provides the required functions. Change-Id: Ia08d01168660830b6e0d08b55a95eac129889cec --- M fe/src/compat-hive-2/java/org/apache/impala/compat/MetastoreShim.java M fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java M fe/src/main/java/org/apache/impala/analysis/AlterTableStmt.java M fe/src/main/java/org/apache/impala/analysis/Analyzer.java M fe/src/main/java/org/apache/impala/analysis/BaseTableRef.java M fe/src/main/java/org/apache/impala/analysis/CreateTableLikeStmt.java M fe/src/main/java/org/apache/impala/analysis/DropTableOrViewStmt.java M fe/src/main/java/org/apache/impala/analysis/InsertStmt.java M fe/src/main/java/org/apache/impala/analysis/LoadDataStmt.java M fe/src/main/java/org/apache/impala/analysis/TruncateStmt.java M fe/src/main/java/org/apache/impala/catalog/MetaStoreClientPool.java M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java M fe/src/main/java/org/apache/impala/util/MetaStoreUtil.java M fe/src/test/java/org/apache/impala/analysis/AnalyzerTest.java M fe/src/test/java/org/apache/impala/analysis/ParserTest.java M testdata/datasets/functional/functional_schema_template.sql M testdata/datasets/functional/schema_constraints.csv 17 files changed, 346 insertions(+), 7 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/58/13558/4 -- 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: newpatchset Gerrit-Change-Id: Ia08d01168660830b6e0d08b55a95eac129889cec Gerrit-Change-Number: 13558 Gerrit-PatchSet: 4 Gerrit-Owner: Yongzhi Chen Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Sudhanshu Arora Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Vihang Karajgaonkar Gerrit-Reviewer: Yongzhi Chen Gerrit-Reviewer: Zoltan Borok-Nagy
