Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SPARK-43028][SQL] Add error class SQL_CONF_NOT_FOUND #40660

Closed

Conversation

allisonwang-db
Copy link
Contributor

What changes were proposed in this pull request?

This PR adds a new error class SQL_CONF_NOT_FOUND.

Why are the changes needed?

To make the error message more user-friendly when getting a non-existing SQL config. For example:

spark.conf.get("some.conf")

Before this PR, it will throw this error:

java.util.NoSuchElementException: some.conf

After this PR:

[SQL_CONF_NOT_FOUND] The SQL config "some.conf" cannot be found. Please verify that the config exists.

Does this PR introduce any user-facing change?

Yes. The error message will be changed.

How was this patch tested?

Added a new UT.

@@ -1414,6 +1414,11 @@
"sortBy must be used together with bucketBy."
]
},
"SQL_CONF_NOT_FOUND" : {
"message" : [
"The SQL config \"<key>\" cannot be found. Please verify that the config exists."
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 the config in the code and remove "" around the key:

Suggested change
"The SQL config \"<key>\" cannot be found. Please verify that the config exists."
"The SQL config <sqlConf> cannot be found. Please verify that the config exists."

def sqlConfigNotFoundError(key: String): SparkRuntimeException = {
new SparkRuntimeException(
errorClass = "SQL_CONF_NOT_FOUND",
messageParameters = Map("key" -> key))
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 the config using toSQLConf()

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.

Waiting for CI.

@allisonwang-db
Copy link
Contributor Author

cc @srielau

@MaxGekk
Copy link
Member

MaxGekk commented Apr 6, 2023

@allisonwang-db Could you fix the R test:

══ Failed ══════════════════════════════════════════════════════════════════════
── 1. Failure ('test_sparkSQL.R:3963'): Setting and getting config on SparkSessi
`sparkR.conf("completely.dummy")` threw an error with unexpected message.
Expected match: "Config 'completely.dummy' is not set"
Actual message: "Unknown error: Error in handleErrors(returnStatus, conn): org.apache.spark.SparkNoSuchElementException: 

@github-actions github-actions bot added the R label Apr 6, 2023
@MaxGekk
Copy link
Member

MaxGekk commented Apr 11, 2023

+1, LGTM. Merging to master.
Thank you, @allisonwang-db and @srielau for review.

@MaxGekk MaxGekk closed this in d4134a8 Apr 11, 2023
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