Skip to content

Spark: Consolidate duplicated test methods to TestHelpers#8024

Merged
szehon-ho merged 5 commits into
apache:masterfrom
hsiang-c:refact_test_helpers
Jul 13, 2023
Merged

Spark: Consolidate duplicated test methods to TestHelpers#8024
szehon-ho merged 5 commits into
apache:masterfrom
hsiang-c:refact_test_helpers

Conversation

@hsiang-c

@hsiang-c hsiang-c commented Jul 9, 2023

Copy link
Copy Markdown
Contributor

Note

@github-actions github-actions Bot added the spark label Jul 9, 2023
Comment thread spark/v3.4/spark/src/test/java/org/apache/iceberg/spark/data/TestHelpers.java Outdated
// Metadata Delete
Table table = Spark3Util.loadIcebergTable(spark, tableName);
Set<DataFile> dataFilesBefore = TestHelpers.dataFiles(table);
Set<DataFile> dataFilesBefore = TestHelpers.uniqueDataFiles(table);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Does the test pass with using dataFiles? (if we change to List) Looks like we are just comparing size, which should hopefully work?

Just wanted to avoid another test method if possible.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@szehon-ho Got it.

As long as you don't need uniqueness, I can pick List-based approach and test again.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@szehon-ho List also works (tested locally).

@szehon-ho szehon-ho left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good to me.

@szehon-ho szehon-ho changed the title Core: Consolidate duplicated test methods to TestHelpers Spark: Consolidate duplicated test methods to TestHelpers Jul 13, 2023
@szehon-ho szehon-ho merged commit 45d834a into apache:master Jul 13, 2023
@szehon-ho

Copy link
Copy Markdown
Member

Merged, thanks @hsiang-c !

@hsiang-c hsiang-c deleted the refact_test_helpers branch July 14, 2023 03:05
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