amogh-jahagirdar commented on code in PR #18225:
URL: https://github.com/apache/iceberg/pull/18225#discussion_r4093917642
##########
core/src/test/java/org/apache/iceberg/TestV4ManifestReader.java:
##########
@@ -345,6 +345,44 @@ public void statusFilter(FileFormat format) throws
IOException {
.containsExactlyElementsOf(files);
}
+ @ParameterizedTest
+ @FieldSource("MANIFEST_FORMATS")
+ public void inheritanceUncommittedOnlyInheritsSnapshotId(FileFormat format)
throws IOException {
+ Tracking trackingWithSnapshotId =
+ new TrackingStruct(EntryStatus.ADDED, 1234567L, null, null, null,
null, null, null);
+ TrackedFile withSnapshotId =
+ unpartitionedDataFile(trackingWithSnapshotId,
"s3://bucket/table/file-b.parquet");
+ Tracking trackingWithoutSnapshotId =
+ new TrackingStruct(EntryStatus.ADDED, null, null, null, null, null,
null, null);
+ TrackedFile withoutSnapshotId =
+ unpartitionedDataFile(trackingWithoutSnapshotId,
"s3://bucket/table/file-a.parquet");
+
+ ManifestFile manifest =
+ writeManifest(
+ format, UNPARTITIONED_TYPE, ImmutableList.of(withSnapshotId,
withoutSnapshotId));
+
+ assertThat(manifest.sequenceNumber())
+ .as("Manifest metadata has a sequence number that will not be
inherited")
+ .isNotNull();
Review Comment:
Do we want to assert the expected sequence number, beyond that it's not null?
##########
core/src/main/java/org/apache/iceberg/V4ManifestReader.java:
##########
@@ -130,7 +143,16 @@ public CloseableIterator<TrackedFile> iterator() {
private TrackedFile applyInheritance(TrackedFile file) {
// the reader uses TrackingStruct to read tracking so this cast is safe
TrackingStruct tracking = (TrackingStruct) file.tracking();
- tracking.inherit(manifest.snapshotId(), manifest.sequenceNumber());
+ if (isUncommitted) {
+ // uncommitted files cannot have a sequence number or assign first row ID
+ tracking.inherit(manifest.snapshotId());
Review Comment:
Minor: Could go either way on this, but do we really need the new `inherit`?
Since it's package private and we already have the comment , I feel like we
could just change the existing inherit to be (long, Long) and then pass in an
explicit null for the sequence number. Then it just becomes a one liner instead
of the if/else.
```
tracking.inherit(manifest.snapshotId(), isUncommitted ? null :
manifest.sequenceNumber());
```
##########
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:
>While we don't expect this to ever happen, do we want to remove what
happens if it does?
I guess I don't understand the "if it does" portion here? That would mean a
writer violated the spec rule of requiring first_row_id in the root, and it's
invalid metadata.
So we're basically testing that in the case that invalid metadata is
produced, then at least the behavior on read is consistent with prior format
versions? Is that an accurate understanding?
--
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]
