-
Notifications
You must be signed in to change notification settings - Fork 29k
[SPARK-40965][SQL] Rename the error class _LEGACY_ERROR_TEMP_1208 to FIELD_NOT_FOUND
#38435
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
Conversation
FIELD_NOT_FOUND_LEGACY_ERROR_TEMP_1208 to FIELD_NOT_FOUND
_LEGACY_ERROR_TEMP_1208 to FIELD_NOT_FOUND_LEGACY_ERROR_TEMP_1208 to FIELD_NOT_FOUND
_LEGACY_ERROR_TEMP_1208 to FIELD_NOT_FOUND_LEGACY_ERROR_TEMP_1208 to FIELD_NOT_FOUND
|
@cloud-fan @itholic @srielau @panbingkun @LuciferYang Could you review this PR, please. |
srielau
left a comment
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.
LGTM
LuciferYang
left a comment
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
itholic
left a comment
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.
LGTM
|
Merging to master. Thank you, @itholic @LuciferYang @srielau for review. |
| ], | ||
| "sqlState" : "22023" | ||
| }, | ||
| "FIELD_NOT_FOUND" : { |
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.
I'm wondering if we should unify column and field (which is nested column) in the error message. cc @srielau
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.
Well, that was kind of my earlier question. How is this different from UNRESOLVED_COLUMN? (Which does include fields in the message). But it seems we KNOW that we are talking about a field here (DDL?)
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.
But it seems we KNOW that we are talking about a field here (DDL?)
Even more specifically, we look for a field in a struct, and the struct doesn't have such field.
…to `FIELD_NOT_FOUND` ### What changes were proposed in this pull request? In the PR, I propose to assign the proper name `FIELD_NOT_FOUND ` to the legacy error class `_LEGACY_ERROR_TEMP_1208 `, and modify test suite to use `checkError()` which checks the error class name, context and etc. ### Why are the changes needed? Proper name improves user experience w/ Spark SQL. ### Does this PR introduce _any_ user-facing change? Yes, the PR changes an user-facing error message. ### How was this patch tested? By running the modified test suites: ``` $ build/sbt "test:testOnly *AnalysisErrorSuite" $ build/sbt "test:testOnly *ResolveSubquerySuite" $ build/sbt "test:testOnly *EncoderResolutionSuite" $ build/sbt "test:testOnly *SQLQuerySuite" ``` Closes apache#38435 from MaxGekk/field-not-found-error-class. Authored-by: Max Gekk <max.gekk@gmail.com> Signed-off-by: Max Gekk <max.gekk@gmail.com>
What changes were proposed in this pull request?
In the PR, I propose to assign the proper name
FIELD_NOT_FOUNDto the legacy error class_LEGACY_ERROR_TEMP_1208, and modify test suite to usecheckError()which checks the error class name, context and etc.Why are the changes needed?
Proper name improves user experience w/ Spark SQL.
Does this PR introduce any user-facing change?
Yes, the PR changes an user-facing error message.
How was this patch tested?
By running the modified test suites: