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. that is an
important invariant to assert on.
--
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]
