Skip to content

Conversation

@panbingkun
Copy link
Contributor

@panbingkun panbingkun commented Dec 10, 2022

What changes were proposed in this pull request?

In the PR, I propose to assign the name ANALYZE_UNCACHED_TEMP_VIEW to the error class _LEGACY_ERROR_TEMP_1234.

Why are the changes needed?

Proper names of error classes should improve user experience with Spark SQL.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Add new UT.
Pass GA.

},
"ANALYZE_TABLE_VIEW_NOT_CACHED" : {
"message" : [
"Temporary view <tableName> is not cached for analyzing columns."
Copy link
Member

@MaxGekk MaxGekk Dec 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you improve the error message by following the common structure as in other errors in error-classes.json:

  1. State the issue
  2. Give a suggestion how to solve it.

The error could be:

Suggested change
"Temporary view <tableName> is not cached for analyzing columns."
"The ANALYZE TABLE FOR COLUMNS command can operate on temporary views that have been cached already. Consider to cache the view <viewName>."

"AES-<mode> with the padding <padding> by the <functionName> function."
]
},
"ANALYZE_TABLE_VIEW_NOT_CACHED" : {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"ANALYZE_TABLE_VIEW_NOT_CACHED" : {
"ANALYZE_UNCACHED_TEMP_VIEW" : {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@AmplabJenkins
Copy link

Can one of the admins verify this patch?

@panbingkun panbingkun requested a review from MaxGekk December 13, 2022 02:45
@MaxGekk
Copy link
Member

MaxGekk commented Dec 13, 2022

+1, LGTM. Merging to master.
Thank you, @panbingkun.

@MaxGekk MaxGekk closed this in 3809ccd Dec 13, 2022
@panbingkun panbingkun deleted the LEGACY_ERROR_TEMP_1234 branch December 15, 2022 01:39
beliefer pushed a commit to beliefer/spark that referenced this pull request Dec 18, 2022
…P_1234

### What changes were proposed in this pull request?
In the PR, I propose to assign the name ANALYZE_UNCACHED_TEMP_VIEW to the error class _LEGACY_ERROR_TEMP_1234.

### Why are the changes needed?
Proper names of error classes should improve user experience with Spark SQL.

### Does this PR introduce _any_ user-facing change?
No.

### How was this patch tested?
Add new UT.
Pass GA.

Closes apache#39018 from panbingkun/LEGACY_ERROR_TEMP_1234.

Authored-by: panbingkun <[email protected]>
Signed-off-by: Max Gekk <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants