[SPARK-33742][SQL] Throw PartitionsAlreadyExistException from HiveExternalCatalog.createPartitions() #30711

MaxGekk · 2020-12-10T17:55:40Z

What changes were proposed in this pull request?

Throw PartitionsAlreadyExistException from createPartitions() in Hive external catalog when a partition exists. Currently, HiveExternalCatalog.createPartitions() throws AlreadyExistsException wrapped by AnalysisException.

In the PR, I propose to catch AlreadyExistsException in HiveClientImpl and replace it by PartitionsAlreadyExistException.

Why are the changes needed?

The behaviour of Hive external catalog deviates from V1/V2 in-memory catalogs that throw PartitionsAlreadyExistException. To improve user experience with Spark SQL, it would be better to throw the same exception.

Does this PR introduce any user-facing change?

Yes

How was this patch tested?

By running existing test suites:

$ build/sbt -Phive-2.3 -Phive-thriftserver "test:testOnly *AlterTableAddPartitionSuite"

MaxGekk · 2020-12-10T17:59:14Z

sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveExternalCatalog.scala

+    var cause = e
+    var found = false
+    while (!found && depth < maxDepth && cause != null) {
+      found = cause.getClass.getCanonicalName == classOf[AlreadyExistsException].getCanonicalName


I had to compare exceptions by their names because pattern matching and isInstanceOf don't work as we could expect. See the comment:

spark/sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveExternalCatalog.scala

Lines 83 to 84 in 781e19c

* Due to classloader isolation issues, pattern matching won't work here so we need

* to compare the canonical names of the exceptions, which we assume to be stable.

cloud-fan · 2020-12-10T18:34:48Z

sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveExternalCatalog.scala

  }

