You are viewing a plain text version of this content. The canonical link for it is here.
Posted to gitbox@hive.apache.org by GitBox <gi...@apache.org> on 2021/06/03 15:59:14 UTC
[GitHub] [hive] lcspinter opened a new pull request #2348: HIVE-25194: Add support for STORED AS ORC/PARQUET/AVRO for Iceberg
lcspinter opened a new pull request #2348:
URL: https://github.com/apache/hive/pull/2348
<!--
Thanks for sending a pull request! Here are some tips for you:
1. If this is your first time, please read our contributor guidelines: https://cwiki.apache.org/confluence/display/Hive/HowToContribute
2. Ensure that you have created an issue on the Hive project JIRA: https://issues.apache.org/jira/projects/HIVE/summary
3. Ensure you have added or run the appropriate tests for your PR:
4. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP]HIVE-XXXXX: Your PR title ...'.
5. Be sure to keep the PR description updated to reflect all changes.
6. Please write your PR title to summarize what this PR proposes.
7. If possible, provide a concise example to reproduce the issue for a faster review.
-->
### What changes were proposed in this pull request?
Support `STORED AS fileformat` syntax together with `STORED BY ICEBERG` or `STORED BY 'org.apache.iceberg.mr.hive.HiveIcebergStorageHandler'`
Work covered in this PR:
- Enhance semantic rule to support both`STORED AS` and `STORED BY` together
- Updated SemanticAnalyzer and StorageFormat to parse the new query format
- Created q tests
<!--
Please clarify what changes you are proposing. The purpose of this section is to outline the changes and how this PR fixes the issue.
If possible, please consider writing useful notes for better and faster reviews in your PR. See the examples below.
1. If you refactor some codes with changing classes, showing the class hierarchy will help reviewers.
2. If you fix some SQL features, you can provide some references of other DBMSes.
3. If there is design documentation, please add the link.
4. If there is a discussion in the mailing list, please add the link.
-->
### Why are the changes needed?
Currently, the file format is configurable through the `TBLPROPERTIES` which can be inconvenient, if the end-user doesn't know which table property key should be used to specify the file format.
<!--
Please clarify why the changes are needed. For instance,
1. If you propose a new API, clarify the use case for a new API.
2. If you fix a bug, you can clarify why it is a bug.
-->
### Does this PR introduce _any_ user-facing change?
New syntax:
`CREATE TABLE ... STORED BY ICEBERG [WITH SERDEPROPERTIES()][STORED AS fileformat]`
<!--
Note that it means *any* user-facing change including all aspects such as the documentation fix.
If yes, please clarify the previous behavior and the change this PR proposes - provide the console output, description, screenshot and/or a reproducable example to show the behavior difference if possible.
If possible, please also clarify if this is a user-facing change compared to the released Hive versions or within the unreleased branches such as master.
If no, write 'No'.
-->
### How was this patch tested?
Manual test, q test
<!--
If tests were added, say they were added here. Please make sure to add some test cases that check the changes thoroughly including negative and positive cases if possible.
If it was tested in a way different from regular unit tests, please clarify how you tested step by step, ideally copy and paste-able, so that other reviewers can test and check, and descendants can verify in the future.
If tests were not added, please describe why they were not added and/or why it was difficult to add.
-->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] lcspinter commented on a change in pull request #2348: HIVE-25194: Add support for STORED AS ORC/PARQUET/AVRO for Iceberg
Posted by GitBox <gi...@apache.org>.
lcspinter commented on a change in pull request #2348:
URL: https://github.com/apache/hive/pull/2348#discussion_r645558813
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/parse/StorageFormat.java
##########
@@ -81,11 +84,37 @@ public boolean fillStorageFormat(ASTNode child) throws SemanticException {
}
break;
case HiveParser.TOK_STORAGEHANDLER:
- storageHandler = processStorageHandler(child.getChild(0).getText());
- if (child.getChildCount() == 2) {
- BaseSemanticAnalyzer.readProps(
- (ASTNode) (child.getChild(1).getChild(0)),
- serdeProps);
+ for (int i = 0; i < child.getChildCount(); i++) {
+ ASTNode grandChild = (ASTNode) child.getChild(i);
+ switch (grandChild.getToken().getType()) {
+ case HiveParser.TOK_FILEFORMAT_GENERIC:
+ HiveStorageHandler handler;
+ try {
+ handler = HiveUtils.getStorageHandler(conf, storageHandler);
+ } catch (HiveException e) {
+ throw new SemanticException("Failed to load storage handler: " + e.getMessage());
+ }
+
+ String fileFormatPropertyKey = handler.getFileFormatPropertyKey();
+ if (fileFormatPropertyKey != null) {
+ String fileFormat = grandChild.getChild(0).getText();
+ if (serdeProps.containsKey(fileFormatPropertyKey)) {
+ throw new SemanticException("Provide only one of the following: STORED BY " + fileFormat +
+ " or WITH SERDEPROPERTIES('" + fileFormatPropertyKey + "'='" + fileFormat + "')");
+ }
+
+ serdeProps.put(fileFormatPropertyKey, fileFormat);
+ } else {
+ throw new SemanticException("STORED BY is not supported for storage handler " +
Review comment:
Yepp, typo
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] marton-bod commented on a change in pull request #2348: HIVE-25194: Add support for STORED AS ORC/PARQUET/AVRO for Iceberg
Posted by GitBox <gi...@apache.org>.
marton-bod commented on a change in pull request #2348:
URL: https://github.com/apache/hive/pull/2348#discussion_r645536560
##########
File path: iceberg/iceberg-handler/src/test/queries/negative/create_iceberg_table_stored_as_with_serdeproperties_failure.q
##########
@@ -0,0 +1,3 @@
+set hive.vectorized.execution.enabled=false;
Review comment:
Can we add a negative test somewhere to verify that it fails for other storage handler types? e.g. stored by kudu stored as orc. Not sure what the best place would be to put it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] marton-bod commented on a change in pull request #2348: HIVE-25194: Add support for STORED AS ORC/PARQUET/AVRO for Iceberg
Posted by GitBox <gi...@apache.org>.
marton-bod commented on a change in pull request #2348:
URL: https://github.com/apache/hive/pull/2348#discussion_r645506912
##########
File path: parser/src/java/org/apache/hadoop/hive/ql/parse/HiveParser.g
##########
@@ -1971,10 +1971,12 @@ tableFileFormat
-> ^(TOK_TABLEFILEFORMAT $inFmt $outFmt $inDriver? $outDriver?)
| KW_STORED KW_BY storageHandler=StringLiteral
(KW_WITH KW_SERDEPROPERTIES serdeprops=tableProperties)?
- -> ^(TOK_STORAGEHANDLER $storageHandler $serdeprops?)
+ (KW_STORED KW_AS fileformat=identifier)?
Review comment:
Does this mean we will now enable STORED BY and STORED AS in the same query for all storage handlers, like Kudu, Kafka, etc? Do we expect them to break as a result, e.g. if saying something like `stored by kudu stored as orc`? In addition, I guess we should update the Hive language manual too after this change to signify that stored as/stored by are not mutually exclusive.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] pvary commented on a change in pull request #2348: HIVE-25194: Add support for STORED AS ORC/PARQUET/AVRO for Iceberg
Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #2348:
URL: https://github.com/apache/hive/pull/2348#discussion_r647203906
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java
##########
@@ -13477,15 +13477,28 @@ ASTNode analyzeCreateTable(
}
}
- if (partitionTransformSpecExists) {
- try {
- HiveStorageHandler storageHandler = HiveUtils.getStorageHandler(conf, storageFormat.getStorageHandler());
- if (!storageHandler.supportsPartitionTransform()) {
- throw new SemanticException("Partition transform is not supported for " +
- storageHandler.getClass().getName());
+ HiveStorageHandler handler;
+ try {
+ handler = HiveUtils.getStorageHandler(conf, storageFormat.getStorageHandler());
Review comment:
Do we have an exception for native tables? In my experience sometimes the StorageHandler is `null`, but there might be some other issues here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] lcspinter commented on a change in pull request #2348: HIVE-25194: Add support for STORED AS ORC/PARQUET/AVRO for Iceberg
Posted by GitBox <gi...@apache.org>.
lcspinter commented on a change in pull request #2348:
URL: https://github.com/apache/hive/pull/2348#discussion_r645345788
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/parse/StorageFormat.java
##########
@@ -81,11 +84,34 @@ public boolean fillStorageFormat(ASTNode child) throws SemanticException {
}
break;
case HiveParser.TOK_STORAGEHANDLER:
- storageHandler = processStorageHandler(child.getChild(0).getText());
- if (child.getChildCount() == 2) {
- BaseSemanticAnalyzer.readProps(
- (ASTNode) (child.getChild(1).getChild(0)),
- serdeProps);
+ for (int i = 0; i < child.getChildCount(); i++) {
Review comment:
In this case, the `storagehandler.getFileFormatPropertyKey` would return `null` and the fileformat property would be disregarded. But I see your point here, maybe we should warn the end user about this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] pvary commented on a change in pull request #2348: HIVE-25194: Add support for STORED AS ORC/PARQUET/AVRO for Iceberg
Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #2348:
URL: https://github.com/apache/hive/pull/2348#discussion_r645003383
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/parse/StorageFormat.java
##########
@@ -81,11 +84,34 @@ public boolean fillStorageFormat(ASTNode child) throws SemanticException {
}
break;
case HiveParser.TOK_STORAGEHANDLER:
- storageHandler = processStorageHandler(child.getChild(0).getText());
- if (child.getChildCount() == 2) {
- BaseSemanticAnalyzer.readProps(
- (ASTNode) (child.getChild(1).getChild(0)),
- serdeProps);
+ for (int i = 0; i < child.getChildCount(); i++) {
+ ASTNode grandChild = (ASTNode) child.getChild(i);
+ switch (grandChild.getToken().getType()) {
+ case HiveParser.TOK_FILEFORMAT_GENERIC:
+ String fileFormatPropertyKey = null;
+ try {
+ HiveStorageHandler handler = HiveUtils.getStorageHandler(conf, this.storageHandler);
+ fileFormatPropertyKey = handler.getFileFormatPropertyKey();
+ } catch (HiveException e) {
+ throw new SemanticException("Failed to load storage handler: " + e.getMessage());
+ }
+
+ if (fileFormatPropertyKey != null) {
+ String fileFormat = grandChild.getChild(0).getText();
+ if (serdeProps.containsKey(fileFormatPropertyKey)) {
+ throw new SemanticException("Provide only one of the following: STORED BY " + fileFormat +
+ " or WITH SERDEPROPERTIES('" + fileFormatPropertyKey + "'='" + fileFormat + "')");
+ }
+
+ serdeProps.put(fileFormatPropertyKey, fileFormat);
Review comment:
What happens if the fileformat is Text or some other valid fileformat which is not supported by Iceberg?
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/parse/StorageFormat.java
##########
@@ -81,11 +84,34 @@ public boolean fillStorageFormat(ASTNode child) throws SemanticException {
}
break;
case HiveParser.TOK_STORAGEHANDLER:
- storageHandler = processStorageHandler(child.getChild(0).getText());
- if (child.getChildCount() == 2) {
- BaseSemanticAnalyzer.readProps(
- (ASTNode) (child.getChild(1).getChild(0)),
- serdeProps);
+ for (int i = 0; i < child.getChildCount(); i++) {
Review comment:
What happens when StorageHandler is HBase and Stored By is also set?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] lcspinter commented on a change in pull request #2348: HIVE-25194: Add support for STORED AS ORC/PARQUET/AVRO for Iceberg
Posted by GitBox <gi...@apache.org>.
lcspinter commented on a change in pull request #2348:
URL: https://github.com/apache/hive/pull/2348#discussion_r646495865
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java
##########
@@ -13456,6 +13457,26 @@ ASTNode analyzeCreateTable(
}
}
+ HiveStorageHandler handler = null;
+ try {
+ handler = HiveUtils.getStorageHandler(conf, storageFormat.getStorageHandler());
+ } catch (HiveException e) {
+ throw new SemanticException("Failed to load storage handler: " + e.getMessage());
+ }
+
+ if (handler != null) {
+ String fileFormatPropertyKey = handler.getFileFormatPropertyKey();
+ if (fileFormatPropertyKey != null) {
+ if (tblProps != null && tblProps.containsKey(fileFormatPropertyKey) && storageFormat.getSerdeProps() != null &&
Review comment:
In `StorageFormat` class I store the fileformat in the serdeproperties, because I do not have reference to the table properties (it might be the case that the tableproperties were not parsed yet)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] pvary commented on a change in pull request #2348: HIVE-25194: Add support for STORED AS ORC/PARQUET/AVRO for Iceberg
Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #2348:
URL: https://github.com/apache/hive/pull/2348#discussion_r647203906
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java
##########
@@ -13477,15 +13477,28 @@ ASTNode analyzeCreateTable(
}
}
- if (partitionTransformSpecExists) {
- try {
- HiveStorageHandler storageHandler = HiveUtils.getStorageHandler(conf, storageFormat.getStorageHandler());
- if (!storageHandler.supportsPartitionTransform()) {
- throw new SemanticException("Partition transform is not supported for " +
- storageHandler.getClass().getName());
+ HiveStorageHandler handler;
+ try {
+ handler = HiveUtils.getStorageHandler(conf, storageFormat.getStorageHandler());
Review comment:
Could it be that the `storageFormat.getStorageHandler()` is null? Like for native tables?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] marton-bod commented on a change in pull request #2348: HIVE-25194: Add support for STORED AS ORC/PARQUET/AVRO for Iceberg
Posted by GitBox <gi...@apache.org>.
marton-bod commented on a change in pull request #2348:
URL: https://github.com/apache/hive/pull/2348#discussion_r645508014
##########
File path: parser/src/java/org/apache/hadoop/hive/ql/parse/HiveParser.g
##########
@@ -1971,10 +1971,12 @@ tableFileFormat
-> ^(TOK_TABLEFILEFORMAT $inFmt $outFmt $inDriver? $outDriver?)
| KW_STORED KW_BY storageHandler=StringLiteral
(KW_WITH KW_SERDEPROPERTIES serdeprops=tableProperties)?
- -> ^(TOK_STORAGEHANDLER $storageHandler $serdeprops?)
+ (KW_STORED KW_AS fileformat=identifier)?
Review comment:
Ah, I just saw below that it shouldn't affect those storage handlers which don't implement new interface method. So the only consideration is then is the language manual update
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] pvary commented on a change in pull request #2348: HIVE-25194: Add support for STORED AS ORC/PARQUET/AVRO for Iceberg
Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #2348:
URL: https://github.com/apache/hive/pull/2348#discussion_r645003383
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/parse/StorageFormat.java
##########
@@ -81,11 +84,34 @@ public boolean fillStorageFormat(ASTNode child) throws SemanticException {
}
break;
case HiveParser.TOK_STORAGEHANDLER:
- storageHandler = processStorageHandler(child.getChild(0).getText());
- if (child.getChildCount() == 2) {
- BaseSemanticAnalyzer.readProps(
- (ASTNode) (child.getChild(1).getChild(0)),
- serdeProps);
+ for (int i = 0; i < child.getChildCount(); i++) {
+ ASTNode grandChild = (ASTNode) child.getChild(i);
+ switch (grandChild.getToken().getType()) {
+ case HiveParser.TOK_FILEFORMAT_GENERIC:
+ String fileFormatPropertyKey = null;
+ try {
+ HiveStorageHandler handler = HiveUtils.getStorageHandler(conf, this.storageHandler);
+ fileFormatPropertyKey = handler.getFileFormatPropertyKey();
+ } catch (HiveException e) {
+ throw new SemanticException("Failed to load storage handler: " + e.getMessage());
+ }
+
+ if (fileFormatPropertyKey != null) {
+ String fileFormat = grandChild.getChild(0).getText();
+ if (serdeProps.containsKey(fileFormatPropertyKey)) {
+ throw new SemanticException("Provide only one of the following: STORED BY " + fileFormat +
+ " or WITH SERDEPROPERTIES('" + fileFormatPropertyKey + "'='" + fileFormat + "')");
+ }
+
+ serdeProps.put(fileFormatPropertyKey, fileFormat);
Review comment:
What happens if the fileformat is Text or some other valid fileformat which is not supported by Iceberg?
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/parse/StorageFormat.java
##########
@@ -81,11 +84,34 @@ public boolean fillStorageFormat(ASTNode child) throws SemanticException {
}
break;
case HiveParser.TOK_STORAGEHANDLER:
- storageHandler = processStorageHandler(child.getChild(0).getText());
- if (child.getChildCount() == 2) {
- BaseSemanticAnalyzer.readProps(
- (ASTNode) (child.getChild(1).getChild(0)),
- serdeProps);
+ for (int i = 0; i < child.getChildCount(); i++) {
Review comment:
What happens when StorageHandler is HBase and Stored By is also set?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] lcspinter commented on a change in pull request #2348: HIVE-25194: Add support for STORED AS ORC/PARQUET/AVRO for Iceberg
Posted by GitBox <gi...@apache.org>.
lcspinter commented on a change in pull request #2348:
URL: https://github.com/apache/hive/pull/2348#discussion_r645358025
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/parse/StorageFormat.java
##########
@@ -81,11 +84,34 @@ public boolean fillStorageFormat(ASTNode child) throws SemanticException {
}
break;
case HiveParser.TOK_STORAGEHANDLER:
- storageHandler = processStorageHandler(child.getChild(0).getText());
- if (child.getChildCount() == 2) {
- BaseSemanticAnalyzer.readProps(
- (ASTNode) (child.getChild(1).getChild(0)),
- serdeProps);
+ for (int i = 0; i < child.getChildCount(); i++) {
+ ASTNode grandChild = (ASTNode) child.getChild(i);
+ switch (grandChild.getToken().getType()) {
+ case HiveParser.TOK_FILEFORMAT_GENERIC:
+ String fileFormatPropertyKey = null;
+ try {
+ HiveStorageHandler handler = HiveUtils.getStorageHandler(conf, this.storageHandler);
+ fileFormatPropertyKey = handler.getFileFormatPropertyKey();
+ } catch (HiveException e) {
+ throw new SemanticException("Failed to load storage handler: " + e.getMessage());
+ }
+
+ if (fileFormatPropertyKey != null) {
+ String fileFormat = grandChild.getChild(0).getText();
+ if (serdeProps.containsKey(fileFormatPropertyKey)) {
+ throw new SemanticException("Provide only one of the following: STORED BY " + fileFormat +
+ " or WITH SERDEPROPERTIES('" + fileFormatPropertyKey + "'='" + fileFormat + "')");
+ }
+
+ serdeProps.put(fileFormatPropertyKey, fileFormat);
Review comment:
If the provided file format is not supported, iceberg defaults back to `avro`. After the table creation, the metadata shows the not supported file format, but right after the first insert the property value is updated to `avro`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] lcspinter merged pull request #2348: HIVE-25194: Add support for STORED AS ORC/PARQUET/AVRO for Iceberg
Posted by GitBox <gi...@apache.org>.
lcspinter merged pull request #2348:
URL: https://github.com/apache/hive/pull/2348
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] marton-bod commented on a change in pull request #2348: HIVE-25194: Add support for STORED AS ORC/PARQUET/AVRO for Iceberg
Posted by GitBox <gi...@apache.org>.
marton-bod commented on a change in pull request #2348:
URL: https://github.com/apache/hive/pull/2348#discussion_r645508438
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/parse/StorageFormat.java
##########
@@ -81,11 +84,37 @@ public boolean fillStorageFormat(ASTNode child) throws SemanticException {
}
break;
case HiveParser.TOK_STORAGEHANDLER:
- storageHandler = processStorageHandler(child.getChild(0).getText());
- if (child.getChildCount() == 2) {
- BaseSemanticAnalyzer.readProps(
- (ASTNode) (child.getChild(1).getChild(0)),
- serdeProps);
+ for (int i = 0; i < child.getChildCount(); i++) {
+ ASTNode grandChild = (ASTNode) child.getChild(i);
+ switch (grandChild.getToken().getType()) {
+ case HiveParser.TOK_FILEFORMAT_GENERIC:
+ HiveStorageHandler handler;
+ try {
+ handler = HiveUtils.getStorageHandler(conf, storageHandler);
+ } catch (HiveException e) {
+ throw new SemanticException("Failed to load storage handler: " + e.getMessage());
+ }
+
+ String fileFormatPropertyKey = handler.getFileFormatPropertyKey();
+ if (fileFormatPropertyKey != null) {
+ String fileFormat = grandChild.getChild(0).getText();
+ if (serdeProps.containsKey(fileFormatPropertyKey)) {
+ throw new SemanticException("Provide only one of the following: STORED BY " + fileFormat +
+ " or WITH SERDEPROPERTIES('" + fileFormatPropertyKey + "'='" + fileFormat + "')");
+ }
+
+ serdeProps.put(fileFormatPropertyKey, fileFormat);
+ } else {
+ throw new SemanticException("STORED BY is not supported for storage handler " +
Review comment:
Shouldn't this be `STORED AS is not supported..`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] lcspinter commented on a change in pull request #2348: HIVE-25194: Add support for STORED AS ORC/PARQUET/AVRO for Iceberg
Posted by GitBox <gi...@apache.org>.
lcspinter commented on a change in pull request #2348:
URL: https://github.com/apache/hive/pull/2348#discussion_r645345788
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/parse/StorageFormat.java
##########
@@ -81,11 +84,34 @@ public boolean fillStorageFormat(ASTNode child) throws SemanticException {
}
break;
case HiveParser.TOK_STORAGEHANDLER:
- storageHandler = processStorageHandler(child.getChild(0).getText());
- if (child.getChildCount() == 2) {
- BaseSemanticAnalyzer.readProps(
- (ASTNode) (child.getChild(1).getChild(0)),
- serdeProps);
+ for (int i = 0; i < child.getChildCount(); i++) {
Review comment:
In this case, the `storagehandler.getFileFormatPropertyKey` would return `null` and the fileformat property would be disregarded. But I see your point here, maybe we should warn the end user about this.
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/parse/StorageFormat.java
##########
@@ -81,11 +84,34 @@ public boolean fillStorageFormat(ASTNode child) throws SemanticException {
}
break;
case HiveParser.TOK_STORAGEHANDLER:
- storageHandler = processStorageHandler(child.getChild(0).getText());
- if (child.getChildCount() == 2) {
- BaseSemanticAnalyzer.readProps(
- (ASTNode) (child.getChild(1).getChild(0)),
- serdeProps);
+ for (int i = 0; i < child.getChildCount(); i++) {
+ ASTNode grandChild = (ASTNode) child.getChild(i);
+ switch (grandChild.getToken().getType()) {
+ case HiveParser.TOK_FILEFORMAT_GENERIC:
+ String fileFormatPropertyKey = null;
+ try {
+ HiveStorageHandler handler = HiveUtils.getStorageHandler(conf, this.storageHandler);
+ fileFormatPropertyKey = handler.getFileFormatPropertyKey();
+ } catch (HiveException e) {
+ throw new SemanticException("Failed to load storage handler: " + e.getMessage());
+ }
+
+ if (fileFormatPropertyKey != null) {
+ String fileFormat = grandChild.getChild(0).getText();
+ if (serdeProps.containsKey(fileFormatPropertyKey)) {
+ throw new SemanticException("Provide only one of the following: STORED BY " + fileFormat +
+ " or WITH SERDEPROPERTIES('" + fileFormatPropertyKey + "'='" + fileFormat + "')");
+ }
+
+ serdeProps.put(fileFormatPropertyKey, fileFormat);
Review comment:
If the provided file format is not supported, iceberg defaults back to `avro`. After the table creation, the metadata shows the not supported file format, but right after the first insert the property value is updated to `avro`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] lcspinter commented on a change in pull request #2348: HIVE-25194: Add support for STORED AS ORC/PARQUET/AVRO for Iceberg
Posted by GitBox <gi...@apache.org>.
lcspinter commented on a change in pull request #2348:
URL: https://github.com/apache/hive/pull/2348#discussion_r646317509
##########
File path: iceberg/iceberg-handler/src/test/queries/negative/create_iceberg_table_stored_as_with_serdeproperties_failure.q
##########
@@ -0,0 +1,3 @@
+set hive.vectorized.execution.enabled=false;
Review comment:
I've added one to the HBase qtests.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] lcspinter commented on a change in pull request #2348: HIVE-25194: Add support for STORED AS ORC/PARQUET/AVRO for Iceberg
Posted by GitBox <gi...@apache.org>.
lcspinter commented on a change in pull request #2348:
URL: https://github.com/apache/hive/pull/2348#discussion_r647356190
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java
##########
@@ -13477,15 +13477,28 @@ ASTNode analyzeCreateTable(
}
}
- if (partitionTransformSpecExists) {
- try {
- HiveStorageHandler storageHandler = HiveUtils.getStorageHandler(conf, storageFormat.getStorageHandler());
- if (!storageHandler.supportsPartitionTransform()) {
- throw new SemanticException("Partition transform is not supported for " +
- storageHandler.getClass().getName());
+ HiveStorageHandler handler;
+ try {
+ handler = HiveUtils.getStorageHandler(conf, storageFormat.getStorageHandler());
Review comment:
Yes, the storage handler can be null in the case of native tables, but this is handled inside of `HiveUtils.getStorageHandler()`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] marton-bod commented on a change in pull request #2348: HIVE-25194: Add support for STORED AS ORC/PARQUET/AVRO for Iceberg
Posted by GitBox <gi...@apache.org>.
marton-bod commented on a change in pull request #2348:
URL: https://github.com/apache/hive/pull/2348#discussion_r646390216
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java
##########
@@ -13456,6 +13457,26 @@ ASTNode analyzeCreateTable(
}
}
+ HiveStorageHandler handler = null;
+ try {
+ handler = HiveUtils.getStorageHandler(conf, storageFormat.getStorageHandler());
+ } catch (HiveException e) {
+ throw new SemanticException("Failed to load storage handler: " + e.getMessage());
+ }
+
+ if (handler != null) {
+ String fileFormatPropertyKey = handler.getFileFormatPropertyKey();
+ if (fileFormatPropertyKey != null) {
+ if (tblProps != null && tblProps.containsKey(fileFormatPropertyKey) && storageFormat.getSerdeProps() != null &&
Review comment:
A quick clarifying question: this section handles the case when both `STORED AS ORC` and `TBLPROPERTIES` are defined in the same DDL query? If so, why do we need the `&& storageFormat.getSerdeProps() != null` part? If we used `||` instead, would that work too for validating both the tblproperties and the serdeproperties case?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org