Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SPARK-42078][PYTHON] Migrate errors thrown by JVM into PySparkException. #39591

Closed
wants to merge 8 commits into from

Conversation

itholic
Copy link
Contributor

@itholic itholic commented Jan 16, 2023

What changes were proposed in this pull request?

This PR proposes to migrate errors thrown by JVM into PySparkException.

Here is the full list of errors suggesting migration:

  • CapturedException (The parent class for every errors thrown by JVM)
    • AnalysisException
    • ParseException
    • IllegalArgumentException
    • StreamingQueryException
    • QueryExecutionException
    • PythonException
    • UnknownException
    • SparkUpgradeException

In short, this PR proposes to migrate all exceptions from pyspark/sql/utils.py into pyspark/errors/exceptions.py and fix them to inherit PySparkException.

Why are the changes needed?

Because the errors thrown by JVM side (e.g. AnalysisException, ParseException , IllegalArgumentException ... ) are managed by pyspark/sql/utils.py which is not very correct place to handle the PySpark errors so far.

Now we have separate error package pyspark.errors which is introduced from #39387, we should centralize whole errors generated by PySpark side into single management point.

So, here I suggest that all errors occurring in the JVM inherit PySparkException so that errors can be managed consistently.

Does this PR introduce any user-facing change?

No, this PR suggests to improve an internal error handling structure without any user-facing change.

How was this patch tested?

The existing CI should pass.

Copy link
Member

@HyukjinKwon HyukjinKwon left a comment

Choose a reason for hiding this comment

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

LGTM otherwise

@HyukjinKwon
Copy link
Member

Merged to master.

HyukjinKwon pushed a commit that referenced this pull request Apr 17, 2023
### What changes were proposed in this pull request?

This is follow-up for #39591 based on the suggestions from comment 1de8350#r109023188.

### Why are the changes needed?

For backward compatibility.

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

No.

### How was this patch tested?

The existing CI should pass

Closes #40816 from itholic/42078-followup.

Authored-by: itholic <haejoon.lee@databricks.com>
Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>
HyukjinKwon pushed a commit that referenced this pull request Apr 17, 2023
### What changes were proposed in this pull request?

This is follow-up for #39591 based on the suggestions from comment 1de8350#r109023188.

### Why are the changes needed?

For backward compatibility.

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

No.

### How was this patch tested?

The existing CI should pass

Closes #40816 from itholic/42078-followup.

Authored-by: itholic <haejoon.lee@databricks.com>
Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>
(cherry picked from commit 0457219)
Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>
@itholic itholic deleted the SPARK-42078 branch April 22, 2023 05:44
snmvaughan pushed a commit to snmvaughan/spark that referenced this pull request Jun 20, 2023
### What changes were proposed in this pull request?

This is follow-up for apache#39591 based on the suggestions from comment apache@1de8350#r109023188.

### Why are the changes needed?

For backward compatibility.

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

No.

### How was this patch tested?

The existing CI should pass

Closes apache#40816 from itholic/42078-followup.

Authored-by: itholic <haejoon.lee@databricks.com>
Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>
(cherry picked from commit 0457219)
Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>
GladwinLee pushed a commit to lyft/spark that referenced this pull request Oct 10, 2023
### What changes were proposed in this pull request?

This is follow-up for apache#39591 based on the suggestions from comment apache@1de8350#r109023188.

### Why are the changes needed?

For backward compatibility.

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

No.

### How was this patch tested?

The existing CI should pass

Closes apache#40816 from itholic/42078-followup.

Authored-by: itholic <haejoon.lee@databricks.com>
Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>
(cherry picked from commit 0457219)
Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>
catalinii pushed a commit to lyft/spark that referenced this pull request Oct 10, 2023
### What changes were proposed in this pull request?

This is follow-up for apache#39591 based on the suggestions from comment apache@1de8350#r109023188.

### Why are the changes needed?

For backward compatibility.

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

No.

### How was this patch tested?

The existing CI should pass

Closes apache#40816 from itholic/42078-followup.

Authored-by: itholic <haejoon.lee@databricks.com>
Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>
(cherry picked from commit 0457219)
Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants