amogh-jahagirdar commented on code in PR #18171:
URL: https://github.com/apache/iceberg/pull/18171#discussion_r4073998995
##########
core/src/main/java/org/apache/iceberg/TrackingStruct.java:
##########
@@ -116,34 +116,20 @@ private TrackingStruct(TrackingStruct toCopy) {
this.replacedPositions = replacedPositions;
}
- void inheritFrom(Tracking manifestTracking) {
- if (manifestTracking != null) {
- if (snapshotId == null) {
- this.snapshotId = manifestTracking.snapshotId();
- }
-
- // manifests do not distinguish between data and file sequence numbers
- Preconditions.checkArgument(
- Objects.equals(
- manifestTracking.dataSequenceNumber(),
manifestTracking.fileSequenceNumber()),
- "Manifest data and file sequence numbers must be equal, got %s and
%s",
- manifestTracking.dataSequenceNumber(),
- manifestTracking.fileSequenceNumber());
+ void inherit(long manifestSnapshotId, long manifestSeqNumber) {
+ if (null == snapshotId) {
+ this.snapshotId = manifestSnapshotId;
+ }
- if (status == EntryStatus.ADDED) {
- if (dataSequenceNumber == null) {
- this.dataSequenceNumber = manifestTracking.fileSequenceNumber();
- }
+ boolean isAdded = status == EntryStatus.ADDED;
Review Comment:
I know this is more forward looking than we need right now, but I'm curious
how do we envision handling the case when the status is MODIFIED and there's a
column update so we need to inherit there as well?
To identify if there was a column change we would pass in the
latest_column_update_snapshot_id, and if it's the same as the current snapshot
ID (which I think we'd also need to pass in because that can be different than
`manifestSnapshotId`), then we know that this was a column update and the data
seq/file seq inheritance should be applied.
##########
core/src/test/java/org/apache/iceberg/TestV4ManifestReader.java:
##########
@@ -344,22 +345,132 @@ public void statusFilter(FileFormat format) throws
IOException {
.containsExactlyElementsOf(files);
}
+ // inheritance test plan:
+ // - snapshot ID inheritance, not dv snapshot ID inheritance
+ // - file seq and data seq inheritance from null WITH ADDED
+ // - file seq and data seq not inherited with EXISTING, MODIFIED, DELETE
(SHOULD FAIL?)
+
+ @ParameterizedTest
+ @FieldSource("MANIFEST_FORMATS")
+ public void inheritanceSnapshotId(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));
+
+ 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().snapshotId())
+ .containsExactly(1234567L, SNAPSHOT_ID);
+ }
+
+ @ParameterizedTest
+ @FieldSource("MANIFEST_FORMATS")
+ public void inheritanceDVSnapshotIdNotInherited(FileFormat format) throws
IOException {
+ Tracking trackingWithDVSnapshotId =
+ new TrackingStruct(EntryStatus.ADDED, SNAPSHOT_ID, null, null,
1234567L, null, null, null);
+ TrackedFile withDVSnapshotId =
+ unpartitionedDataFile(trackingWithDVSnapshotId,
"s3://bucket/table/file-b.parquet");
+ Tracking trackingWithoutDVSnapshotId =
+ new TrackingStruct(EntryStatus.ADDED, SNAPSHOT_ID, null, null, null,
null, null, null);
+ TrackedFile withoutDVSnapshotId =
+ unpartitionedDataFile(trackingWithoutDVSnapshotId,
"s3://bucket/table/file-a.parquet");
+
+ ManifestFile manifest =
+ writeManifest(
+ format, UNPARTITIONED_TYPE, ImmutableList.of(withDVSnapshotId,
withoutDVSnapshotId));
+
+ 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().dvSnapshotId())
+ .containsExactly(1234567L, null);
+ }
+
+ @ParameterizedTest
+ @FieldSource("MANIFEST_FORMATS")
+ public void inheritanceAddedDataSequenceNumber(FileFormat format) throws
IOException {
+ Tracking trackingWithoutSeq =
+ new TrackingStruct(EntryStatus.ADDED, SNAPSHOT_ID, null, null, null,
null, null, null);
+ TrackedFile withoutSeq =
+ unpartitionedDataFile(trackingWithoutSeq,
"s3://bucket/table/file-a.parquet");
+ Tracking trackingWithDataSeq =
+ new TrackingStruct(EntryStatus.ADDED, SNAPSHOT_ID, 500L, null, null,
null, null, null);
+ TrackedFile withDataSeq =
+ unpartitionedDataFile(trackingWithDataSeq,
"s3://bucket/table/file-b.parquet");
+
+ ManifestFile manifest =
+ writeManifest(
+ format, UNPARTITIONED_TYPE, ImmutableList.of(withoutSeq,
withDataSeq));
+
+ 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().dataSequenceNumber())
+ .containsExactly(MANIFEST_SEQ, 500L);
+ }
+
+ @ParameterizedTest
+ @FieldSource("MANIFEST_FORMATS")
+ public void inheritanceAddedFileSequenceNumber(FileFormat format) throws
IOException {
+ Tracking trackingWithoutSeq =
+ new TrackingStruct(EntryStatus.ADDED, SNAPSHOT_ID, null, null, null,
null, null, null);
+ TrackedFile withoutSeq =
+ unpartitionedDataFile(trackingWithoutSeq,
"s3://bucket/table/file-a.parquet");
+ Tracking trackingWithFileSeq =
+ new TrackingStruct(EntryStatus.ADDED, SNAPSHOT_ID, null, 500L, null,
null, null, null);
+ TrackedFile withFileSeq =
+ unpartitionedDataFile(trackingWithFileSeq,
"s3://bucket/table/file-c.parquet");
+
+ ManifestFile manifest =
+ writeManifest(
+ format, UNPARTITIONED_TYPE, ImmutableList.of(withoutSeq,
withFileSeq));
+
+ 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().fileSequenceNumber())
+ .containsExactly(MANIFEST_SEQ, 500L);
+ }
+
@ParameterizedTest
@FieldSource("MANIFEST_FORMATS")
public void inheritanceManifestLocationAndPosition(FileFormat format) throws
IOException {
List<TrackedFile> files =
ImmutableList.of(FILE_A, FILE_B, DATA_MANIFEST_REF,
DELETE_MANIFEST_REF);
ManifestFile manifest = writeManifest(format, UNPARTITIONED_TYPE, files);
+ String location = manifest.path();
V4ManifestReader.Builder builder =
V4ManifestReader.builder(manifest, IO, TABLE_SCHEMA,
ID_PARTITIONING_SPECS)
.metricsConfig(METRICS_CONFIG);
List<TrackedFile> read = read(builder);
assertThat(read)
Review Comment:
minor:
```
assertThat(read)
.hasSize(4)
.extracting(file -> file.tracking().manifestLocation())
.containsOnly(location);
```
##########
core/src/main/java/org/apache/iceberg/avro/Avro.java:
##########
@@ -716,6 +717,11 @@ public <D> AvroIterable<D> build() {
reader = (DatumReader<D>) defaultCreateReaderFunc.apply(schema);
}
+ if (reader instanceof InternalReader) {
Review Comment:
Should we add a test to `TestInternalData` so that we can verify the _file
column works as expected when doing an InternalData.read() call (that test
class is generic to covering the parquet case as well).
##########
core/src/main/java/org/apache/iceberg/TrackingStruct.java:
##########
@@ -116,34 +116,20 @@ private TrackingStruct(TrackingStruct toCopy) {
this.replacedPositions = replacedPositions;
}
- void inheritFrom(Tracking manifestTracking) {
- if (manifestTracking != null) {
- if (snapshotId == null) {
- this.snapshotId = manifestTracking.snapshotId();
- }
-
- // manifests do not distinguish between data and file sequence numbers
Review Comment:
Curious, why did we drop this check? I think the comment could be dropped
but I do think we have the requirement that writers set the same data sequence
and file sequence number in V4 for leaf manifests.
##########
core/src/main/java/org/apache/iceberg/avro/Avro.java:
##########
@@ -716,6 +717,11 @@ public <D> AvroIterable<D> build() {
reader = (DatumReader<D>) defaultCreateReaderFunc.apply(schema);
}
+ if (reader instanceof InternalReader) {
Review Comment:
The V4ManifestReader effectively covers this fwiw but this kind of test is
testing a different abstraction.
##########
core/src/main/java/org/apache/iceberg/TrackingStruct.java:
##########
@@ -116,34 +116,20 @@ private TrackingStruct(TrackingStruct toCopy) {
this.replacedPositions = replacedPositions;
}
- void inheritFrom(Tracking manifestTracking) {
- if (manifestTracking != null) {
- if (snapshotId == null) {
- this.snapshotId = manifestTracking.snapshotId();
- }
-
- // manifests do not distinguish between data and file sequence numbers
- Preconditions.checkArgument(
- Objects.equals(
- manifestTracking.dataSequenceNumber(),
manifestTracking.fileSequenceNumber()),
- "Manifest data and file sequence numbers must be equal, got %s and
%s",
- manifestTracking.dataSequenceNumber(),
- manifestTracking.fileSequenceNumber());
+ void inherit(long manifestSnapshotId, long manifestSeqNumber) {
+ if (null == snapshotId) {
+ this.snapshotId = manifestSnapshotId;
+ }
- if (status == EntryStatus.ADDED) {
- if (dataSequenceNumber == null) {
- this.dataSequenceNumber = manifestTracking.fileSequenceNumber();
- }
+ boolean isAdded = status == EntryStatus.ADDED;
- if (fileSequenceNumber == null) {
- this.fileSequenceNumber = manifestTracking.fileSequenceNumber();
- }
- }
+ if (null == dataSequenceNumber && (isAdded || manifestSeqNumber == 0)) {
Review Comment:
Why manifestSeqNumber == 0?
##########
core/src/main/java/org/apache/iceberg/TrackingStruct.java:
##########
@@ -116,34 +116,20 @@ private TrackingStruct(TrackingStruct toCopy) {
this.replacedPositions = replacedPositions;
}
- void inheritFrom(Tracking manifestTracking) {
- if (manifestTracking != null) {
- if (snapshotId == null) {
- this.snapshotId = manifestTracking.snapshotId();
- }
-
- // manifests do not distinguish between data and file sequence numbers
- Preconditions.checkArgument(
- Objects.equals(
- manifestTracking.dataSequenceNumber(),
manifestTracking.fileSequenceNumber()),
- "Manifest data and file sequence numbers must be equal, got %s and
%s",
- manifestTracking.dataSequenceNumber(),
- manifestTracking.fileSequenceNumber());
+ void inherit(long manifestSnapshotId, long manifestSeqNumber) {
+ if (null == snapshotId) {
+ this.snapshotId = manifestSnapshotId;
+ }
- if (status == EntryStatus.ADDED) {
- if (dataSequenceNumber == null) {
- this.dataSequenceNumber = manifestTracking.fileSequenceNumber();
- }
+ boolean isAdded = status == EntryStatus.ADDED;
- if (fileSequenceNumber == null) {
- this.fileSequenceNumber = manifestTracking.fileSequenceNumber();
- }
- }
+ if (null == dataSequenceNumber && (isAdded || manifestSeqNumber == 0)) {
Review Comment:
Oh, for v1 manifests nvm
##########
core/src/test/java/org/apache/iceberg/TestTrackingStruct.java:
##########
@@ -128,106 +132,64 @@ void copy() {
assertThat(copy.replacedPositions().array()).isNotSameAs(tracking.replacedPositions().array());
}
- @Test
- void inheritSnapshotId() {
- TrackingStruct tracking =
- new TrackingStruct(EntryStatus.ADDED, null, null, null, null, null,
null, null);
+ @ParameterizedTest
+ @EnumSource(EntryStatus.class)
+ void inheritSnapshotId(EntryStatus status) {
+ TrackingStruct tracking = new TrackingStruct(status, null, null, null,
null, null, null, null);
- tracking.inheritFrom(createManifestTracking(100L, 60L));
+ tracking.inherit(100L, 60L);
- // snapshotId is null, should inherit from manifest
assertThat(tracking.snapshotId()).isEqualTo(100L);
}
- @Test
- void inheritSequenceNumberForAddedEntries() {
- TrackingStruct tracking =
- new TrackingStruct(EntryStatus.ADDED, 42L, null, null, null, null,
null, null);
-
- tracking.inheritFrom(createManifestTracking(100L, 60L));
-
- // sequence numbers are null and status is ADDED, should inherit
- assertThat(tracking.dataSequenceNumber()).isEqualTo(60L);
- assertThat(tracking.fileSequenceNumber()).isEqualTo(60L);
- }
-
- @Test
- void doNotInheritSequenceNumberForExistingEntries() {
- TrackingStruct tracking =
- new TrackingStruct(EntryStatus.EXISTING, 42L, 5L, 6L, null, null,
null, null);
-
- tracking.inheritFrom(createManifestTracking(100L, 60L));
-
- // sequence numbers are not inherited for EXISTING entries
- assertThat(tracking.dataSequenceNumber()).isEqualTo(5L);
- assertThat(tracking.fileSequenceNumber()).isEqualTo(6L);
- }
-
- @Test
- void doNotInheritSequenceNumberForModifiedEntries() {
- TrackingStruct tracking =
- new TrackingStruct(EntryStatus.MODIFIED, 42L, 5L, 6L, null, null,
null, null);
-
- tracking.inheritFrom(createManifestTracking(100L, 60L));
-
- // sequence numbers are not inherited for MODIFIED entries
- assertThat(tracking.dataSequenceNumber()).isEqualTo(5L);
- assertThat(tracking.fileSequenceNumber()).isEqualTo(6L);
- }
-
- @Test
- void explicitValuesOverrideInheritance() {
- TrackingStruct tracking =
- new TrackingStruct(EntryStatus.ADDED, 200L, 75L, 76L, null, null,
null, null);
+ @ParameterizedTest
+ @EnumSource(EntryStatus.class)
+ void inheritancePreservesExplicitValues(EntryStatus status) {
+ TrackingStruct tracking = new TrackingStruct(status, 200L, 75L, 76L, null,
null, null, null);
- tracking.inheritFrom(createManifestTracking(100L, 60L));
+ tracking.inherit(100L, 60L);
- // explicit values should take precedence
assertThat(tracking.snapshotId()).isEqualTo(200L);
assertThat(tracking.dataSequenceNumber()).isEqualTo(75L);
assertThat(tracking.fileSequenceNumber()).isEqualTo(76L);
}
- @Test
- void inheritFromRejectsUnequalSequenceNumbers() {
- TrackingStruct tracking =
- new TrackingStruct(EntryStatus.ADDED, 42L, null, null, null, null,
null, null);
+ @ParameterizedTest
+ @EnumSource(EntryStatus.class)
+ void inheritanceAllStatusesInheritSeq0(EntryStatus status) {
+ // Sequence number 0 is inherited for all statuses for reading pre-seq v1
tables
+ TrackingStruct tracking = new TrackingStruct(status, 42L, null, null,
null, null, null, null);
- TrackingStruct manifestTracking =
- new TrackingStruct(EntryStatus.ADDED, 100L, 50L, 60L, null, null,
null, null);
+ tracking.inherit(100L, 0L);
- assertThatThrownBy(() -> tracking.inheritFrom(manifestTracking))
- .isInstanceOf(IllegalArgumentException.class)
- .hasMessage("Manifest data and file sequence numbers must be equal,
got 50 and 60");
+ assertThat(tracking.dataSequenceNumber()).isEqualTo(0L);
+ assertThat(tracking.fileSequenceNumber()).isEqualTo(0L);
}
@Test
- void noDefaultingWithoutInheritance() {
+ void inheritanceAddedEntriesInheritSequenceNumber() {
TrackingStruct tracking =
- new TrackingStruct(EntryStatus.ADDED, null, null, null, null, null,
null, null);
+ new TrackingStruct(EntryStatus.ADDED, 42L, null, null, null, null,
null, null);
+
+ tracking.inherit(100L, 60L);
- // no inheritance, nulls stay null
- assertThat(tracking.snapshotId()).isNull();
- assertThat(tracking.dataSequenceNumber()).isNull();
- assertThat(tracking.fileSequenceNumber()).isNull();
+ assertThat(tracking.dataSequenceNumber()).isEqualTo(60L);
+ assertThat(tracking.fileSequenceNumber()).isEqualTo(60L);
}
- @Test
- void inheritFromNullIsNoOp() {
- TrackingStruct tracking =
- new TrackingStruct(EntryStatus.ADDED, null, null, null, null, null,
null, null);
+ private static final List<EntryStatus> NON_INHERITING_STATUSES =
+ List.of(
+ EntryStatus.EXISTING, EntryStatus.MODIFIED, EntryStatus.DELETED,
EntryStatus.REPLACED);
- tracking.inheritFrom(null);
+ @ParameterizedTest
+ @FieldSource("NON_INHERITING_STATUSES")
+ void inheritanceWithNonInheritingStatus(EntryStatus status) {
+ TrackingStruct tracking = new TrackingStruct(status, 42L, null, null,
null, null, null, null);
- // null source is a no-op; all unset fields stay null
- assertThat(tracking.snapshotId()).isNull();
- assertThat(tracking.dataSequenceNumber()).isNull();
- assertThat(tracking.fileSequenceNumber()).isNull();
- }
+ tracking.inherit(100L, 60L);
- private static Tracking createManifestTracking(long snapshotId, long
sequenceNumber) {
- return new TrackingStruct(
- EntryStatus.ADDED, snapshotId, sequenceNumber, sequenceNumber, null,
null, null, null);
+ assertThat(tracking.dataSequenceNumber()).isEqualTo(null);
+ assertThat(tracking.fileSequenceNumber()).isEqualTo(null);
Review Comment:
minor: `isNull` instead of equalTo(null)?
--
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]
