Skip to content

Conversation

@MaxGekk
Copy link
Member

@MaxGekk MaxGekk commented Sep 13, 2022

What changes were proposed in this pull request?

In the PR, I propose to use error classes in the case of type check failure in the CAST expression.

Why are the changes needed?

Migration onto error classes unifies Spark SQL error messages, and improves search-ability of errors.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

By running the modified test suites:

$ build/sbt "test:testOnly *CastWithAnsiOnSuite"
$ build/sbt "test:testOnly *DatasetSuite"
$ build/sbt "sql/testOnly org.apache.spark.sql.SQLQueryTestSuite -- -z cast.sql"

@MaxGekk MaxGekk changed the title [WIP][SQL] Migrate type check fails in CAST to error classes [WIP][SPARK-40370][SQL] Migrate type check fails in CAST to error classes Sep 14, 2022
@MaxGekk MaxGekk marked this pull request as ready for review September 14, 2022 14:17
@MaxGekk MaxGekk changed the title [WIP][SPARK-40370][SQL] Migrate type check fails in CAST to error classes [SPARK-40370][SQL] Migrate type check fails in CAST to error classes Sep 14, 2022
@MaxGekk MaxGekk changed the title [SPARK-40370][SQL] Migrate type check fails in CAST to error classes [SPARK-40370][SQL] Migrate type check fails to error classes in CAST Sep 14, 2022
@MaxGekk
Copy link
Member Author

MaxGekk commented Sep 14, 2022

@srielau @anchovYu @cloud-fan @gengliangwang Could you review this PR, please.

Comment on lines 621 to 622
"srcType" : "\"TINYINT\"",
"srcType" : "\"TINYINT\"",
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have a bug when an error message refers to the same param more than once. I will fix this separately.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fix is trivial. Will do in this PR.

@MaxGekk
Copy link
Member Author

MaxGekk commented Sep 15, 2022

The failure "SQLAppStatusListenerMemoryLeakSuite.no memory leak" is not related to the changes, I believe.
Merging to master. Thank you, @cloud-fan for review.

@MaxGekk MaxGekk closed this in 6d067d0 Sep 15, 2022
"If you have to cast <srcType> to <targetType>, you can set <config> as <configVal>."
]
},
"CAST_WITH_FUN_SUGGESTION" : {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: FUN is really confusing here. I think FUNC is more commonly used as a short name of FUNCTION

Copy link
Member Author

@MaxGekk MaxGekk Oct 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tell this Rust to programmers ;-)

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.

2 participants