stevenzwu commented on code in PR #18225:
URL: https://github.com/apache/iceberg/pull/18225#discussion_r4086949043
##########
core/src/test/java/org/apache/iceberg/TestV4ManifestReader.java:
##########
@@ -447,6 +485,86 @@ public void inheritanceAddedFileSequenceNumber(FileFormat
format) throws IOExcep
.containsExactly(MANIFEST_SEQ, 500L);
}
+ @ParameterizedTest
+ @FieldSource("MANIFEST_FORMATS")
+ public void inheritanceFirstRowId(FileFormat format) throws IOException {
+ Tracking trackingWithoutFirstRowId =
+ new TrackingStruct(EntryStatus.ADDED, SNAPSHOT_ID, null, null, null,
null, null, null);
+ TrackedFile withoutFirstRowId =
+ unpartitionedDataFile(trackingWithoutFirstRowId,
"s3://bucket/table/file-a.parquet");
+ Tracking trackingWithFirstRowId =
+ new TrackingStruct(EntryStatus.EXISTING, SNAPSHOT_ID, null, null,
null, 5_000L, null, null);
+ TrackedFile withFirstRowId =
+ unpartitionedDataFile(trackingWithFirstRowId,
"s3://bucket/table/file-c.parquet");
+
+ ManifestFile manifest =
+ writeManifest(
+ format, UNPARTITIONED_TYPE, ImmutableList.of(withoutFirstRowId,
withFirstRowId));
+ when(manifest.firstRowId()).thenReturn(10_000L);
+
+ V4ManifestReader.Builder builder =
+ V4ManifestReader.builder(manifest, IO, TABLE_SCHEMA,
ID_PARTITIONING_SPECS)
+ .metricsConfig(METRICS_CONFIG);
+ List<TrackedFile> actual = read(builder);
+
+ assertThat(actual)
+ .extracting(file -> file.tracking().firstRowId())
+ .containsExactly(10_000L, 5_000L);
Review Comment:
This test never checks that `nextRowId` advances after an assignment. File A
is unassigned (becomes `10_000`); file B already has `5_000`, so it does not
take the next ID. A bug that assigned every unassigned file `10_000` would
still pass.
A concrete plan, with `RECORD_COUNT = 100` and manifest `first_row_id =
10_000`:
- A: unassigned → inherit `10_000`
- B: unassigned → inherit `10_100`
- C: already assigned → no inheritance (does not bump)
- D: unassigned → inherit `10_200` (C is not in the count bump)
Also add a partition filter that drops A: B must still be `10_100` and D
`10_200`, because assignment runs on every file before filters.
##########
core/src/test/java/org/apache/iceberg/TestV4ManifestReader.java:
##########
@@ -447,6 +485,86 @@ public void inheritanceAddedFileSequenceNumber(FileFormat
format) throws IOExcep
.containsExactly(MANIFEST_SEQ, 500L);
}
+ @ParameterizedTest
+ @FieldSource("MANIFEST_FORMATS")
+ public void inheritanceFirstRowId(FileFormat format) throws IOException {
+ Tracking trackingWithoutFirstRowId =
+ new TrackingStruct(EntryStatus.ADDED, SNAPSHOT_ID, null, null, null,
null, null, null);
+ TrackedFile withoutFirstRowId =
+ unpartitionedDataFile(trackingWithoutFirstRowId,
"s3://bucket/table/file-a.parquet");
+ Tracking trackingWithFirstRowId =
+ new TrackingStruct(EntryStatus.EXISTING, SNAPSHOT_ID, null, null,
null, 5_000L, null, null);
+ TrackedFile withFirstRowId =
+ unpartitionedDataFile(trackingWithFirstRowId,
"s3://bucket/table/file-c.parquet");
+
+ ManifestFile manifest =
+ writeManifest(
+ format, UNPARTITIONED_TYPE, ImmutableList.of(withoutFirstRowId,
withFirstRowId));
+ when(manifest.firstRowId()).thenReturn(10_000L);
+
+ V4ManifestReader.Builder builder =
+ V4ManifestReader.builder(manifest, IO, TABLE_SCHEMA,
ID_PARTITIONING_SPECS)
+ .metricsConfig(METRICS_CONFIG);
+ List<TrackedFile> actual = read(builder);
+
+ assertThat(actual)
+ .extracting(file -> file.tracking().firstRowId())
+ .containsExactly(10_000L, 5_000L);
+ }
+
+ @ParameterizedTest
+ @FieldSource("MANIFEST_FORMATS")
+ public void inheritanceUncommittedSkipsFirstRowId(FileFormat format) throws
IOException {
+ Tracking trackingWithoutFirstRowId =
+ new TrackingStruct(EntryStatus.ADDED, SNAPSHOT_ID, null, null, null,
null, null, null);
+ TrackedFile withoutFirstRowId =
+ unpartitionedDataFile(trackingWithoutFirstRowId,
"s3://bucket/table/file-a.parquet");
+ Tracking trackingWithFirstRowId =
+ new TrackingStruct(EntryStatus.EXISTING, SNAPSHOT_ID, null, null,
null, 5_000L, null, null);
+ TrackedFile withFirstRowId =
+ unpartitionedDataFile(trackingWithFirstRowId,
"s3://bucket/table/file-c.parquet");
+
+ ManifestFile manifest =
+ writeManifest(
+ format, UNPARTITIONED_TYPE, ImmutableList.of(withoutFirstRowId,
withFirstRowId));
+ when(manifest.firstRowId()).thenReturn(10_000L);
+
+ V4ManifestReader.Builder builder =
+ V4ManifestReader.uncommitted(manifest, IO, TABLE_SCHEMA,
ID_PARTITIONING_SPECS)
+ .metricsConfig(METRICS_CONFIG);
+ List<TrackedFile> actual = read(builder);
+
+ assertThat(actual)
+ .extracting(file -> file.tracking().firstRowId())
+ .containsExactly(null, 5_000L);
+ }
+
+ @ParameterizedTest
+ @FieldSource("MANIFEST_FORMATS")
+ public void inheritanceFirstRowIdNullFromUpgradedTable(FileFormat format)
throws IOException {
+ Tracking trackingWithoutFirstRowId =
+ new TrackingStruct(EntryStatus.ADDED, SNAPSHOT_ID, null, null, null,
null, null, null);
+ TrackedFile withoutFirstRowId =
+ unpartitionedDataFile(trackingWithoutFirstRowId,
"s3://bucket/table/file-a.parquet");
+ Tracking trackingWithFirstRowId =
+ new TrackingStruct(EntryStatus.EXISTING, SNAPSHOT_ID, null, null,
null, 5_000L, null, null);
+ TrackedFile withFirstRowId =
+ unpartitionedDataFile(trackingWithFirstRowId,
"s3://bucket/table/file-c.parquet");
+
+ ManifestFile manifest =
+ writeManifest(
+ format, UNPARTITIONED_TYPE, ImmutableList.of(withoutFirstRowId,
withFirstRowId));
+ // first row ID is null in upgraded tables, which is propagated to all
files
Review Comment:
if we require all entries in the v4 root manifest file have materialized
first_row_id, this case will not happen, right?
During upgrade to v4, the manifest list file is rewritten to a root manifest
file, which would materialize the first_row_id for all entries.
--
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]
