You are viewing a plain text version of this content. The canonical link for it is here.
Posted to gitbox@hive.apache.org by "okumin (via GitHub)" <gi...@apache.org> on 2023/12/16 06:28:36 UTC
[PR] [WIP] HIVE-27957: Better error message for STORED BY [hive]
okumin opened a new pull request, #4954:
URL: https://github.com/apache/hive/pull/4954
### What changes were proposed in this pull request?
This PR would change an error message thrown when a user gives a wrong storage format in STORED BY.
### Why are the changes needed?
The current messages are less informative. I got confused while I was testing Iceberg and non-Iceberg tables in turns.
```
0: jdbc:hive2://hive-hiveserver2:10000/defaul> CREATE TABLE test (a STRING) STORED BY ORC TBLPROPERTIES ("orc.compress"="ZSTD");
Error: Error while compiling statement: FAILED: SemanticException Cannot find class '' (state=42000,code=40000)
0: jdbc:hive2://hive-hiveserver2:10000/defaul> CREATE TABLE test (a STRING) STORED BY DEFAULT;
Error: Error while compiling statement: FAILED: NullPointerException null (state=42000,code=40000)
```
### Does this PR introduce _any_ user-facing change?
It changes only error messages.
### Is the change a dependency upgrade?
No.
### How was this patch tested?
I added test cases.
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
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
Re: [PR] HIVE-27957: Better error message for STORED BY [hive]
Posted by "abstractdog (via GitHub)" <gi...@apache.org>.
abstractdog commented on code in PR #4954:
URL: https://github.com/apache/hive/pull/4954#discussion_r1444241141
##########
ql/src/java/org/apache/hadoop/hive/ql/parse/StorageFormat.java:
##########
@@ -157,17 +165,41 @@ public boolean fillStorageFormat(ASTNode child) throws SemanticException {
return true;
}
- private String processStorageHandler(String name) throws SemanticException {
- for (StorageHandlerTypes type : StorageHandlerTypes.values()) {
- if (type.name().equalsIgnoreCase(name)) {
- name = type.className();
- inputFormat = type.inputFormat();
- outputFormat = type.outputFormat();
- break;
+ private String processStorageHandler(ASTNode node) throws SemanticException {
+ if (node.getType() == HiveParser.StringLiteral) {
+ // e.g. STORED BY 'org.apache.iceberg.mr.hive.HiveIcebergStorageHandler'
+ try {
+ return ensureClassExists(BaseSemanticAnalyzer.unescapeSQLString(node.getText()));
+ } catch (SemanticException e) {
+ throw createUnsupportedStorageHandlerTypeError(node, e);
+ }
+ }
+ if (node.getType() == HiveParser.Identifier) {
+ // e.g. STORED BY ICEBERG
+ for (StorageHandlerTypes type : StorageHandlerTypes.NON_DEFAULT_TYPES) {
+ if (type.name().equalsIgnoreCase(node.getText())) {
+ inputFormat = type.inputFormat();
+ outputFormat = type.outputFormat();
+ assert type.className != null;
Review Comment:
assert is not supposed for production AFAIK, please use explicit null checking here, e.g. [Objects.requireNonNull](https://docs.oracle.com/javase/8/docs/api/java/util/Objects.html#requireNonNull-T-java.lang.String-)
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
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
Re: [PR] HIVE-27957: Better error message for STORED BY [hive]
Posted by "okumin (via GitHub)" <gi...@apache.org>.
okumin commented on code in PR #4954:
URL: https://github.com/apache/hive/pull/4954#discussion_r1555124855
##########
ql/src/java/org/apache/hadoop/hive/ql/parse/StorageFormat.java:
##########
@@ -157,17 +165,41 @@ public boolean fillStorageFormat(ASTNode child) throws SemanticException {
return true;
}
- private String processStorageHandler(String name) throws SemanticException {
- for (StorageHandlerTypes type : StorageHandlerTypes.values()) {
- if (type.name().equalsIgnoreCase(name)) {
- name = type.className();
- inputFormat = type.inputFormat();
- outputFormat = type.outputFormat();
- break;
+ private String processStorageHandler(ASTNode node) throws SemanticException {
+ if (node.getType() == HiveParser.StringLiteral) {
+ // e.g. STORED BY 'org.apache.iceberg.mr.hive.HiveIcebergStorageHandler'
+ try {
+ return ensureClassExists(BaseSemanticAnalyzer.unescapeSQLString(node.getText()));
+ } catch (SemanticException e) {
+ throw createUnsupportedStorageHandlerTypeError(node, e);
+ }
+ }
+ if (node.getType() == HiveParser.Identifier) {
+ // e.g. STORED BY ICEBERG
+ for (StorageHandlerTypes type : StorageHandlerTypes.NON_DEFAULT_TYPES) {
+ if (type.name().equalsIgnoreCase(node.getText())) {
+ inputFormat = type.inputFormat();
+ outputFormat = type.outputFormat();
+ assert type.className != null;
Review Comment:
Thanks. You mean we'd like this validation to be effective at runtime. I replaced the assertion, rebased it based on the latest master, and then confirmed tests succeeded.
https://github.com/apache/hive/pull/4954/commits/df90d42c98f2290ab84455d3a4725fe4f5b0f1a2
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
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
Re: [PR] HIVE-27957: Better error message for STORED BY [hive]
Posted by "abstractdog (via GitHub)" <gi...@apache.org>.
abstractdog commented on code in PR #4954:
URL: https://github.com/apache/hive/pull/4954#discussion_r1444239096
##########
ql/src/java/org/apache/hadoop/hive/ql/parse/StorageFormat.java:
##########
@@ -48,9 +51,14 @@ public class StorageFormat {
public enum StorageHandlerTypes {
DEFAULT(),
- ICEBERG("\'org.apache.iceberg.mr.hive.HiveIcebergStorageHandler\'",
Review Comment:
wow, what was this "\" used for and why?
##########
ql/src/java/org/apache/hadoop/hive/ql/parse/StorageFormat.java:
##########
@@ -48,9 +51,14 @@ public class StorageFormat {
public enum StorageHandlerTypes {
DEFAULT(),
- ICEBERG("\'org.apache.iceberg.mr.hive.HiveIcebergStorageHandler\'",
Review Comment:
wow, what was this backslash used for and why?
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
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
Re: [PR] HIVE-27957: Better error message for STORED BY [hive]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #4954:
URL: https://github.com/apache/hive/pull/4954#issuecomment-1869448287
## [](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=4954) **Quality Gate passed**
Kudos, no new issues were introduced!
[0 New issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4954&resolved=false&inNewCodePeriod=true)
[0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=4954&resolved=false&inNewCodePeriod=true)
No data about Coverage
No data about Duplication
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=4954)
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
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
Re: [PR] HIVE-27957: Better error message for STORED BY [hive]
Posted by "abstractdog (via GitHub)" <gi...@apache.org>.
abstractdog commented on PR #4954:
URL: https://github.com/apache/hive/pull/4954#issuecomment-2041079185
LGTM +1
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
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
Re: [PR] HIVE-27957: Better error message for STORED BY [hive]
Posted by "aturoczy (via GitHub)" <gi...@apache.org>.
aturoczy commented on PR #4954:
URL: https://github.com/apache/hive/pull/4954#issuecomment-1880185304
@deniskuzZ @ayushtkn @abstractdog Could you pls check 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.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
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
Re: [PR] [WIP] HIVE-27957: Better error message for STORED BY [hive]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #4954:
URL: https://github.com/apache/hive/pull/4954#issuecomment-1858749372
## [](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=4954) **Quality Gate passed**
Kudos, no new issues were introduced!
[0 New issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4954&resolved=false&inNewCodePeriod=true)
[0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=4954&resolved=false&inNewCodePeriod=true)
No data about Coverage
No data about Duplication
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=4954)
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
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
Re: [PR] HIVE-27957: Better error message for STORED BY [hive]
Posted by "okumin (via GitHub)" <gi...@apache.org>.
okumin commented on code in PR #4954:
URL: https://github.com/apache/hive/pull/4954#discussion_r1430151291
##########
ql/src/test/results/clientnegative/create_table_stored_by_invalid3.q.out:
##########
@@ -0,0 +1 @@
+FAILED: SemanticException Cannot find class 'ICEBERG'
Review Comment:
Thanks for sharing your thought. Now, I also think your idea sounds more informative. Let me revise 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.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
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
Re: [PR] HIVE-27957: Better error message for STORED BY [hive]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #4954:
URL: https://github.com/apache/hive/pull/4954#issuecomment-1884168673
## [](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=4954) **Quality Gate passed**
The SonarCloud Quality Gate passed, but some issues were introduced.
[7 New issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4954&resolved=false&inNewCodePeriod=true)
[0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=4954&resolved=false&inNewCodePeriod=true)
No data about Coverage
No data about Duplication
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=4954)
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
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
Re: [PR] HIVE-27957: Better error message for STORED BY [hive]
Posted by "abstractdog (via GitHub)" <gi...@apache.org>.
abstractdog commented on code in PR #4954:
URL: https://github.com/apache/hive/pull/4954#discussion_r1444241141
##########
ql/src/java/org/apache/hadoop/hive/ql/parse/StorageFormat.java:
##########
@@ -157,17 +165,41 @@ public boolean fillStorageFormat(ASTNode child) throws SemanticException {
return true;
}
- private String processStorageHandler(String name) throws SemanticException {
- for (StorageHandlerTypes type : StorageHandlerTypes.values()) {
- if (type.name().equalsIgnoreCase(name)) {
- name = type.className();
- inputFormat = type.inputFormat();
- outputFormat = type.outputFormat();
- break;
+ private String processStorageHandler(ASTNode node) throws SemanticException {
+ if (node.getType() == HiveParser.StringLiteral) {
+ // e.g. STORED BY 'org.apache.iceberg.mr.hive.HiveIcebergStorageHandler'
+ try {
+ return ensureClassExists(BaseSemanticAnalyzer.unescapeSQLString(node.getText()));
+ } catch (SemanticException e) {
+ throw createUnsupportedStorageHandlerTypeError(node, e);
+ }
+ }
+ if (node.getType() == HiveParser.Identifier) {
+ // e.g. STORED BY ICEBERG
+ for (StorageHandlerTypes type : StorageHandlerTypes.NON_DEFAULT_TYPES) {
+ if (type.name().equalsIgnoreCase(node.getText())) {
+ inputFormat = type.inputFormat();
+ outputFormat = type.outputFormat();
+ assert type.className != null;
Review Comment:
assert is not supposed for production AFAIK, please use explicit null checking here, e.g. [Objects.requireNonNull](https://docs.oracle.com/javase/8/docs/api/java/util/Objects.html#requireNonNull-T-java.lang.String-)
this assertion can be done before assigning inpoutFormat/outputFormat
className(), I guess you might want to use method / direct access consistently
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
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
Re: [PR] HIVE-27957: Better error message for STORED BY [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on PR #4954:
URL: https://github.com/apache/hive/pull/4954#issuecomment-2041046553
@abstractdog, could this be merged or some comments are still pending?
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
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
Re: [PR] HIVE-27957: Better error message for STORED BY [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ merged PR #4954:
URL: https://github.com/apache/hive/pull/4954
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
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
Re: [PR] HIVE-27957: Better error message for STORED BY [hive]
Posted by "okumin (via GitHub)" <gi...@apache.org>.
okumin commented on code in PR #4954:
URL: https://github.com/apache/hive/pull/4954#discussion_r1434240550
##########
ql/src/java/org/apache/hadoop/hive/ql/parse/StorageFormat.java:
##########
@@ -157,17 +165,32 @@ public boolean fillStorageFormat(ASTNode child) throws SemanticException {
return true;
}
- private String processStorageHandler(String name) throws SemanticException {
- for (StorageHandlerTypes type : StorageHandlerTypes.values()) {
- if (type.name().equalsIgnoreCase(name)) {
- name = type.className();
- inputFormat = type.inputFormat();
- outputFormat = type.outputFormat();
- break;
+ private String processStorageHandler(ASTNode node) throws SemanticException {
+ if (node.getType() == HiveParser.StringLiteral) {
+ // e.g. STORED BY 'org.apache.iceberg.mr.hive.HiveIcebergStorageHandler'
+ return ensureClassExists(BaseSemanticAnalyzer.unescapeSQLString(node.getText()));
+ }
+ if (node.getType() == HiveParser.Identifier) {
+ // e.g. STORED BY ICEBERG
+ for (StorageHandlerTypes type : StorageHandlerTypes.SUPPORTED_BY_STORED_BY) {
+ if (type.name().equalsIgnoreCase(node.getText())) {
+ inputFormat = type.inputFormat();
+ outputFormat = type.outputFormat();
+ assert type.className != null;
+ // Should never fail
+ return ensureClassExists(BaseSemanticAnalyzer.unescapeSQLString(type.className));
+ }
}
}
-
- return ensureClassExists(BaseSemanticAnalyzer.unescapeSQLString(name));
+ final String supportedTypes = StorageHandlerTypes
+ .SUPPORTED_BY_STORED_BY
+ .stream()
+ .map(Enum::toString)
+ .collect(Collectors.joining(", "));
+ throw new SemanticException(String.format(
+ "Unrecognized storage handler in STORED BY clause: %s. Supported types = %s or FQCN of a storage handler",
Review Comment:
I simply followed it as it sounds better. I will update more if anyone has a better idea
https://github.com/apache/hive/pull/4954/commits/136035e9704ea9cd676d0722b83dbd4ee0310039
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
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
Re: [PR] HIVE-27957: Better error message for STORED BY [hive]
Posted by "okumin (via GitHub)" <gi...@apache.org>.
okumin commented on code in PR #4954:
URL: https://github.com/apache/hive/pull/4954#discussion_r1434237712
##########
ql/src/java/org/apache/hadoop/hive/ql/parse/StorageFormat.java:
##########
@@ -48,9 +51,14 @@ public class StorageFormat {
public enum StorageHandlerTypes {
DEFAULT(),
- ICEBERG("\'org.apache.iceberg.mr.hive.HiveIcebergStorageHandler\'",
+ ICEBERG("'org.apache.iceberg.mr.hive.HiveIcebergStorageHandler'",
"org.apache.iceberg.mr.hive.HiveIcebergInputFormat", "org.apache.iceberg.mr.hive.HiveIcebergOutputFormat");
+ private static final List<StorageHandlerTypes> SUPPORTED_BY_STORED_BY = Arrays
Review Comment:
Done
https://github.com/apache/hive/pull/4954/commits/136035e9704ea9cd676d0722b83dbd4ee0310039
##########
ql/src/test/results/clientnegative/create_table_stored_by_invalid3.q.out:
##########
@@ -0,0 +1 @@
+FAILED: SemanticException Cannot find class 'ICEBERG'
Review Comment:
Done
https://github.com/apache/hive/pull/4954/commits/136035e9704ea9cd676d0722b83dbd4ee0310039
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
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
Re: [PR] HIVE-27957: Better error message for STORED BY [hive]
Posted by "okumin (via GitHub)" <gi...@apache.org>.
okumin commented on code in PR #4954:
URL: https://github.com/apache/hive/pull/4954#discussion_r1445490410
##########
ql/src/java/org/apache/hadoop/hive/ql/parse/StorageFormat.java:
##########
@@ -48,9 +51,14 @@ public class StorageFormat {
public enum StorageHandlerTypes {
DEFAULT(),
- ICEBERG("\'org.apache.iceberg.mr.hive.HiveIcebergStorageHandler\'",
Review Comment:
I'm not sure about the original intention. IntelliJ asks me to remove the backslash and it seems to be safe to remove.
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
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
Re: [PR] HIVE-27957: Better error message for STORED BY [hive]
Posted by "abstractdog (via GitHub)" <gi...@apache.org>.
abstractdog commented on PR #4954:
URL: https://github.com/apache/hive/pull/4954#issuecomment-2041080431
> @abstractdog, could this be merged or some comments are still pending?
there are minor comments unaddressed
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
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
Re: [PR] HIVE-27957: Better error message for STORED BY [hive]
Posted by "abstractdog (via GitHub)" <gi...@apache.org>.
abstractdog commented on PR #4954:
URL: https://github.com/apache/hive/pull/4954#issuecomment-2041411551
LGTM, pending tests
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
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
Re: [PR] HIVE-27957: Better error message for STORED BY [hive]
Posted by "abstractdog (via GitHub)" <gi...@apache.org>.
abstractdog commented on code in PR #4954:
URL: https://github.com/apache/hive/pull/4954#discussion_r1554585576
##########
ql/src/java/org/apache/hadoop/hive/ql/parse/StorageFormat.java:
##########
@@ -157,17 +165,41 @@ public boolean fillStorageFormat(ASTNode child) throws SemanticException {
return true;
}
- private String processStorageHandler(String name) throws SemanticException {
- for (StorageHandlerTypes type : StorageHandlerTypes.values()) {
- if (type.name().equalsIgnoreCase(name)) {
- name = type.className();
- inputFormat = type.inputFormat();
- outputFormat = type.outputFormat();
- break;
+ private String processStorageHandler(ASTNode node) throws SemanticException {
+ if (node.getType() == HiveParser.StringLiteral) {
+ // e.g. STORED BY 'org.apache.iceberg.mr.hive.HiveIcebergStorageHandler'
+ try {
+ return ensureClassExists(BaseSemanticAnalyzer.unescapeSQLString(node.getText()));
+ } catch (SemanticException e) {
+ throw createUnsupportedStorageHandlerTypeError(node, e);
+ }
+ }
+ if (node.getType() == HiveParser.Identifier) {
+ // e.g. STORED BY ICEBERG
+ for (StorageHandlerTypes type : StorageHandlerTypes.NON_DEFAULT_TYPES) {
+ if (type.name().equalsIgnoreCase(node.getText())) {
+ inputFormat = type.inputFormat();
+ outputFormat = type.outputFormat();
+ assert type.className != null;
Review Comment:
the problem with assert is that it's not meant for production, it can be completely bypassed with JVM args
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
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
Re: [PR] HIVE-27957: Better error message for STORED BY [hive]
Posted by "akshat0395 (via GitHub)" <gi...@apache.org>.
akshat0395 commented on code in PR #4954:
URL: https://github.com/apache/hive/pull/4954#discussion_r1429180157
##########
ql/src/java/org/apache/hadoop/hive/ql/parse/StorageFormat.java:
##########
@@ -48,9 +51,14 @@ public class StorageFormat {
public enum StorageHandlerTypes {
DEFAULT(),
- ICEBERG("\'org.apache.iceberg.mr.hive.HiveIcebergStorageHandler\'",
+ ICEBERG("'org.apache.iceberg.mr.hive.HiveIcebergStorageHandler'",
"org.apache.iceberg.mr.hive.HiveIcebergInputFormat", "org.apache.iceberg.mr.hive.HiveIcebergOutputFormat");
+ private static final List<StorageHandlerTypes> SUPPORTED_BY_STORED_BY = Arrays
Review Comment:
nit: Can we rename this to SUPPORTED_STORED_BY?
##########
ql/src/test/results/clientnegative/create_table_stored_by_invalid3.q.out:
##########
@@ -0,0 +1 @@
+FAILED: SemanticException Cannot find class 'ICEBERG'
Review Comment:
Thanks for checking this out, the message looks clean and precise now.
Can you also handle case this ('ICEBERG') to keep the message consistent? We can have something like `Unrecognized storage handler in STORED BY clause: 'ICEBERG'. Supported types = ICEBERG or FQCN of a storage handler.`
This should be an indicator enough for the user to remove the quotes. WDYT
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
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
Re: [PR] HIVE-27957: Better error message for STORED BY [hive]
Posted by "aturoczy (via GitHub)" <gi...@apache.org>.
aturoczy commented on code in PR #4954:
URL: https://github.com/apache/hive/pull/4954#discussion_r1430385183
##########
ql/src/java/org/apache/hadoop/hive/ql/parse/StorageFormat.java:
##########
@@ -157,17 +165,32 @@ public boolean fillStorageFormat(ASTNode child) throws SemanticException {
return true;
}
- private String processStorageHandler(String name) throws SemanticException {
- for (StorageHandlerTypes type : StorageHandlerTypes.values()) {
- if (type.name().equalsIgnoreCase(name)) {
- name = type.className();
- inputFormat = type.inputFormat();
- outputFormat = type.outputFormat();
- break;
+ private String processStorageHandler(ASTNode node) throws SemanticException {
+ if (node.getType() == HiveParser.StringLiteral) {
+ // e.g. STORED BY 'org.apache.iceberg.mr.hive.HiveIcebergStorageHandler'
+ return ensureClassExists(BaseSemanticAnalyzer.unescapeSQLString(node.getText()));
+ }
+ if (node.getType() == HiveParser.Identifier) {
+ // e.g. STORED BY ICEBERG
+ for (StorageHandlerTypes type : StorageHandlerTypes.SUPPORTED_BY_STORED_BY) {
+ if (type.name().equalsIgnoreCase(node.getText())) {
+ inputFormat = type.inputFormat();
+ outputFormat = type.outputFormat();
+ assert type.className != null;
+ // Should never fail
+ return ensureClassExists(BaseSemanticAnalyzer.unescapeSQLString(type.className));
+ }
}
}
-
- return ensureClassExists(BaseSemanticAnalyzer.unescapeSQLString(name));
+ final String supportedTypes = StorageHandlerTypes
+ .SUPPORTED_BY_STORED_BY
+ .stream()
+ .map(Enum::toString)
+ .collect(Collectors.joining(", "));
+ throw new SemanticException(String.format(
+ "Unrecognized storage handler in STORED BY clause: %s. Supported types = %s or FQCN of a storage handler",
Review Comment:
I like this initiative because hive error message is too massy, and need a pilot certificate to understand it. So big plus for it!
On the other hand, I would recommend the following message like:
The storage handler specified in the STORED BY clause is not recognized: %s. Please use one of the supported types, which are %s, or provide the Fully Qualified Class Name (FQCN) of a valid storage handler.
Even thinking the second half of the sentence is too technical.
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
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
Re: [PR] HIVE-27957: Better error message for STORED BY [hive]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #4954:
URL: https://github.com/apache/hive/pull/4954#issuecomment-1869892701
## [](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=4954) **Quality Gate passed**
Kudos, no new issues were introduced!
[0 New issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4954&resolved=false&inNewCodePeriod=true)
[0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=4954&resolved=false&inNewCodePeriod=true)
No data about Coverage
No data about Duplication
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=4954)
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
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
Re: [PR] HIVE-27957: Better error message for STORED BY [hive]
Posted by "akshat0395 (via GitHub)" <gi...@apache.org>.
akshat0395 commented on PR #4954:
URL: https://github.com/apache/hive/pull/4954#issuecomment-1870850252
Changes LGTM +1, Thanks
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
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
Re: [PR] HIVE-27957: Better error message for STORED BY [hive]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #4954:
URL: https://github.com/apache/hive/pull/4954#issuecomment-1987429923
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Feel free to reach out on the dev@hive.apache.org list if the patch is in need of reviews.
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
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
Re: [PR] HIVE-27957: Better error message for STORED BY [hive]
Posted by "okumin (via GitHub)" <gi...@apache.org>.
okumin commented on code in PR #4954:
URL: https://github.com/apache/hive/pull/4954#discussion_r1445499914
##########
ql/src/java/org/apache/hadoop/hive/ql/parse/StorageFormat.java:
##########
@@ -157,17 +165,41 @@ public boolean fillStorageFormat(ASTNode child) throws SemanticException {
return true;
}
- private String processStorageHandler(String name) throws SemanticException {
- for (StorageHandlerTypes type : StorageHandlerTypes.values()) {
- if (type.name().equalsIgnoreCase(name)) {
- name = type.className();
- inputFormat = type.inputFormat();
- outputFormat = type.outputFormat();
- break;
+ private String processStorageHandler(ASTNode node) throws SemanticException {
+ if (node.getType() == HiveParser.StringLiteral) {
+ // e.g. STORED BY 'org.apache.iceberg.mr.hive.HiveIcebergStorageHandler'
+ try {
+ return ensureClassExists(BaseSemanticAnalyzer.unescapeSQLString(node.getText()));
+ } catch (SemanticException e) {
+ throw createUnsupportedStorageHandlerTypeError(node, e);
+ }
+ }
+ if (node.getType() == HiveParser.Identifier) {
+ // e.g. STORED BY ICEBERG
+ for (StorageHandlerTypes type : StorageHandlerTypes.NON_DEFAULT_TYPES) {
+ if (type.name().equalsIgnoreCase(node.getText())) {
+ inputFormat = type.inputFormat();
+ outputFormat = type.outputFormat();
+ assert type.className != null;
Review Comment:
In my opinion, either is fine as this doesn't validate users' input but does assert invariant conditions of enum values for readability. I mean the current implementation doesn't care about the possibility where we add an enum value with null or a wrong class name. What do you think?
As for the direct access, I will fix that.
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
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
Re: [PR] HIVE-27957: Better error message for STORED BY [hive]
Posted by "okumin (via GitHub)" <gi...@apache.org>.
okumin commented on code in PR #4954:
URL: https://github.com/apache/hive/pull/4954#discussion_r1430164116
##########
ql/src/java/org/apache/hadoop/hive/ql/parse/StorageFormat.java:
##########
@@ -48,9 +51,14 @@ public class StorageFormat {
public enum StorageHandlerTypes {
DEFAULT(),
- ICEBERG("\'org.apache.iceberg.mr.hive.HiveIcebergStorageHandler\'",
+ ICEBERG("'org.apache.iceberg.mr.hive.HiveIcebergStorageHandler'",
"org.apache.iceberg.mr.hive.HiveIcebergInputFormat", "org.apache.iceberg.mr.hive.HiveIcebergOutputFormat");
+ private static final List<StorageHandlerTypes> SUPPORTED_BY_STORED_BY = Arrays
Review Comment:
I started feeling we may not need `StorageHandlerTypes.DEFAULT`. But it seems to be a result of a discussion.
https://github.com/apache/hive/pull/4348#discussion_r1239887908
I'm thinking to rename it into like `NON_DEFAULT` for simplicity.
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
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
Re: [PR] HIVE-27957: Better error message for STORED BY [hive]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #4954:
URL: https://github.com/apache/hive/pull/4954#issuecomment-1866696385
## [](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=4954) **Quality Gate passed**
Kudos, no new issues were introduced!
[0 New issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4954&resolved=false&inNewCodePeriod=true)
[0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=4954&resolved=false&inNewCodePeriod=true)
No data about Coverage
No data about Duplication
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=4954)
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
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
Re: [PR] HIVE-27957: Better error message for STORED BY [hive]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #4954:
URL: https://github.com/apache/hive/pull/4954#issuecomment-2041417515
## [](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=4954) **Quality Gate passed**
Issues
 [0 New issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4954&resolved=false&inNewCodePeriod=true)
 [0 Accepted issues](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=4954&metric=new_accepted_issues&view=list)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=4954&resolved=false&inNewCodePeriod=true)
 No data about Coverage
 No data about Duplication
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=4954)
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
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