Skip to content

Conversation

@panbingkun
Copy link
Contributor

@panbingkun panbingkun commented Oct 22, 2022

What changes were proposed in this pull request?

This pr replaces TypeCheckFailure by DataTypeMismatch in type checks in the misc expressions, includes:

  1. Coalesce nullExpressions.scala#L60
  2. SortOrder SortOrder.scala#L75
  3. UnwrapUDT UnwrapUDT.scala#L36
  4. ParseUrl urlExpressions.scala#L185
  5. XPathExtract xpath.scala#L45

Why are the changes needed?

Migration onto error classes unifies Spark SQL error messages.

Does this PR introduce any user-facing change?

Yes. The PR changes user-facing error messages.

How was this patch tested?

  1. Add new UT
  2. Update existed UT
  3. Pass GA

@panbingkun panbingkun changed the title [SPARK-40752][SQL] Migrate type check failures of misc expressions onto error classes [WIP][SPARK-40752][SQL] Migrate type check failures of misc expressions onto error classes Oct 22, 2022
@AmplabJenkins
Copy link

Can one of the admins verify this patch?

@MaxGekk
Copy link
Member

MaxGekk commented Oct 25, 2022

@panbingkun Please, fix the coding style issue:

[error] /home/runner/work/spark/spark/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/xml/xpath.scala:21:82: Unused import
[error] import org.apache.spark.sql.catalyst.analysis.TypeCheckResult.{DataTypeMismatch, TypeCheckFailure}

@panbingkun panbingkun changed the title [WIP][SPARK-40752][SQL] Migrate type check failures of misc expressions onto error classes [SPARK-40752][SQL] Migrate type check failures of misc expressions onto error classes Oct 28, 2022
@MaxGekk
Copy link
Member

MaxGekk commented Oct 28, 2022

@panbingkun
This pr replace TypeCheckFailure -> This pr replaces TypeCheckFailure

} else {
TypeCheckResult.TypeCheckFailure(s"cannot sort data type ${dataType.catalogString}")
DataTypeMismatch(
errorSubClass = "UNSUPPORTED_INPUT_TYPE",
Copy link
Member

Choose a reason for hiding this comment

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

Could you re-use the existing error class INVALID_ORDERING_TYPE. Just call TypeUtils.checkForOrderingExpr(...) here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@MaxGekk
Copy link
Member

MaxGekk commented Oct 28, 2022

@panbingkun Please, remove duplicate items in PR's description:

- 1.Add new UT
- 2.Update existed UT
- 3.Pass GA.

to

1. Add new UT
2. Update existed UT
3. Pass GA.

@panbingkun panbingkun requested a review from MaxGekk October 28, 2022 14:50
@panbingkun
Copy link
Contributor Author

@panbingkun This pr replace TypeCheckFailure -> This pr replaces TypeCheckFailure

Done.

@MaxGekk
Copy link
Member

MaxGekk commented Oct 29, 2022

+1, LGTM. Merging to master.
Thank you, @panbingkun.

@MaxGekk MaxGekk closed this in 2cc4e3f Oct 29, 2022
@panbingkun panbingkun deleted the SPARK-40752 branch November 7, 2022 02:01
SandishKumarHN pushed a commit to SandishKumarHN/spark that referenced this pull request Dec 12, 2022
…to error classes

### What changes were proposed in this pull request?
This pr replaces TypeCheckFailure by DataTypeMismatch in type checks in the misc expressions, includes:
1. Coalesce [nullExpressions.scala#L60](https://github.com/apache/spark/blob/1431975723d8df30a25b2333eddcfd0bb6c57677/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/nullExpressions.scala#L60)
2. SortOrder [SortOrder.scala#L75](https://github.com/apache/spark/blob/1431975723d8df30a25b2333eddcfd0bb6c57677/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/SortOrder.scala#L75)
3. UnwrapUDT [UnwrapUDT.scala#L36](https://github.com/apache/spark/blob/1431975723d8df30a25b2333eddcfd0bb6c57677/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/UnwrapUDT.scala#L36)
4. ParseUrl [urlExpressions.scala#L185](https://github.com/apache/spark/blob/1431975723d8df30a25b2333eddcfd0bb6c57677/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/urlExpressions.scala#L185)
5. XPathExtract [xpath.scala#L45](https://github.com/apache/spark/blob/a241256ed0778005245253fb147db8a16105f75c/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/xml/xpath.scala#L45)

### Why are the changes needed?
Migration onto error classes unifies Spark SQL error messages.

### Does this PR introduce _any_ user-facing change?
Yes. The PR changes user-facing error messages.

### How was this patch tested?
1. Add new UT
2. Update existed UT
3. Pass GA

Closes apache#38350 from panbingkun/SPARK-40752.

Authored-by: panbingkun <[email protected]>
Signed-off-by: Max Gekk <[email protected]>
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.

3 participants