+  private def isAlreadyExistsException(e: Throwable): Boolean = {
+    val maxDepth = 4


did you test with different hive versions? one idea is to re-throw the exception in HiveClientImpl, and we can test it in VersionsSuite.

done. Added a test to VersionsSuite.

SparkQA · 2020-12-10T18:49:52Z

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

SparkQA · 2020-12-10T19:20:40Z

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

SparkQA · 2020-12-10T20:21:25Z

Test build #132583 has finished for PR 30711 at commit e7685a0.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-12-10T21:37:10Z

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

SparkQA · 2020-12-10T22:09:31Z

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

SparkQA · 2020-12-11T00:23:47Z

Test build #132590 has finished for PR 30711 at commit f834246.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

dongjoon-hyun · 2020-12-11T01:44:34Z

Retest this please

dongjoon-hyun

+1, LGTM.
Merged to master for Apache Spark 3.2.0.
Could you make backporting PRs for the older release branches to pass the tests?

SparkQA · 2020-12-11T04:04:26Z

Test build #132606 has finished for PR 30711 at commit f834246.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

…ernalCatalog.createPartitions() Throw `PartitionsAlreadyExistException` from `createPartitions()` in Hive external catalog when a partition exists. Currently, `HiveExternalCatalog.createPartitions()` throws `AlreadyExistsException` wrapped by `AnalysisException`. In the PR, I propose to catch `AlreadyExistsException` in `HiveClientImpl` and replace it by `PartitionsAlreadyExistException`. The behaviour of Hive external catalog deviates from V1/V2 in-memory catalogs that throw `PartitionsAlreadyExistException`. To improve user experience with Spark SQL, it would be better to throw the same exception. Yes By running existing test suites: ``` $ build/sbt -Phive-2.3 -Phive-thriftserver "test:testOnly *AlterTableAddPartitionSuite" ``` Closes apache#30711 from MaxGekk/hive-partition-exception. Authored-by: Max Gekk <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]> (cherry picked from commit fab2995) Signed-off-by: Max Gekk <[email protected]>

…ernalCatalog.createPartitions() Throw `PartitionsAlreadyExistException` from `createPartitions()` in Hive external catalog when a partition exists. Currently, `HiveExternalCatalog.createPartitions()` throws `AlreadyExistsException` wrapped by `AnalysisException`. In the PR, I propose to catch `AlreadyExistsException` in `HiveClientImpl` and replace it by `PartitionsAlreadyExistException`. The behaviour of Hive external catalog deviates from V1/V2 in-memory catalogs that throw `PartitionsAlreadyExistException`. To improve user experience with Spark SQL, it would be better to throw the same exception. Yes By running existing test suites: ``` $ build/sbt -Phive-2.3 -Phive-thriftserver "test:testOnly *AlterTableAddPartitionSuite" ``` Closes apache#30711 from MaxGekk/hive-partition-exception. Authored-by: Max Gekk <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]> (cherry picked from commit fab2995) Signed-off-by: Max Gekk <[email protected]> (cherry picked from commit b284ea3) Signed-off-by: Max Gekk <[email protected]>

…ernalCatalog.createPartitions() Throw `PartitionsAlreadyExistException` from `createPartitions()` in Hive external catalog when a partition exists. Currently, `HiveExternalCatalog.createPartitions()` throws `AlreadyExistsException` wrapped by `AnalysisException`. In the PR, I propose to catch `AlreadyExistsException` in `HiveClientImpl` and replace it by `PartitionsAlreadyExistException`. The behaviour of Hive external catalog deviates from V1/V2 in-memory catalogs that throw `PartitionsAlreadyExistException`. To improve user experience with Spark SQL, it would be better to throw the same exception. Yes By running existing test suites: ``` $ build/sbt -Phive-2.3 -Phive-thriftserver "test:testOnly *AlterTableAddPartitionSuite" ``` Closes apache#30711 from MaxGekk/hive-partition-exception. Authored-by: Max Gekk <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]> (cherry picked from commit fab2995) Signed-off-by: Max Gekk <[email protected]> (cherry picked from commit b284ea3) Signed-off-by: Max Gekk <[email protected]> (cherry picked from commit 0226cfd) Signed-off-by: Max Gekk <[email protected]>

MaxGekk · 2020-12-11T11:49:55Z

@dongjoon-hyun Here are backports to:

… in `HiveClientImpl` ### What changes were proposed in this pull request? Update the SQL migration guide about the changes made by: - #30778 - #30711 - #30866 ### Why are the changes needed? To inform users about the recent changes in the upcoming releases. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? N/A Closes #30925 from MaxGekk/sql-migr-guide-hiveclientimpl. Authored-by: Max Gekk <[email protected]> Signed-off-by: HyukjinKwon <[email protected]>

…anges in `HiveClientImpl` ### What changes were proposed in this pull request? Update the SQL migration guide about the changes made by: - #30778 - #30711 ### Why are the changes needed? To inform users about the recent changes in the upcoming releases. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? N/A Closes #30931 from MaxGekk/sql-migr-guide-hiveclientimpl-3.1. Authored-by: Max Gekk <[email protected]> Signed-off-by: HyukjinKwon <[email protected]>

…anges in `HiveClientImpl` ### What changes were proposed in this pull request? Update the SQL migration guide about the changes made by: - #30778 - #30711 ### Why are the changes needed? To inform users about the recent changes in the upcoming releases. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? N/A Closes #30932 from MaxGekk/sql-migr-guide-hiveclientimpl-3.0. Authored-by: Max Gekk <[email protected]> Signed-off-by: HyukjinKwon <[email protected]>

…anges in `HiveClientImpl` ### What changes were proposed in this pull request? Update the SQL migration guide about the changes made by: - #30778 - #30711 ### Why are the changes needed? To inform users about the recent changes in the upcoming releases. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? N/A Closes #30933 from MaxGekk/sql-migr-guide-hiveclientimpl-2.4. Authored-by: Max Gekk <[email protected]> Signed-off-by: HyukjinKwon <[email protected]>

MaxGekk added 2 commits December 10, 2020 17:55

Move the test to the common trait

e100834

Throw PartitionsAlreadyExistException

e7685a0

github-actions bot added the SQL label Dec 10, 2020

MaxGekk commented Dec 10, 2020

View reviewed changes

MaxGekk mentioned this pull request Dec 10, 2020

[SPARK-33558][SQL][TESTS] Unify v1 and v2 ALTER TABLE .. ADD PARTITION tests #30685

Closed

cloud-fan reviewed Dec 10, 2020

View reviewed changes

Move wrapping to HiveClientImpl

f834246

dongjoon-hyun approved these changes Dec 11, 2020

View reviewed changes

dongjoon-hyun closed this in fab2995 Dec 11, 2020

MaxGekk deleted the hive-partition-exception branch February 19, 2021 15:04

	* Due to classloader isolation issues, pattern matching won't work here so we need
	* to compare the canonical names of the exceptions, which we assume to be stable.

[SPARK-33742][SQL] Throw PartitionsAlreadyExistException from HiveExternalCatalog.createPartitions() #30711

[SPARK-33742][SQL] Throw PartitionsAlreadyExistException from HiveExternalCatalog.createPartitions() #30711

Uh oh!

Conversation

MaxGekk commented Dec 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

MaxGekk Dec 10, 2020

Choose a reason for hiding this comment

Uh oh!

cloud-fan Dec 10, 2020

Choose a reason for hiding this comment

Uh oh!

MaxGekk Dec 10, 2020

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Dec 10, 2020

Uh oh!

SparkQA commented Dec 10, 2020

Uh oh!

SparkQA commented Dec 10, 2020

Uh oh!

SparkQA commented Dec 10, 2020

Uh oh!

SparkQA commented Dec 10, 2020

Uh oh!

SparkQA commented Dec 11, 2020

Uh oh!

dongjoon-hyun commented Dec 11, 2020

Uh oh!

dongjoon-hyun left a comment

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Dec 11, 2020

Uh oh!

MaxGekk commented Dec 11, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

MaxGekk commented Dec 10, 2020 •

edited

Loading