-
Notifications
You must be signed in to change notification settings - Fork 29k
[SPARK-33767][SQL][TESTS] Unify v1 and v2 ALTER TABLE .. DROP PARTITION tests #30747
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-33767][SQL][TESTS] Unify v1 and v2 ALTER TABLE .. DROP PARTITION tests #30747
Conversation
| sql(s"CREATE TABLE $t (id bigint, data string) $defaultUsing PARTITIONED BY (id)") | ||
| sql(s"ALTER TABLE $t ADD PARTITION (id=1) LOCATION 'loc'") | ||
|
|
||
| val errMsg = intercept[AnalysisException] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
V1 and V2 in-memory catalogs throw NoSuchPartitionsException
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is the PR for that: #30778
|
Test build #132699 has finished for PR 30747 at commit
|
| super.test(s"ALTER TABLE .. ADD PARTITION $version: " + testName, testTags: _*)(testFun) | ||
| } | ||
|
|
||
| protected def withNsTable(ns: String, tableName: String, cat: String = catalog) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will refactor and move this to a common trait later. I don't want to touch tests that are not related to ALTER TABLE .. DROP PARTITION.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yea, like adding a DDLCommandTestUtils trait.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cloud-fan Here is the PR for that: #30779
|
@cloud-fan @HyukjinKwon Please, review this PR. |
|
|
||
| override def test(testName: String, testTags: Tag*)(testFun: => Any) | ||
| (implicit pos: Position): Unit = { | ||
| super.test(s"ALTER TABLE .. ADD PARTITION $version: " + testName, testTags: _*)(testFun) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DROP PARTITION
cloud-fan
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, please fix conflicts
…-table-drop-partition-tests # Conflicts: # sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/parser/DDLParserSuite.scala
|
Test build #132773 has finished for PR 30747 at commit
|
|
Kubernetes integration test starting |
|
Kubernetes integration test status success |
|
Kubernetes integration test starting |
|
Kubernetes integration test status success |
|
Test build #132775 has finished for PR 30747 at commit
|
|
thanks, merging to master! |
What changes were proposed in this pull request?
ALTER TABLE .. DROP PARTITIONparsing tests toAlterTableDropPartitionParserSuiteALTER TABLE .. DROP PARTITIONfromDDLSuiteand v2 tests fromAlterTablePartitionV2SQLSuiteto the common traitAlterTableDropPartitionSuiteBase, so, the tests will run for V1, Hive V1 and V2 DS.Why are the changes needed?
ALTER TABLE .. DROP PARTITIONtests for both DSv1 and Hive DSv1, DSv2Does this PR introduce any user-facing change?
No
How was this patch tested?
By running new test suites: