Skip to content

Conversation

@tomtongue
Copy link
Contributor

Change

The current test parameters in the TestRewritePositionDeleteFiles don't match the actual test values (the details are below). So this PR tries to fix these parameters to match the actual values, and also to parameterize the table format version for the method createTable.

Details

An example of the current test logs (each parameter name like formatVersion, catalogName etc. doesn’t match the corresponding value as below):

TestRewritePositionDeleteFiles > testDatePartition() > formatVersion = testhive, catalogName = org.apache.iceberg.spark.SparkCatalog, implementation = {type=hive, default-namespace=default, cache-enabled=false}, config = {3} STANDARD_ERROR // <= not match
    [Test worker] INFO org.apache.spark.sql.internal.SharedState - Setting hive.metastore.warehouse.dir ('null') to the value of spark.sql.warehouse.dir.
    [Test worker] INFO org.apache.spark.sql.internal.SharedState - Warehouse path is 'file:/path/to/iceberg/spark/v4.0/spark-extensions/spark-warehouse'.
    [Test worker] INFO org.sparkproject.jetty.server.handler.ContextHandler - Started o.s.j.s.ServletContextHandler@708378cc{/SQL,null,AVAILABLE,@Spark}
...

After applying this change (each parameter name matches the corresponding value):

TestRewritePositionDeleteFiles > testDatePartition() > catalogName = testhive, implementation = org.apache.iceberg.spark.SparkCatalog, config = {type=hive, default-namespace=default, cache-enabled=false}, formatVersion = 2 STANDARD_ERROR // <= match
    [Test worker] INFO org.apache.spark.sql.internal.SharedState - Setting hive.metastore.warehouse.dir ('null') to the value of spark.sql.warehouse.dir.

@github-actions github-actions bot added the spark label Oct 20, 2025
@tomtongue
Copy link
Contributor Author

I believe it's also possible to simply remove the formatVersion parameter if this test is only for V2.

@tomtongue tomtongue changed the title Spark 3.5, 4.0: Fix the format version parameter to match test parameters Spark 3.5, 4.0: Fix the test parameters to match the corresponding values Oct 20, 2025
private static final int DELETE_FILE_SIZE = 10;

@Parameters(name = "formatVersion = {0}, catalogName = {1}, implementation = {2}, config = {3}")
@Parameters(name = "catalogName = {0}, implementation = {1}, config = {2}, formatVersion = {3}")
Copy link
Contributor

@ebyhr ebyhr Oct 20, 2025

Choose a reason for hiding this comment

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

I believe it's also possible to simply remove the formatVersion parameter if this test is only for V2.

+1 to removing the formatVersion from this annotation, since I don't expect any new format versions to be added here given the deletion vectors. We can always parameterize it later if needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

we should actually keep the format version and upgrade the test to also run with V3. I have some changes locally that I created a while ago to do that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, thanks for checking this part.

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.

LGTM, thanks for fixing @tomtongue

@nastra nastra merged commit 8c44ccc into apache:main Oct 20, 2025
27 checks passed
@tomtongue tomtongue deleted the fix-test-params-rewrite-positional-delete-files branch October 20, 2025 06:51
@tomtongue
Copy link
Contributor Author

Thanks for the review and merge @nastra and thanks for checking this change @ebyhr!

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.

3 participants