Spark: Test metadata tables with format-version=3 #12135
Conversation
| private int formatVersion; | ||
|
|
||
| @Parameters(name = "catalogName = {0}, implementation = {1}, config = {2}, formatVersion = {3}") | ||
| protected static Object[][] parameters() { |
There was a problem hiding this comment.
as defined in CatalogTestBase the default catalogs that were previously tested were Hadoop/Hive/Spark/REST. I'm limiting this here to Spark + REST, since we're adding testing for format version 2 + 3 here
There was a problem hiding this comment.
does this mean we drop the coverage for hadoop/hive catalog for format version 2 while add format version 3 for spark and rest catalog?
There was a problem hiding this comment.
yes, but the important piece here is rather to test V2 and V3. I don't think we want to test all possible combinations of catalogs and V2 + V3 as that would be a large test matrix
There was a problem hiding this comment.
I think we can just add following lines to keep the original v2 coverage and then selectively enable v3 for catalogs which are ready. Since many iceberg users are still in the process of adopting REST based catalog and v2 is our default spec at the moment, I think keep the current coverage in place to test against metadata table change would be essential.
{
SparkCatalogConfig.HIVE.catalogName(),
SparkCatalogConfig.HIVE.implementation(),
SparkCatalogConfig.HIVE.properties(),
2
},
{
SparkCatalogConfig.HADOOP.catalogName(),
SparkCatalogConfig.HADOOP.implementation(),
SparkCatalogConfig.HADOOP.properties(),
2
},
| @@ -84,6 +84,9 @@ public InternalRow next() { | |||
| rowValues.add(deleteFile.contentOffset()); | |||
| } else if (fieldId == MetadataColumns.CONTENT_SIZE_IN_BYTES_COLUMN_ID) { | |||
| rowValues.add(ScanTaskUtil.contentSizeInBytes(deleteFile)); | |||
| } else if (fieldId == MetadataColumns.DELETE_FILE_ROW_FIELD_ID) { | |||
| // DVs don't track the row that was deleted | |||
| rowValues.add(null); | |||
There was a problem hiding this comment.
this fixes an issue when reading from the .position_deletes table when the underlying table is V3. By default, the schema includes
| assertThat(sql("SELECT * FROM %s.delete_files", tableName)).hasSize(1); | ||
|
|
||
| // check position_deletes table | ||
| assertThat(sql("SELECT * FROM %s.position_deletes", tableName)).hasSize(2); |
There was a problem hiding this comment.
Should we assert the contents of the rows? I can see how it's a bit of a pain between v2/v3 but feels like a stronger assertion.
No description provided.