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.

Sign up for GitHub

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

Jump to bottom

[SPARK-47255][SQL] Assign names to the error classes _LEGACY_ERROR_TEMP_323[6-7] and _LEGACY_ERROR_TEMP_324[7-9] #45423

Closed
wants to merge 4 commits into from
Closed

[SPARK-47255][SQL] Assign names to the error classes _LEGACY_ERROR_TEMP_323[6-7] and _LEGACY_ERROR_TEMP_324[7-9] #45423

wants to merge 4 commits into from

Conversation

miland-db
Copy link
Contributor

@miland-db miland-db commented Mar 7, 2024 •
edited

What changes were proposed in this pull request?

In the PR, I propose to assign the proper names to the legacy error classes _LEGACY_ERROR_TEMP_323[6-7] and _LEGACY_ERROR_TEMP_324[7-9], and modify tests in testing suites to reflect these changes and use checkError() function. Also this PR improves the error messages.

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?

Error _LEGACY_ERROR_TEMP_3249 is tested by running the modified test suite:
$ build/sbt "catalyst/testOnly *RowJsonSuite"

Errors _LEGACY_ERROR_TEMP_323[6-7] and _LEGACY_ERROR_TEMP_324[7-8] are tested by running the modified test suite:
$ build/sbt "catalyst/testOnly *CSVExprUtilsSuite"

Golden files are regenerated using the:
$ SPARK_GENERATE_GOLDEN_FILES=1
$ build/sbt "core/testOnly *SparkThrowableSuite"

Was this patch authored or co-authored using generative AI tooling?

No

@github-actions github-actions bot added the SQL label Mar 7, 2024
@miland-db miland-db closed this Mar 7, 2024
@miland-db miland-db reopened this Mar 7, 2024
@HyukjinKwon
Copy link
Member

Mind filing a JIRA and linking it to the PR title please?

@HyukjinKwon
Copy link
Member

See also https://spark.apache.org/contributing.html

@miland-db miland-db changed the title Miland db/miland legacy error class [WIP][SPARK-47255][SQL] Assign names to the error classes _LEGACY_ERROR_TEMP_324[7-9] Mar 8, 2024
@miland-db
Copy link
Contributor Author

I corrected the title, and I am attaching the JIRA link: https://issues.apache.org/jira/browse/SPARK-47255

MaxGekk
MaxGekk requested changes Mar 8, 2024
View reviewed changes
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.

@miland-db As you are here, could you assign proper names to:

spark/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/csv/CSVExprUtils.scala

Lines 83 to 88 in 640ed4f

case Seq('\\', _) =>
throw new SparkIllegalArgumentException(
errorClass = "_LEGACY_ERROR_TEMP_3236", messageParameters = Map("str" -> str))
case _ =>
throw new SparkIllegalArgumentException(
errorClass = "_LEGACY_ERROR_TEMP_3237", messageParameters = Map("str" -> str))

common/utils/src/main/resources/error/error-classes.json Outdated Show resolved Hide resolved
common/utils/src/main/resources/error/error-classes.json Outdated Show resolved Hide resolved
common/utils/src/main/resources/error/error-classes.json Outdated Show resolved Hide resolved
common/utils/src/main/resources/error/error-classes.json Outdated Show resolved Hide resolved
common/utils/src/main/resources/error/error-classes.json Outdated Show resolved Hide resolved
@github-actions github-actions bot added the DOCS label Mar 11, 2024
@miland-db miland-db requested a review from MaxGekk March 11, 2024 09:30
@miland-db miland-db changed the title [WIP][SPARK-47255][SQL] Assign names to the error classes _LEGACY_ERROR_TEMP_324[7-9] [SPARK-47255][SQL] Assign names to the error classes _LEGACY_ERROR_TEMP_324[7-9] and _LEGACY_ERROR_TEMP_323[6-7] Mar 11, 2024
@miland-db miland-db changed the title [SPARK-47255][SQL] Assign names to the error classes _LEGACY_ERROR_TEMP_324[7-9] and _LEGACY_ERROR_TEMP_323[6-7] [SPARK-47255][SQL] Assign names to the error classes _LEGACY_ERROR_TEMP_323[6-7] and _LEGACY_ERROR_TEMP_324[7-9] Mar 11, 2024
@MaxGekk
Copy link
Member

