Skip to content

Conversation

@Peng-Lei
Copy link
Contributor

@Peng-Lei Peng-Lei commented Jul 29, 2021

What changes were proposed in this pull request?

When we refactor the query execution errors to use error classes in QueryExecutionErrors, we need define some exception that mix SparkThrowable into a base Exception type.
according the example SparkArithmeticException

Add SparkXXXException as follows:

  • SparkClassNotFoundException
  • SparkConcurrentModificationException
  • SparkDateTimeException
  • SparkFileAlreadyExistsException
  • SparkFileNotFoundException
  • SparkNoSuchMethodException
  • SparkIndexOutOfBoundsException
  • SparkIOException
  • SparkSecurityException
  • SparkSQLException
  • SparkSQLFeatureNotSupportedException

Refactor some exceptions in QueryExecutionErrors to use error classes and new exception for testing new exception

Some added by PR as follows:

  • SparkUnsupportedOperationException
  • SparkIllegalStateException
  • SparkNumberFormatException
  • SparkIllegalArgumentException
  • SparkArrayIndexOutOfBoundsException
  • SparkNoSuchElementException

Why are the changes needed?

SPARK-36336

Does this PR introduce any user-facing change?

No

How was this patch tested?

existed ut test

@github-actions github-actions bot added the CORE label Jul 29, 2021
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@karenfeng I'd rather define cause as type of Exception, What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

cause is always a type of Throwable in the base type and it needs to be propagated, so I think we can leave it as is. But I think we can simplify this class to only accept errorClass and messageParameters (and maybe cause if needed); see SparkArithmeticException as an example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok,I will simplify this class to only accept errorClass and messageParameters,also with others. The cause is needed for it in QueryExecutionErrors. Thank you.

@Peng-Lei Peng-Lei changed the title [SPARK-36336][SQL][SQL] Add new exception of base exception used in QueryExecutionErrors [SPARK-36336][SQL] Add new exception of base exception used in QueryExecutionErrors Jul 29, 2021
@Peng-Lei Peng-Lei changed the title [SPARK-36336][SQL] Add new exception of base exception used in QueryExecutionErrors [WIP][SPARK-36336][SQL] Add new exception of base exception used in QueryExecutionErrors Jul 29, 2021
@AmplabJenkins
Copy link

Can one of the admins verify this patch?

@Peng-Lei
Copy link
Contributor Author

@karenfeng FYI

@Peng-Lei Peng-Lei changed the title [WIP][SPARK-36336][SQL] Add new exception of base exception used in QueryExecutionErrors [SPARK-36336][SQL] Add new exception of base exception used in QueryExecutionErrors Jul 29, 2021
@Peng-Lei Peng-Lei force-pushed the SPARK-36336 branch 2 times, most recently from 82eea33 to dc9dc9b Compare July 30, 2021 09:51
Copy link
Contributor

@karenfeng karenfeng left a comment

Choose a reason for hiding this comment

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

Thanks for doing this! LGTM. @HyukjinKwon, can you also take a look?

@dgd-contributor
Copy link

Any update here?

@Peng-Lei
Copy link
Contributor Author

Any update here?

Wait for 3.2 release

@Peng-Lei
Copy link
Contributor Author

@HyukjinKwon Could you take a look?

@HyukjinKwon
Copy link
Member

Merged to master.

/**
* Class not found exception thrown from Spark with an error class.
*/
class SparkClassNotFoundException(
Copy link
Contributor

Choose a reason for hiding this comment

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

shall we mark these classes as private[spark]? cc @gengliangwang

Copy link
Member

Choose a reason for hiding this comment

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

oh yeah

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cloud-fan @HyukjinKwon @gengliangwang I post the follow PR. Could you take a look?

Copy link
Member

Choose a reason for hiding this comment

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

Yes I think so

HyukjinKwon pushed a commit that referenced this pull request Aug 30, 2021
### What changes were proposed in this pull request?

Mark the exception added `private[spark]`
according [comments](#33573 (comment))

### Why are the changes needed?
[comments](#33573 (comment))

### Does this PR introduce _any_ user-facing change?
No

### How was this patch tested?
existed ut testcase

Closes #33856 from Peng-Lei/SPARK-36336-FOLLOW.

Authored-by: PengLei <[email protected]>
Signed-off-by: Hyukjin Kwon <[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.

7 participants