-
Notifications
You must be signed in to change notification settings - Fork 28.9k
[SPARK-33858][SQL][TESTS] Unify v1 and v2 ALTER TABLE .. RENAME PARTITION tests #30863
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
Conversation
|
Kubernetes integration test starting |
|
Kubernetes integration test status success |
|
Test build #133101 has finished for PR 30863 at commit
|
...rc/test/scala/org/apache/spark/sql/execution/command/v1/AlterTableRenamePartitionSuite.scala
Show resolved
Hide resolved
|
@cloud-fan @HyukjinKwon Please, review this PR. |
|
Kubernetes integration test starting |
|
Kubernetes integration test status success |
|
Test build #133107 has finished for PR 30863 at commit
|
|
Kubernetes integration test starting |
|
Kubernetes integration test status success |
|
Test build #133114 has finished for PR 30863 at commit
|
|
@MaxGekk can you fix conflicts? |
…ion-tests # Conflicts: # sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/parser/DDLParserSuite.scala # sql/core/src/test/scala/org/apache/spark/sql/connector/AlterTablePartitionV2SQLSuite.scala # sql/core/src/test/scala/org/apache/spark/sql/execution/command/DDLSuite.scala # sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveDDLSuite.scala
| createSinglePartTable(t) | ||
| sql(s"INSERT INTO $t PARTITION (id = 2) SELECT 'def'") | ||
| checkPartitions(t, Map("id" -> "1"), Map("id" -> "2")) | ||
| // TODO(SPARK-33862): Throw PartitionAlreadyExistsException |
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.
This PR #30866 solves the issue.
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.
@dongjoon-hyun merged #30866, I am going to rebase this PR on the master and try to move this test to the common v1 trait.
|
Test build #133146 has finished for PR 30863 at commit
|
|
jenkins, retest this, please |
|
Kubernetes integration test starting |
|
Kubernetes integration test status success |
…ion-tests # Conflicts: # sql/core/src/test/scala/org/apache/spark/sql/execution/command/DDLSuite.scala
|
Kubernetes integration test starting |
|
Kubernetes integration test status success |
|
Kubernetes integration test starting |
|
Kubernetes integration test status failure |
|
Test build #133148 has finished for PR 30863 at commit
|
|
Test build #133152 has finished for PR 30863 at commit
|
|
Test build #133220 has finished for PR 30863 at commit
|
|
Kubernetes integration test starting |
|
Kubernetes integration test status success |
|
Test build #133232 has finished for PR 30863 at commit
|
| import org.apache.spark.sql.execution.command | ||
| import org.apache.spark.sql.internal.SQLConf | ||
|
|
||
| trait AlterTableRenamePartitionSuiteBase extends command.AlterTableRenamePartitionSuiteBase { |
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.
It would be helpful if you add a comment before this PR about the new hierarchy of the test suites, @MaxGekk .
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.
this is a good idea. Shall we do it for all the new test suites?
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.
Let me do that separately for all unified test suites, and to don't block this PR.
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.
Let me do that separately for all unified test suites, and to don't block this PR.
Since this is merged, please proceed the comment PR, @MaxGekk .
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 #30929 , please, review it.
|
thanks, merging to master! |
What changes were proposed in this pull request?
ALTER TABLE .. RENAME PARTITIONparsing tests toAlterTableRenamePartitionParserSuiteALTER TABLE .. RENAME PARTITIONfromDDLSuitetov1.AlterTableRenamePartitionSuiteand v2 tests fromAlterTablePartitionV2SQLSuitetov2.AlterTableRenamePartitionSuite, so, the tests will run for V1, Hive V1 and V2 DS.Why are the changes needed?
ALTER TABLE .. RENAME 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: