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

MaxGekk · 2020-12-09T12:18:57Z

What changes were proposed in this pull request?

Move the ALTER TABLE .. ADD PARTITION parsing tests to AlterTableAddPartitionParserSuite
Place v1 tests for ALTER TABLE .. ADD PARTITION from DDLSuite and v2 tests from AlterTablePartitionV2SQLSuite to the common trait AlterTableAddPartitionSuiteBase, so, the tests will run for V1, Hive V1 and V2 DS.

Why are the changes needed?

The unification will allow to run common ALTER TABLE .. ADD PARTITION tests for both DSv1 and Hive DSv1, DSv2
We can detect missing features and differences between DSv1 and DSv2 implementations.

Does this PR introduce any user-facing change?

No

How was this patch tested?

By running new test suites:

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

MaxGekk · 2020-12-09T12:20:14Z

.../src/test/scala/org/apache/spark/sql/execution/command/AlterTableAddPartitionSuiteBase.scala

+        "part6" -> "abc",
+        "part7" -> "true",
+        "part8" -> "2020-11-23",
+        "part9" -> s"2020-11-23${if (version == "V2") " " else "T"}22:13:10.123456")


Here is the behavior diff between V1 and V2 in showing partitions.

Actually, V1 doesn't care of correctness of partition values. We can create partitions with any garbage like 2020-11-23 --22:13:10.123456. V1 takes the string AS IS, and creates partition. No checks that partition value matches partition field type.

MaxGekk · 2020-12-09T12:23:02Z

...src/test/scala/org/apache/spark/sql/hive/execution/command/AlterTableAddPartitionSuite.scala

