-
Notifications
You must be signed in to change notification settings - Fork 28.9k
[SPARK-53789][SQL][CONNECT] Unify error condition CANNOT_MODIFY_CONFIG #52506
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
base: master
Are you sure you want to change the base?
Conversation
cc @MaxGekk |
cc @LuciferYang |
throw new AnalysisException( | ||
errorClass = "_LEGACY_ERROR_TEMP_3050", | ||
messageParameters = Map("k" -> k)) | ||
throw CompilationErrors.cannotModifyValueOfStaticConfigError(k) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is it necessary to direct users to refer to <docroot>/sql-migration-guide.html#ddl-statements
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@LuciferYang I think it might be because of
spark/docs/sql-migration-guide.md
Line 324 in 29434ea
- Spark 2.4 and below: the `SET` command works without any warnings even if the specified key is for `SparkConf` entries and it has no effect because the command does not update `SparkConf`, but the behavior might confuse users. In 3.0, the command fails if a `SparkConf` key is used. You can disable such a check by setting `spark.sql.legacy.setCommandRejectsSparkCoreConfs` to `false`. |

There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I see your point, here spark.sql.legacy.setCommandRejectsSparkCoreConfs
does not take effect, so the docs do not help
def cannotModifyValueOfStaticConfigError(key: String): Throwable = { | ||
new AnalysisException( | ||
errorClass = "CANNOT_MODIFY_STATIC_CONFIG", | ||
messageParameters = Map("key" -> toSQLConf(key))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@LuciferYang I updated the code to remove the doc link since <docroot>/sql-migration-guide.html#ddl-statements
does not provide useful info for this error
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1, LGTM
What changes were proposed in this pull request?
Migrate error condition
_LEGACY_ERROR_TEMP_3050
to existingCANNOT_MODIFY_STATIC_CONFIG
.Why are the changes needed?
More consistent error message.
Does this PR introduce any user-facing change?
More consistent error message.
How was this patch tested?
Pass GHA.
Was this patch authored or co-authored using generative AI tooling?
No.