Skip to content

Conversation

@ivoson
Copy link
Contributor

@ivoson ivoson commented Feb 6, 2022

What changes were proposed in this pull request?

Migrate the following errors in QueryParsingErrors onto use error classes:

  1. joinCriteriaUnimplementedError => throw IllegalStateException instead, since it should never happen and not visible to users, introduced by improving exhaustivity in PR
  2. naturalCrossJoinUnsupportedError => UNSUPPORTED_FEATURE

Why are the changes needed?

Porting join parsing errors to new error framework.

Does this PR introduce any user-facing change?

No

How was this patch tested?

UT added.

@github-actions github-actions bot added the SQL label Feb 6, 2022
@AmplabJenkins
Copy link

Can one of the admins verify this patch?

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.

INTERNAL_ERROR (should never happen, introduced by improving exhaustivity in #30455)

Let's throw IllegalStateException since it is not visible to end users (as we have already implemented that in #35302). We should use error classes only for errors that can be propagated to user space.

@ivoson
Copy link
Contributor Author

ivoson commented Feb 6, 2022

INTERNAL_ERROR (should never happen, introduced by improving exhaustivity in #30455)

Let's throw IllegalStateException since it is not visible to end users (as we have already implemented that in #35302). We should use error classes only for errors that can be propagated to user space.

Thanks @MaxGekk

@MaxGekk
Copy link
Member

MaxGekk commented Feb 6, 2022

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

@MaxGekk MaxGekk closed this in 0d56c94 Feb 6, 2022
@ivoson ivoson deleted the SPARK-38105 branch February 7, 2022 01:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants