Skip to content

Conversation

@allisonwang-db
Copy link
Contributor

@allisonwang-db allisonwang-db commented Feb 6, 2023

What changes were proposed in this pull request?

This PR adds a new error class INVALID_TEMP_OBJ_REFERENCE and replaces two existing error classes with this new one:

  • _LEGACY_ERROR_TEMP_1283
  • _LEGACY_ERROR_TEMP_1284

Why are the changes needed?

To improve the error messages.

Does this PR introduce any user-facing change?

Yes, the PR changes a user-facing error message.

How was this patch tested?

Existing unit tests.

},
"CREATE_PERSISTENT_OBJECT_OVER_TEMP_OBJECT" : {
"message" : [
"Cannot create a persistent <obj> <objName> by referencing a temporary <tempObj> <tempObjName>. Please make the temporary <tempObj> persistent, or make the persistent <obj> temporary."
Copy link
Member

Choose a reason for hiding this comment

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

Is there any difference among persistent and permanent? The original errors talk about permanent but your named the object differently. Just wonder why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think they are used interchangeably in Spark. Any preference here @srielau?

Copy link
Member

@MaxGekk MaxGekk left a comment

Choose a reason for hiding this comment

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

@allisonwang-db Please, update PR's title and description w/ new error class name.

"name" -> name.toString,
"nameParts" -> nameParts))
"obj" -> "view",
"objName" -> name.toString,
Copy link
Member

@MaxGekk MaxGekk Feb 17, 2023

Choose a reason for hiding this comment

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

Could you quote the table id by using toSQLId:

Suggested change
"objName" -> name.toString,
"objName" -> toSQLId(name.nameParts),

"obj" -> "view",
"objName" -> name.toString,
"tempObj" -> "view",
"tempObjName" -> nameParts))
Copy link
Member

Choose a reason for hiding this comment

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

The quote() method uses quoteIfNeeded, please, use toSQLId to quote the name (it always quotes ids).

"name" -> name.toString,
"funcName" -> funcName))
"obj" -> "view",
"objName" -> name.toString,
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
"objName" -> name.toString,
"objName" -> toSQLId(name.nameParts),

"obj" -> "view",
"objName" -> name.toString,
"tempObj" -> "function",
"tempObjName" -> funcName))
Copy link
Member

Choose a reason for hiding this comment

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

Please, quote function id using toSQLId().

@allisonwang-db allisonwang-db changed the title [SPARK-42337][SQL] Add error class CREATE_PERSISTENT_OBJECT_OVER_TEMP_OBJECT [SPARK-42337][SQL] Add error class INVALID_TEMP_OBJ_REFERENCE Feb 18, 2023
@MaxGekk
Copy link
Member

MaxGekk commented Feb 18, 2023

The kub test is not related to this PR, I think:

[info] - SPARK-38187: Run SparkPi Jobs with minCPU *** FAILED *** (3 minutes, 1 second)
[info]   The code passed to eventually never returned normally. Attempted 189 times over 3.0106043718 minutes. Last failure message: 0 did not equal 2. (VolcanoTestsSuite.scala:302)

@MaxGekk
Copy link
Member

MaxGekk commented Feb 18, 2023

+1, LGTM. Merging to master.
Thank you, @allisonwang-db.

@MaxGekk MaxGekk closed this in f99580e Feb 18, 2023
MaxGekk pushed a commit that referenced this pull request Mar 1, 2023
…P_OBJ_REFERENCE

### What changes were proposed in this pull request?

This PR is a follow-up for #39910. It updates the error message of the error class INVALID_TEMP_OBJ_REFERENCE.

### Why are the changes needed?

To make the error message more user-friendly.

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

Yes. This PR updates the error message for INVALID_TEMP_OBJ_REFERENCE.

### How was this patch tested?

Existing tests

Closes #40198 from allisonwang-db/spark-42337-follow-up.

Authored-by: allisonwang-db <allison.wang@databricks.com>
Signed-off-by: Max Gekk <max.gekk@gmail.com>
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