HIVE-27957: Better error message for STORED BY by okumin · Pull Request #4954 · apache/hive

okumin · 2023-12-16T06:28:34Z

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.

aturoczy

Minor change request

aturoczy · 2023-12-18T16:33:11Z

+        .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",


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.

I simply followed it as it sounds better. I will update more if anyone has a better idea
136035e

aturoczy

+1

akshat0395 · 2023-12-28T05:57:31Z

Changes LGTM +1, Thanks

aturoczy · 2024-01-07T21:43:50Z

@deniskuzZ @ayushtkn @abstractdog Could you pls check this?

abstractdog · 2024-01-08T07:26:06Z


  public enum StorageHandlerTypes {
    DEFAULT(),
-    ICEBERG("\'org.apache.iceberg.mr.hive.HiveIcebergStorageHandler\'",


wow, what was this backslash used for and why?

I'm not sure about the original intention. IntelliJ asks me to remove the backslash and it seems to be safe to remove.

abstractdog · 2024-01-08T07:29:19Z

+        if (type.name().equalsIgnoreCase(node.getText())) {
+          inputFormat = type.inputFormat();
+          outputFormat = type.outputFormat();
+          assert type.className != null;


assert is not supposed for production AFAIK, please use explicit null checking here, e.g. Objects.requireNonNull

this assertion can be done before assigning inpoutFormat/outputFormat

className(), I guess you might want to use method / direct access consistently

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.

the problem with assert is that it's not meant for production, it can be completely bypassed with JVM args

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.
df90d42

sonarqubecloud · 2024-01-10T04:21:54Z

Quality Gate passed

The SonarCloud Quality Gate passed, but some issues were introduced.

7 New issues
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

github-actions · 2024-03-11T00:19:59Z

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.

deniskuzZ · 2024-04-06T10:49:38Z

@abstractdog, could this be merged or some comments are still pending?

abstractdog · 2024-04-06T13:19:54Z

@abstractdog, could this be merged or some comments are still pending?

there are minor comments unaddressed

abstractdog · 2024-04-07T10:30:31Z

LGTM, pending tests

sonarqubecloud · 2024-04-07T10:38:36Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

asf-ci-hive added tests pending tests passed and removed tests pending labels Dec 16, 2023

okumin changed the title ~~[WIP] HIVE-27957: Better error message for STORED BY~~ HIVE-27957: Better error message for STORED BY Dec 16, 2023

okumin marked this pull request as ready for review December 16, 2023 11:49

akshat0395 suggested changes Dec 17, 2023

View reviewed changes

Comment thread ql/src/java/org/apache/hadoop/hive/ql/parse/StorageFormat.java Outdated

Comment thread ql/src/test/results/clientnegative/create_table_stored_by_invalid3.q.out Outdated

aturoczy suggested changes Dec 18, 2023

View reviewed changes

asf-ci-hive added tests pending tests unstable tests passed and removed tests passed tests pending tests unstable labels Dec 21, 2023

aturoczy approved these changes Dec 27, 2023

View reviewed changes

akshat0395 approved these changes Dec 28, 2023

View reviewed changes

abstractdog reviewed Jan 8, 2024

View reviewed changes

asf-ci-hive added tests pending and removed tests passed labels Jan 10, 2024

asf-ci-hive added tests passed and removed tests pending labels Jan 10, 2024

github-actions Bot added the stale label Mar 11, 2024

deniskuzZ removed the stale label Mar 13, 2024

abstractdog self-requested a review April 6, 2024 13:13

okumin added 3 commits April 7, 2024 13:50

HIVE-27957: Better error message for STORED BY

ff43349

Improve messages

47139a5

Use a method to refer to StorageHandlerTypes#className

2a9fd42

okumin force-pushed the HIVE-27957-stored-by branch from 00299c5 to d94983b Compare April 7, 2024 04:51

asf-ci-hive added tests pending and removed tests passed labels Apr 7, 2024

Check null explicitly

df90d42

okumin force-pushed the HIVE-27957-stored-by branch from d94983b to df90d42 Compare April 7, 2024 04:51

asf-ci-hive added tests failed tests pending and removed tests pending tests failed labels Apr 7, 2024

abstractdog approved these changes Apr 7, 2024

View reviewed changes

asf-ci-hive added tests passed and removed tests pending labels Apr 7, 2024

deniskuzZ merged commit cf0d4f1 into apache:master Apr 10, 2024

okumin deleted the HIVE-27957-stored-by branch April 10, 2024 11:44

Uh oh!

Conversation

okumin commented Dec 16, 2023

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

Is the change a dependency upgrade?

How was this patch tested?

Uh oh!

Uh oh!

Uh oh!

aturoczy left a comment

Choose a reason for hiding this comment

Uh oh!

aturoczy Dec 18, 2023

Choose a reason for hiding this comment

Uh oh!

okumin Dec 21, 2023

Choose a reason for hiding this comment

Uh oh!

aturoczy left a comment

Choose a reason for hiding this comment

Uh oh!

akshat0395 commented Dec 28, 2023

Uh oh!

aturoczy commented Jan 7, 2024

Uh oh!

abstractdog Jan 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

okumin Jan 8, 2024

Choose a reason for hiding this comment

Uh oh!

abstractdog Jan 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

okumin Jan 9, 2024

Choose a reason for hiding this comment

Uh oh!

abstractdog Apr 6, 2024

Choose a reason for hiding this comment

Uh oh!

okumin Apr 8, 2024

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud Bot commented Jan 10, 2024

Quality Gate passed

Uh oh!

github-actions Bot commented Mar 11, 2024

Uh oh!

deniskuzZ commented Apr 6, 2024

Uh oh!

abstractdog commented Apr 6, 2024

Uh oh!

abstractdog commented Apr 7, 2024

Uh oh!

sonarqubecloud Bot commented Apr 7, 2024

Quality Gate passed

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

abstractdog Jan 8, 2024 •

edited

Loading

abstractdog Jan 8, 2024 •

edited

Loading