Skip to content

[SPARK-35397][SQL] Replace sys.err usage with explicit exception type #32535

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

[SPARK-35397][SQL] Replace sys.err usage with explicit exception type #32535

wants to merge 2 commits into from

Conversation

@viirya
Copy link
Member

@viirya viirya commented May 13, 2021

What changes were proposed in this pull request?

This patch replaces sys.err usages with explicit exception types.

Why are the changes needed?

Motivated by the previous comment #32519 (comment), it sounds better to replace sys.err usages with explicit exception type.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Existing tests.

@github-actions github-actions bot added the SQL label May 13, 2021
dongjoon-hyun
dongjoon-hyun reviewed May 13, 2021
View reviewed changes
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala Outdated
case _ =>
sys.error("Unsupported natural join type " + joinType)
throw QueryExecutionErrors.unsupportedNaturalJoinTypeError(joinType)

Copy link
Member

@dongjoon-hyun dongjoon-hyun May 13, 2021

Choose a reason for hiding this comment

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

nit. extra space.

Copy link
Member Author

@viirya viirya May 13, 2021

Choose a reason for hiding this comment

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

Removed. Thanks.

dongjoon-hyun
dongjoon-hyun approved these changes May 13, 2021
View reviewed changes
Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

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

Looks fine and consistent to me.

@SparkQA
Copy link

SparkQA commented May 13, 2021

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/43023/

@SparkQA
Copy link

SparkQA commented May 13, 2021

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/43023/

@SparkQA
Copy link

SparkQA commented May 13, 2021

Test build #138503 has finished for PR 32535 at commit 122d853.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
  • new RuntimeException(s\"Failed to convert value $value (class of $cls) \" +

srowen
srowen approved these changes May 13, 2021
View reviewed changes
Copy link
Member

@srowen srowen left a comment

Choose a reason for hiding this comment

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

It will change the exception type, but, the exception generated now is uselessly broad anyway. I think it's fine.

@SparkQA
Copy link

SparkQA commented May 13, 2021

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/43038/

@SparkQA
Copy link

SparkQA commented May 13, 2021

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/43038/

@dongjoon-hyun
Copy link
Member

dongjoon-hyun commented May 13, 2021

Merged to master for Apache Spark 3.2.0.
Thank you, @viirya and all!

@viirya
Copy link
Member Author

viirya commented May 13, 2021

Thank you @dongjoon-hyun @maropu @srowen

@SparkQA
Copy link

SparkQA commented May 13, 2021

Test build #138517 has finished for PR 32535 at commit 98b2183.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@viirya viirya deleted the replace-sys-err branch December 27, 2023 18:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@maropu maropu maropu approved these changes

@srowen srowen srowen approved these changes

@dongjoon-hyun dongjoon-hyun dongjoon-hyun approved these changes

Assignees

No one assigned

Labels

SQL

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@viirya @SparkQA @dongjoon-hyun @maropu @srowen