Skip to content

Migrate DDL sub-classes in spark-extensions to JUnit5 and AssertJ style#9624

Merged
nastra merged 5 commits intoapache:mainfrom
tomtongue:mig-junit5-subcls-extension
Feb 6, 2024
Merged

Migrate DDL sub-classes in spark-extensions to JUnit5 and AssertJ style#9624
nastra merged 5 commits intoapache:mainfrom
tomtongue:mig-junit5-subcls-extension

Conversation

@tomtongue
Copy link
Contributor

@tomtongue tomtongue commented Feb 2, 2024

Migrate sub-classes in spark-extension to JUnit 5 along with #9613, and #9086.

For now, only one test TestAlterTablePartitionFields is updated with JUnit 5. Will add all DDL sub-classes in this PR.

Current progress

  • TestAlterTablePartitionsFields
  • TestAlterTableSchema
  • TestBranchDDL
  • TestReplaceBranch
  • TestRequiredDistributionAndOrdering
  • TestSetWriteDistributionAndOrdering
  • TestTagDDL

TestViews will be separately created because of the code size (~ 1400 lines).

@github-actions github-actions bot added the spark label Feb 2, 2024
@tomtongue tomtongue changed the title Migrate SparkExtensions sub-classes to JUnit5 [WIP] Migrate SparkExtensions sub-classes to JUnit5 Feb 2, 2024
@nastra
Copy link
Contributor

nastra commented Feb 2, 2024

Will add all sub-classes in this PR

depending on the size of the diff it's also fine to split this into 2-3 PRs. You could probably start within a specific package and combine subclasses within one or more packages together in a PR (again depending on the size of the diff).

@tomtongue
Copy link
Contributor Author

Sure, thank you for the suggestion. The size of diff would be a bit big if I gather all the diffs in extension package in this PR. So, I will separately create PRs, but first add relevant classes (like DDLs) into this PR. If I misunderstand, please correct me.

@tomtongue tomtongue changed the title [WIP] Migrate SparkExtensions sub-classes to JUnit5 [WIP] Migrate SparkExtensions DDL sub-classes to JUnit5 Feb 5, 2024
@tomtongue
Copy link
Contributor Author

tomtongue commented Feb 6, 2024

@nastra Change the following 7 DDL extensions to JUnit 5 and AssertJ style.

  • TestAlterTablePartitionsFields
  • TestAlterTableSchema
  • TestBranchDDL
  • TestReplaceBranch
  • TestRequiredDistributionAndOrdering
  • TestSetWriteDistributionAndOrdering
  • TestTagDDL

For now Assertions.assertThatThrownBy is not changed, and not statically imported (assertThat is statically imported like other classes).

If I need to add more classes in this PR, please let me know.

@tomtongue tomtongue changed the title [WIP] Migrate SparkExtensions DDL sub-classes to JUnit5 Migrate DDL sub-classes in spark-extensions to JUnit5 and AssertJ style Feb 6, 2024
table.manageSnapshots().createBranch(branchName, first).commit();
SnapshotRef b1 = table.refs().get(branchName);
Integer minSnapshotsToKeep = b1.minSnapshotsToKeep();
Long maxSnapshotAgeMs = b1.maxSnapshotAgeMs();
Copy link
Contributor

@nastra nastra Feb 6, 2024

Choose a reason for hiding this comment

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

why are these being removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The sql in this test is like ALTER TABLE catalog.db.tbl REPLACE BRANCH branch AS OF VERSION snapshotId RETAIN maxRefAge timeUnit. For this query, the minSnapshotsToKeep and maxSnapshotAgeMs are always null in the current impl. If the query has WITH SNAPSHOT RETENTION the parameters are filled. So I remove this part and change the tests to null checks.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok thanks for confirming, no need to change anything as I think being explicit and making sure those are null is good in this case

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thank you!

import org.apache.spark.sql.internal.SQLConf;
import org.assertj.core.api.Assertions;
import org.junit.After;
import org.junit.Assert;
Copy link
Contributor

Choose a reason for hiding this comment

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

should be removed as well

Copy link
Contributor

@nastra nastra left a comment

Choose a reason for hiding this comment

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

this looks almost ready to go, just 2 comments. Also the diff size is about right for reviewing.

@nastra nastra merged commit 4835549 into apache:main Feb 6, 2024
@tomtongue
Copy link
Contributor Author

Thanks so much for the review. Will create a PR for other sub-classes.

@tomtongue tomtongue deleted the mig-junit5-subcls-extension branch February 6, 2024 11:01
@tomtongue tomtongue restored the mig-junit5-subcls-extension branch February 6, 2024 11:15
@tomtongue tomtongue deleted the mig-junit5-subcls-extension branch March 4, 2024 15:59
devangjhabakh pushed a commit to cdouglas/iceberg that referenced this pull request Apr 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants