[SPARK-47262][SQL] Assign names to error conditions for parquet conversions by vladanvasi-db · Pull Request #47802 · apache/spark

vladanvasi-db · 2024-08-19T14:33:16Z

What changes were proposed in this pull request?

In the PR, I propose to rename the legacy error classes _LEGACY_ERROR_TEMP_2238, _LEGACY_ERROR_TEMP_2239 and _LEGACY_ERROR_TEMP_2240 to PARQUET_CONVERSION_FAILURE.WITHOUT_DECIMAL_METADATA, PARQUET_CONVERSION_FAILURE.DECIMAL and PARQUET_CONVERSION_FAILURE.

Why are the changes needed?

Proper name improves user experience w/ Spark SQL.

Does this PR introduce any user-facing change?

No

How was this patch tested?

No testing was needed.

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

No

common/utils/src/main/resources/error/error-conditions.json

MaxGekk · 2024-08-21T09:09:19Z

sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryExecutionErrors.scala

-      errorClass = "_LEGACY_ERROR_TEMP_2238",
+      errorClass = "PARQUET_CONVERSION_FAILURE_WITHOUT_DECIMAL_METADATA",
      messageParameters = Map(
        "typeName" -> t.typeName,


Use toSQLType to quote it.

Suggested change

"typeName" -> t.typeName,

"typeName" -> toSQLType(t),

sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryExecutionErrors.scala

…f parquet conversion and modified parquet type in exception message to use sql format

MaxGekk · 2024-08-21T11:45:14Z

sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryExecutionErrors.scala

-      errorClass = "_LEGACY_ERROR_TEMP_2238",
+      errorClass = "PARQUET_CONVERSION_FAILURE_WITHOUT_DECIMAL_METADATA",
      messageParameters = Map(
        "typeName" -> t.typeName,


Suggested change

"typeName" -> t.typeName,

"typeName" -> toSQLType(t),

MaxGekk · 2024-08-21T11:45:43Z

sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryExecutionErrors.scala

+      errorClass = "PARQUET_CONVERSION_FAILURE.DECIMAL",
      messageParameters = Map(
-        "t" -> t.json,
+        "t" -> t.sql,


Suggested change

"t" -> t.sql,

"t" -> toSQLType(t),

MaxGekk · 2024-08-21T11:46:12Z

sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryExecutionErrors.scala

+      errorClass = "PARQUET_CONVERSION_FAILURE",
      messageParameters = Map(
-        "t" -> t.json,
+        "dataType" -> t.sql,


Suggested change

"dataType" -> t.sql,

"t" -> toSQLType(t),

MaxGekk · 2024-08-21T11:47:08Z

common/utils/src/main/resources/error/error-conditions.json

+    "subClass": {
+      "WITHOUT_DECIMAL_METADATA" : {
+        "message" : [
+          "Unable to create a Parquet converter for <typeName> whose Parquet type is <parquetType> without decimal metadata. Please read this column/field as Spark BINARY type."


I don't need to repeat parent class error message otherwise users will see duplicates.

…ion error subclasses' messages, so they do not repeat

MaxGekk

@vladanvasi-db Please, update PR's description according to your recent changes.

MaxGekk · 2024-08-21T12:12:30Z

common/utils/src/main/resources/error/error-conditions.json

+    "subClass": {
+      "WITHOUT_DECIMAL_METADATA" : {
+        "message" : [
+          " Please read this column/field as Spark BINARY type."


The gap is not needed because we merge parent's and chid's messages via a space.

Suggested change

" Please read this column/field as Spark BINARY type."

"Please read this column/field as Spark BINARY type."

MaxGekk · 2024-08-21T12:12:39Z

common/utils/src/main/resources/error/error-conditions.json

+      },
+      "DECIMAL" : {
+        "message" : [
+          " Parquet DECIMAL type can only be backed by INT32, INT64, FIXED_LEN_BYTE_ARRAY, or BINARY."


Suggested change

" Parquet DECIMAL type can only be backed by INT32, INT64, FIXED_LEN_BYTE_ARRAY, or BINARY."

"Parquet DECIMAL type can only be backed by INT32, INT64, FIXED_LEN_BYTE_ARRAY, or BINARY."

MaxGekk

LGTM. Waiting for CI.

sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryExecutionErrors.scala

MaxGekk · 2024-08-22T16:14:47Z

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

MaxGekk · 2024-08-22T16:16:37Z

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

Renamed legacy error classes for failing parquet conversions

267c135

github-actions bot added the SQL label Aug 19, 2024

vladanvasi-db changed the title ~~[SPARK-47262] Renamed legacy classes for Parquet conversion errors~~ [SPARK-47262] Changed Parquet conversion errors to throw SparkException.internal Aug 20, 2024

vladanvasi-db force-pushed the vladanvasi-db/parquet-conversion-error-classes-fix branch from 9b86702 to 267c135 Compare August 20, 2024 13:26

HyukjinKwon changed the title ~~[SPARK-47262] Changed Parquet conversion errors to throw SparkException.internal~~ [SPARK-47262][SQL] Changed Parquet conversion errors to throw SparkException.internal Aug 21, 2024

MaxGekk requested changes Aug 21, 2024

View reviewed changes

Converted parquet decimal conversion error classes into sub-classes o…

1b5fa33

…f parquet conversion and modified parquet type in exception message to use sql format

MaxGekk requested changes Aug 21, 2024

View reviewed changes

Modified display of DataType in exception message and parquet convers…

9cb64b9

…ion error subclasses' messages, so they do not repeat

MaxGekk reviewed Aug 21, 2024

View reviewed changes

Added sqlState for parquet conversion errors

9c6e30e

vladanvasi-db force-pushed the vladanvasi-db/parquet-conversion-error-classes-fix branch from da1ca27 to 9c6e30e Compare August 21, 2024 12:20

MaxGekk reviewed Aug 21, 2024

View reviewed changes

vladanvasi-db changed the title ~~[SPARK-47262][SQL] Changed Parquet conversion errors to throw SparkException.internal~~ [SPARK-47262][SQL] Renamed parquet conversion error legacy classes Aug 21, 2024

MaxGekk reviewed Aug 21, 2024

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryExecutionErrors.scala Outdated Show resolved Hide resolved

vladanvasi-db added 2 commits August 21, 2024 16:05

Added a subclass for standard parquet failing conversion error class

e68f52c

Retrigger workflow

056ebcd

MaxGekk approved these changes Aug 21, 2024

View reviewed changes

vladanvasi-db added 3 commits August 21, 2024 16:33

Empty commit for workflow retriggering

3befe0a

Reformated error-conditions modifying subclasses order and whitespace

4e1150b

Modified error message for Parquet conversion in a Parquet test suite

a0faa9d

MaxGekk closed this in 3e8cd99 Aug 22, 2024

	" Please read this column/field as Spark BINARY type."
	"Please read this column/field as Spark BINARY type."

	" Parquet DECIMAL type can only be backed by INT32, INT64, FIXED_LEN_BYTE_ARRAY, or BINARY."
	"Parquet DECIMAL type can only be backed by INT32, INT64, FIXED_LEN_BYTE_ARRAY, or BINARY."

Conversation

vladanvasi-db commented Aug 19, 2024 • edited by MaxGekk Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

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

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MaxGekk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MaxGekk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

MaxGekk commented Aug 22, 2024

Uh oh!

MaxGekk commented Aug 22, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

vladanvasi-db commented Aug 19, 2024 •

edited by MaxGekk

Loading