You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "nastra (via GitHub)" <gi...@apache.org> on 2025/01/30 10:32:14 UTC
[PR] Spark: Test metadata tables with format-version=3 [iceberg]
nastra opened a new pull request, #12135:
URL: https://github.com/apache/iceberg/pull/12135
(no comment)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark: Test metadata tables with format-version=3 [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #12135:
URL: https://github.com/apache/iceberg/pull/12135#discussion_r1936775702
##########
spark/v3.5/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestMetadataTables.java:
##########
@@ -62,6 +67,44 @@
@ExtendWith(ParameterizedTestExtension.class)
public class TestMetadataTables extends ExtensionsTestBase {
+ @Parameter(index = 3)
+ private int formatVersion;
+
+ @Parameters(name = "catalogName = {0}, implementation = {1}, config = {2}, formatVersion = {3}")
+ protected static Object[][] parameters() {
Review 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark: Test metadata tables with format-version=3 [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #12135:
URL: https://github.com/apache/iceberg/pull/12135#discussion_r1935377367
##########
spark/v3.5/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestMetadataTables.java:
##########
@@ -62,6 +67,44 @@
@ExtendWith(ParameterizedTestExtension.class)
public class TestMetadataTables extends ExtensionsTestBase {
+ @Parameter(index = 3)
+ private int formatVersion;
+
+ @Parameters(name = "catalogName = {0}, implementation = {1}, config = {2}, formatVersion = {3}")
+ protected static Object[][] parameters() {
Review 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark: Test metadata tables with format-version=3 [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #12135:
URL: https://github.com/apache/iceberg/pull/12135#discussion_r1941281736
##########
spark/v3.5/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestMetadataTables.java:
##########
@@ -62,6 +67,44 @@
@ExtendWith(ParameterizedTestExtension.class)
public class TestMetadataTables extends ExtensionsTestBase {
+ @Parameter(index = 3)
+ private int formatVersion;
+
+ @Parameters(name = "catalogName = {0}, implementation = {1}, config = {2}, formatVersion = {3}")
+ protected static Object[][] parameters() {
Review Comment:
done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark: Test metadata tables with format-version=3 [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #12135:
URL: https://github.com/apache/iceberg/pull/12135#discussion_r1935947512
##########
spark/v3.5/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestMetadataTables.java:
##########
@@ -142,15 +185,43 @@ public void testUnpartitionedTable() throws Exception {
TestHelpers.nonDerivedSchema(actualFilesDs), expectedFiles.get(1), actualFiles.get(1));
}
+ @TestTemplate
+ public void testPositionDeletesTable() throws Exception {
+ sql(
+ "CREATE TABLE %s (id bigint, data string) USING iceberg TBLPROPERTIES"
+ + "('format-version'='%s', 'write.delete.mode'='merge-on-read')",
+ tableName, formatVersion);
+
+ List<SimpleRecord> records =
+ Lists.newArrayList(
+ new SimpleRecord(1, "a"),
+ new SimpleRecord(2, "b"),
+ new SimpleRecord(3, "c"),
+ new SimpleRecord(4, "d"));
+ spark
+ .createDataset(records, Encoders.bean(SimpleRecord.class))
+ .coalesce(1)
+ .writeTo(tableName)
+ .append();
+
+ sql("DELETE FROM %s WHERE id=1 OR id=3", tableName);
+
+ // check delete files table
+ assertThat(sql("SELECT * FROM %s.delete_files", tableName)).hasSize(1);
+
+ // check position_deletes table
+ assertThat(sql("SELECT * FROM %s.position_deletes", tableName)).hasSize(2);
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark: Test metadata tables with format-version=3 [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #12135:
URL: https://github.com/apache/iceberg/pull/12135#discussion_r1941282549
##########
spark/v3.5/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestMetadataTables.java:
##########
@@ -142,15 +185,43 @@ public void testUnpartitionedTable() throws Exception {
TestHelpers.nonDerivedSchema(actualFilesDs), expectedFiles.get(1), actualFiles.get(1));
}
+ @TestTemplate
+ public void testPositionDeletesTable() throws Exception {
+ sql(
+ "CREATE TABLE %s (id bigint, data string) USING iceberg TBLPROPERTIES"
+ + "('format-version'='%s', 'write.delete.mode'='merge-on-read')",
+ tableName, formatVersion);
+
+ List<SimpleRecord> records =
+ Lists.newArrayList(
+ new SimpleRecord(1, "a"),
+ new SimpleRecord(2, "b"),
+ new SimpleRecord(3, "c"),
+ new SimpleRecord(4, "d"));
+ spark
+ .createDataset(records, Encoders.bean(SimpleRecord.class))
+ .coalesce(1)
+ .writeTo(tableName)
+ .append();
+
+ sql("DELETE FROM %s WHERE id=1 OR id=3", tableName);
+
+ // check delete files table
+ assertThat(sql("SELECT * FROM %s.delete_files", tableName)).hasSize(1);
+
+ // check position_deletes table
+ assertThat(sql("SELECT * FROM %s.position_deletes", tableName)).hasSize(2);
Review Comment:
makes sense, I've updated the check
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark: Test metadata tables with format-version=3 [iceberg]
Posted by "dramaticlly (via GitHub)" <gi...@apache.org>.
dramaticlly commented on code in PR #12135:
URL: https://github.com/apache/iceberg/pull/12135#discussion_r1937711116
##########
spark/v3.5/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestMetadataTables.java:
##########
@@ -62,6 +67,44 @@
@ExtendWith(ParameterizedTestExtension.class)
public class TestMetadataTables extends ExtensionsTestBase {
+ @Parameter(index = 3)
+ private int formatVersion;
+
+ @Parameters(name = "catalogName = {0}, implementation = {1}, config = {2}, formatVersion = {3}")
+ protected static Object[][] parameters() {
Review 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.
```java
{
SparkCatalogConfig.HIVE.catalogName(),
SparkCatalogConfig.HIVE.implementation(),
SparkCatalogConfig.HIVE.properties(),
2
},
{
SparkCatalogConfig.HADOOP.catalogName(),
SparkCatalogConfig.HADOOP.implementation(),
SparkCatalogConfig.HADOOP.properties(),
2
},
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark: Test metadata tables with format-version=3 [iceberg]
Posted by "dramaticlly (via GitHub)" <gi...@apache.org>.
dramaticlly commented on code in PR #12135:
URL: https://github.com/apache/iceberg/pull/12135#discussion_r1936095912
##########
spark/v3.5/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestMetadataTables.java:
##########
@@ -62,6 +67,44 @@
@ExtendWith(ParameterizedTestExtension.class)
public class TestMetadataTables extends ExtensionsTestBase {
+ @Parameter(index = 3)
+ private int formatVersion;
+
+ @Parameters(name = "catalogName = {0}, implementation = {1}, config = {2}, formatVersion = {3}")
+ protected static Object[][] parameters() {
Review 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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark: Test metadata tables with format-version=3 [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #12135:
URL: https://github.com/apache/iceberg/pull/12135#discussion_r1935379994
##########
spark/v3.5/spark/src/main/java/org/apache/iceberg/spark/source/DVIterator.java:
##########
@@ -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);
Review Comment:
this fixes an issue when reading from the `.position_deletes` table when the underlying table is V3. By default, the schema includes https://github.com/apache/iceberg/blob/026a9b00459ae458eb4c6c620c78742dc43a00e8/core/src/main/java/org/apache/iceberg/PositionDeletesTable.java#L123 which we don't track for DVs, thus we're returning null here
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark: Test metadata tables with format-version=3 [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra merged PR #12135:
URL: https://github.com/apache/iceberg/pull/12135
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org