MaxGekk commented Mar 11, 2024

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

@MaxGekk MaxGekk closed this in 10be032 Mar 11, 2024
@MaxGekk
Copy link
Member

MaxGekk commented Mar 11, 2024

@miland-db Congratulations with your first contribution to Apache Spark!

@miland-db
Copy link
Contributor Author

Thank you! @MaxGekk and thank you @HyukjinKwon for the comments

jpcorreia99 pushed a commit to jpcorreia99/spark that referenced this pull request Mar 12, 2024
@miland-db @jpcorreia99
[SPARK-47255][SQL] Assign names to the error classes _LEGACY_ERROR_TE…
5b40b53 
…MP_323[6-7] and _LEGACY_ERROR_TEMP_324[7-9]

### What changes were proposed in this pull request?
In the PR, I propose to assign the proper names to the legacy error classes _LEGACY_ERROR_TEMP_323[6-7] and _LEGACY_ERROR_TEMP_324[7-9], and modify tests in testing suites to reflect these changes and use checkError() function. Also this PR improves the error messages.

### 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?
Error  _LEGACY_ERROR_TEMP_3249 is tested by running the modified test suite:
`$ build/sbt  "catalyst/testOnly *RowJsonSuite"`

Errors _LEGACY_ERROR_TEMP_323[6-7] and _LEGACY_ERROR_TEMP_324[7-8] are tested by running the modified test suite:
`$ build/sbt  "catalyst/testOnly *CSVExprUtilsSuite"`

Golden files are regenerated using the:
`$ SPARK_GENERATE_GOLDEN_FILES=1`
`$ build/sbt "core/testOnly *SparkThrowableSuite"`

### Was this patch authored or co-authored using generative AI tooling?
No

Closes apache#45423 from miland-db/miland-db/miland-legacy-error-class.

Authored-by: Milan Dankovic <milan.dankovic@databricks.com>
Signed-off-by: Max Gekk <max.gekk@gmail.com>
sweisdb pushed a commit to sweisdb/spark that referenced this pull request Apr 1, 2024
@miland-db @sweisdb
[SPARK-47255][SQL] Assign names to the error classes _LEGACY_ERROR_TE…
0bf6e21 
…MP_323[6-7] and _LEGACY_ERROR_TEMP_324[7-9]

### What changes were proposed in this pull request?
In the PR, I propose to assign the proper names to the legacy error classes _LEGACY_ERROR_TEMP_323[6-7] and _LEGACY_ERROR_TEMP_324[7-9], and modify tests in testing suites to reflect these changes and use checkError() function. Also this PR improves the error messages.

### 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?
Error  _LEGACY_ERROR_TEMP_3249 is tested by running the modified test suite:
`$ build/sbt  "catalyst/testOnly *RowJsonSuite"`

Errors _LEGACY_ERROR_TEMP_323[6-7] and _LEGACY_ERROR_TEMP_324[7-8] are tested by running the modified test suite:
`$ build/sbt  "catalyst/testOnly *CSVExprUtilsSuite"`

Golden files are regenerated using the:
`$ SPARK_GENERATE_GOLDEN_FILES=1`
`$ build/sbt "core/testOnly *SparkThrowableSuite"`

### Was this patch authored or co-authored using generative AI tooling?
No

Closes apache#45423 from miland-db/miland-db/miland-legacy-error-class.

Authored-by: Milan Dankovic <milan.dankovic@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
Reviewers

@MaxGekk MaxGekk MaxGekk approved these changes

Assignees
No one assigned
Labels
DOCS SQL
Projects
None yet
Milestone
No milestone
3 participants
@miland-db @HyukjinKwon @MaxGekk