+      sql(s"CREATE TABLE $t (id bigint, data string) $defaultUsing PARTITIONED BY (id)")
+      sql(s"ALTER TABLE $t ADD PARTITION (id=2) LOCATION 'loc1'")
+
+      val errMsg = intercept[AnalysisException] {


Here is one more difference: Hive's exception is not handled, and propagated to the upper layer. Comparing to V1 (in-memory) and V2, where PartitionsAlreadyExistException is thrown.

We should try-catch the hive exception and throw PartitionsAlreadyExistException

Sure, I will open JIRA for that, and fix separately since this PR is about test changes.

Here is the fix #30711

MaxGekk · 2020-12-09T12:25:10Z

...e/src/test/scala/org/apache/spark/sql/execution/command/v2/AlterTableAddPartitionSuite.scala

+    .set(s"spark.sql.catalog.$catalog", classOf[InMemoryPartitionTableCatalog].getName)
+    .set(s"spark.sql.catalog.non_part_$catalog", classOf[InMemoryTableCatalog].getName)
+
+  override protected def checkLocation(


We will remove this after SPARK-33393

MaxGekk · 2020-12-09T12:25:40Z

@cloud-fan @HyukjinKwon Please, have a look at this PR.

SparkQA · 2020-12-09T13:35:39Z

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

SparkQA · 2020-12-09T14:07:54Z

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

SparkQA · 2020-12-09T15:55:22Z

Test build #132493 has finished for PR 30685 at commit 2ce716b.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class AlterTableAddPartitionSuite extends AlterTableAddPartitionSuiteBase with SharedSparkSession

cloud-fan · 2020-12-09T17:20:41Z

.../src/test/scala/org/apache/spark/sql/execution/command/AlterTableAddPartitionSuiteBase.scala

+  protected def withNsTable(ns: String, tableName: String)(f: String => Unit): Unit = {
+    withNamespace(ns) {
+      sql(s"CREATE NAMESPACE $ns")
+      val t = s"$ns.$tableName"


nit: the caller side can just pass in the namespace, and here we return $catalog.$ns.$tableName

nvm, I see https://github.com/apache/spark/pull/30685/files#diff-34e280062b2b58a677fe8fae16f67bfff596e80f59f61aeea6743dd9903eb7f0R81

an alternative is: def withNsTable(ns: String, tableName: String, cat: String = catalog)

...rc/test/scala/org/apache/spark/sql/execution/command/AlterTableAddPartitionParserSuite.scala

SparkQA · 2020-12-09T19:46:26Z

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

SparkQA · 2020-12-09T20:14:27Z

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

SparkQA · 2020-12-09T20:38:06Z

Test build #132509 has finished for PR 30685 at commit f2c4ecc.

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

SparkQA · 2020-12-10T02:09:27Z

Test build #132513 has finished for PR 30685 at commit 50c59aa.

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

cloud-fan · 2020-12-10T04:54:49Z

thanks, merging to master/3.1! (since it's test only)

…N tests ### What changes were proposed in this pull request? 1. Move the `ALTER TABLE .. ADD PARTITION` parsing tests to `AlterTableAddPartitionParserSuite` 2. Place v1 tests for `ALTER TABLE .. ADD PARTITION` from `DDLSuite` and v2 tests from `AlterTablePartitionV2SQLSuite` to the common trait `AlterTableAddPartitionSuiteBase`, so, the tests will run for V1, Hive V1 and V2 DS. ### Why are the changes needed? - The unification will allow to run common `ALTER TABLE .. ADD PARTITION` tests for both DSv1 and Hive DSv1, DSv2 - We can detect missing features and differences between DSv1 and DSv2 implementations. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? By running new test suites: ``` $ build/sbt -Phive-2.3 -Phive-thriftserver "test:testOnly *AlterTableAddPartitionSuite" ``` Closes #30685 from MaxGekk/unify-alter-table-add-partition-tests. Authored-by: Max Gekk <max.gekk@gmail.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com> (cherry picked from commit af37c7f) Signed-off-by: Wenchen Fan <wenchen@databricks.com>

cloud-fan · 2020-12-10T06:22:15Z

I've reverted it from 3.1, as the tests failed. It's probably because some v2 PRs go to master only.

MaxGekk added 15 commits December 8, 2020 22:40

First test

623e136

Add checkLocation()

97c6fa1

Improve checkLocation()

f7c06fe

Move checkLocation()

ee65251

Add the test "multiple partitions"

1c3542a

Add the test "partition already exists"

4501f63

Add withNsTable()

89268f7

Add case sensitivity test

e5011cd

Move "universal type conversions of partition values"

dbb370b

Move the SPARK-33650 test

7ec87fe

Move the SPARK-33676 test

f763def

Add "multi-part partition" test

e7af1bc

Unify V1 tests

c4d5028

Add AlterTableAddPartitionParserSuite

329fcc9

Move specific tests

2ce716b

MaxGekk commented Dec 9, 2020

View reviewed changes

MaxGekk mentioned this pull request Dec 9, 2020

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

Closed

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

cloud-fan reviewed Dec 9, 2020

View reviewed changes

MaxGekk commented Dec 9, 2020

View reviewed changes

...rc/test/scala/org/apache/spark/sql/execution/command/AlterTableAddPartitionParserSuite.scala Outdated Show resolved Hide resolved

MaxGekk added 2 commits December 9, 2020 21:44

Fix test names in AlterTableAddPartitionParserSuite

29a560f

Address Wenchen's review comment

f2c4ecc

Timestamp with gap

50c59aa

cloud-fan closed this in af37c7f Dec 10, 2020

MaxGekk deleted the unify-alter-table-add-partition-tests branch February 19, 2021 15:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

MaxGekk commented Dec 9, 2020 •

edited

Loading

MaxGekk Dec 9, 2020

MaxGekk Dec 9, 2020

MaxGekk Dec 9, 2020

cloud-fan Dec 9, 2020

MaxGekk Dec 9, 2020

MaxGekk Dec 10, 2020

MaxGekk Dec 9, 2020

MaxGekk commented Dec 9, 2020

SparkQA commented Dec 9, 2020

SparkQA commented Dec 9, 2020

SparkQA commented Dec 9, 2020

cloud-fan Dec 9, 2020 •

edited

Loading

cloud-fan Dec 9, 2020

cloud-fan Dec 9, 2020

SparkQA commented Dec 9, 2020

SparkQA commented Dec 9, 2020

SparkQA commented Dec 9, 2020

SparkQA commented Dec 10, 2020

cloud-fan commented Dec 10, 2020

cloud-fan commented Dec 10, 2020

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

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

Conversation

MaxGekk commented Dec 9, 2020 • edited Loading

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?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaxGekk commented Dec 9, 2020

SparkQA commented Dec 9, 2020

SparkQA commented Dec 9, 2020

SparkQA commented Dec 9, 2020

cloud-fan Dec 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Dec 9, 2020

SparkQA commented Dec 9, 2020

SparkQA commented Dec 9, 2020

SparkQA commented Dec 10, 2020

cloud-fan commented Dec 10, 2020

cloud-fan commented Dec 10, 2020

MaxGekk commented Dec 9, 2020 •

edited

Loading

cloud-fan Dec 9, 2020 •

edited

Loading