gaborkaszab commented on code in PR #16125:
URL: https://github.com/apache/iceberg/pull/16125#discussion_r3266739543
##########
spark/v4.1/spark/src/test/java/org/apache/iceberg/spark/source/TestIcebergSourceTablesBase.java:
##########
@@ -1749,13 +1759,29 @@ public void testPartitionsTableDeleteStats() {
.set("position_delete_file_count", 0)
.set("equality_delete_record_count", 2L) // should be incremented
now
.set("equality_delete_file_count", 2) // should be incremented now
+ .set("dv_count", 0)
.set("last_updated_at",
table.snapshot(eqDeleteCommitId).timestampMillis() * 1000)
.set("last_updated_snapshot_id", eqDeleteCommitId)
.build());
for (int i = 0; i < 2; i += 1) {
TestHelpers.assertEqualsSafe(
partitionsTable.schema().asStruct(), expected.get(i), actual.get(i));
}
+
+ // verify dv_count is 0 when only V2 position delete files are present
(not DVs)
Review Comment:
Isn't this verified by adding `.set("dv_count", 0)` to the expected result?
##########
spark/v4.1/spark/src/test/java/org/apache/iceberg/spark/source/TestIcebergSourceTablesBase.java:
##########
@@ -1308,7 +1308,8 @@ public void testUnpartitionedPartitionsTable() {
10,
"last_updated_snapshot_id",
Types.LongType.get(),
- "Id of snapshot that last updated this partition"));
+ "Id of snapshot that last updated this partition"),
Review Comment:
I went through this test file and I don't see a single test where the new
dv_count field is not zero. Would you mind extending
`testPartitionsTableDeleteStats` with this?
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/source/TestIcebergSourceTablesBase.java:
##########
@@ -104,6 +106,9 @@
public abstract class TestIcebergSourceTablesBase extends TestBase {
+ private static final SimpleDateFormat TIMESTAMP_FORMAT =
Review Comment:
Isn't this irrelevant for the PR? Is this here because of some missing back
port to older Spark releases?
##########
spark/v4.0/spark/src/test/java/org/apache/iceberg/spark/source/TestIcebergSourceTablesBase.java:
##########
@@ -2342,7 +2376,7 @@ public void testSessionConfigSupport() {
withSQLConf(
// set read option through session configuration
- ImmutableMap.of("spark.datasource.iceberg.snapshot-id",
String.valueOf(s1)),
+ ImmutableMap.of("spark.datasource.iceberg.versionAsOf",
String.valueOf(s1)),
Review Comment:
I don't understand why these `versionAsOf` changes are needed. (other
occurrences above)
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
