You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "amogh-jahagirdar (via GitHub)" <gi...@apache.org> on 2026/07/01 23:49:41 UTC
[PR] Core: Cleanup some of the TrackedFile tests [iceberg]
amogh-jahagirdar opened a new pull request, #17035:
URL: https://github.com/apache/iceberg/pull/17035
(no comment)
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Cleanup some of the TrackedFile tests [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #17035:
URL: https://github.com/apache/iceberg/pull/17035#discussion_r3538328797
##########
core/src/test/java/org/apache/iceberg/TestTrackingStruct.java:
##########
@@ -495,6 +496,16 @@ void testInternalSetIgnoresUnknownOrdinal() {
assertThat(tracking.replacedPositions()).isEqualTo(ByteBuffer.wrap(new byte[] {3, 4}));
}
+ @Test
+ void testReaderSideFields() {
Review Comment:
@gaborkaszab @rdblue i went ahead and then just ommitted this change so that it can be included in @gaborkaszab change!
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Cleanup some of the TrackedFile tests [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #17035:
URL: https://github.com/apache/iceberg/pull/17035#discussion_r3532452669
##########
core/src/test/java/org/apache/iceberg/TestTrackedFileStruct.java:
##########
@@ -36,254 +36,203 @@ class TestTrackedFileStruct {
Types.NestedField.optional(1000, "id_bucket", Types.IntegerType.get()),
Types.NestedField.optional(1001, "category", Types.StringType.get()));
- // Ordinals looked up from the TrackedFile schema so tests don't hard-code positions.
- private static final List<Types.NestedField> SCHEMA_FIELDS =
- TrackedFile.schemaWithContentStats(Types.StructType.of(), Types.StructType.of()).fields();
- private static final int TRACKING_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.TRACKING);
- private static final int CONTENT_TYPE_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.CONTENT_TYPE);
- private static final int FORMAT_VERSION_ORDINAL =
- SCHEMA_FIELDS.indexOf(TrackedFile.FORMAT_VERSION);
- private static final int LOCATION_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.LOCATION);
- private static final int FILE_FORMAT_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.FILE_FORMAT);
- private static final int RECORD_COUNT_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.RECORD_COUNT);
- private static final int FILE_SIZE_IN_BYTES_ORDINAL =
- SCHEMA_FIELDS.indexOf(TrackedFile.FILE_SIZE_IN_BYTES);
- private static final int SPEC_ID_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.SPEC_ID);
- private static final int PARTITION_ORDINAL = ordinalOf(TrackedFile.PARTITION_ID);
- private static final int SORT_ORDER_ID_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.SORT_ORDER_ID);
- private static final int DELETION_VECTOR_ORDINAL =
- SCHEMA_FIELDS.indexOf(TrackedFile.DELETION_VECTOR);
- private static final int MANIFEST_INFO_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.MANIFEST_INFO);
- private static final int KEY_METADATA_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.KEY_METADATA);
- private static final int SPLIT_OFFSETS_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.SPLIT_OFFSETS);
- private static final int EQUALITY_IDS_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.EQUALITY_IDS);
-
- // Ordinal of MetadataColumns.ROW_POSITION within TrackingStruct's BASE_TYPE,
- // which appends ROW_POSITION after the Tracking schema fields.
- private static final int MANIFEST_POS_ORDINAL = Tracking.schema().fields().size();
-
- private static int ordinalOf(int fieldId) {
- for (int i = 0; i < SCHEMA_FIELDS.size(); i++) {
- if (SCHEMA_FIELDS.get(i).fieldId() == fieldId) {
- return i;
- }
- }
- throw new IllegalArgumentException("Field not found in TrackedFile schema: " + fieldId);
- }
+ private static final List<Types.NestedField> FIELDS =
+ TrackedFile.schemaWithContentStats(PARTITION_TYPE, Types.StructType.of()).fields();
+
+ private static final Tracking TRACKING =
+ new TrackingStruct(EntryStatus.ADDED, 42L, 10L, 10L, 43L, 1000L, null, null);
+
+ private static final PartitionData PARTITION = newPartition(7, "music");
+
+ private static final DeletionVectorStruct DELETION_VECTOR =
+ DeletionVectorStruct.builder()
+ .location("s3://bucket/dv.puffin")
+ .offset(100L)
+ .sizeInBytes(50L)
+ .cardinality(5L)
+ .build();
+
+ private static final ManifestInfoStruct MANIFEST_INFO =
+ ManifestInfoStruct.builder()
+ .addedFilesCount(10)
+ .existingFilesCount(20)
+ .deletedFilesCount(3)
+ .replacedFilesCount(2)
+ .addedRowsCount(1000L)
+ .existingRowsCount(2000L)
+ .deletedRowsCount(300L)
+ .replacedRowsCount(200L)
+ .minSequenceNumber(5L)
+ .build();
@Test
- void testFieldAccess() {
- TrackedFileStruct file = new TrackedFileStruct();
- Tracking tracking = TrackingBuilder.added(42L).build();
- DeletionVectorStruct dv =
- DeletionVectorStruct.builder()
- .location("s3://bucket/dv.puffin")
- .offset(100L)
- .sizeInBytes(50L)
- .cardinality(5L)
- .build();
- ManifestInfoStruct info =
- ManifestInfoStruct.builder()
- .addedFilesCount(10)
- .existingFilesCount(20)
- .deletedFilesCount(3)
- .replacedFilesCount(2)
- .addedRowsCount(1000L)
- .existingRowsCount(2000L)
- .deletedRowsCount(300L)
- .replacedRowsCount(200L)
- .minSequenceNumber(5L)
- .build();
-
- file.set(TRACKING_ORDINAL, tracking);
- file.set(CONTENT_TYPE_ORDINAL, FileContent.EQUALITY_DELETES.id());
- file.set(FORMAT_VERSION_ORDINAL, FORMAT_VERSION_V4);
- file.set(LOCATION_ORDINAL, "s3://bucket/data/eq-delete.avro");
- file.set(FILE_FORMAT_ORDINAL, "avro");
- file.set(RECORD_COUNT_ORDINAL, 50L);
- file.set(FILE_SIZE_IN_BYTES_ORDINAL, 512L);
- file.set(SPEC_ID_ORDINAL, 1);
- file.set(SORT_ORDER_ID_ORDINAL, 5);
- file.set(DELETION_VECTOR_ORDINAL, dv);
- file.set(MANIFEST_INFO_ORDINAL, info);
- file.set(KEY_METADATA_ORDINAL, ByteBuffer.wrap(new byte[] {1, 2, 3}));
- file.set(SPLIT_OFFSETS_ORDINAL, ImmutableList.of(100L, 200L));
- file.set(EQUALITY_IDS_ORDINAL, ImmutableList.of(1, 2, 3));
-
- assertThat(file.tracking()).isNotNull();
- assertThat(file.tracking().status()).isEqualTo(EntryStatus.ADDED);
- assertThat(file.tracking().snapshotId()).isEqualTo(42L);
- assertThat(file.contentType()).isEqualTo(FileContent.EQUALITY_DELETES);
+ void fieldAccess() {
Review Comment:
I would prefer `test` too but it looks like recently there's been changes to the contribution guidelines that for new tests we omit this prefix. So I'm just trying to keep consistent with that for these changes @nastra @dramaticlly
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Cleanup some of the TrackedFile tests [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #17035:
URL: https://github.com/apache/iceberg/pull/17035#discussion_r3509805088
##########
core/src/test/java/org/apache/iceberg/TestTrackedFileStruct.java:
##########
@@ -121,16 +125,99 @@ void testFieldAccess() {
assertThat(file.fileSizeInBytes()).isEqualTo(512L);
assertThat(file.specId()).isEqualTo(1);
assertThat(file.sortOrderId()).isEqualTo(5);
- assertThat(file.deletionVector()).isSameAs(dv);
- assertThat(file.manifestInfo()).isSameAs(info);
+ assertThat(file.deletionVector()).isSameAs(DELETION_VECTOR);
+ assertThat(file.manifestInfo()).isSameAs(MANIFEST_INFO);
assertThat(file.keyMetadata()).isEqualTo(ByteBuffer.wrap(new byte[] {1, 2, 3}));
assertThat(file.splitOffsets()).containsExactly(100L, 200L);
assertThat(file.equalityIds()).containsExactly(1, 2, 3);
- // should return EMPTY_PARTITION_DATA
+ // partition defaults to EMPTY_PARTITION_DATA when not provided
assertThat(file.partition()).isNotNull();
assertThat(file.partition().size()).isEqualTo(0);
}
+ @Test
+ void testSetByPosition() {
+ Tracking tracking = TrackingBuilder.added(42L).build();
+ PartitionData partition = newPartition(7, "music");
+
+ TrackedFileStruct file = new TrackedFileStruct();
+ file.set(0, tracking);
+ file.set(1, FileContent.EQUALITY_DELETES.id());
Review Comment:
Well we're not going to get 100% realistic if we have a single setter/getter test, just because we want to test all the fields . we could split across multiple tests but then it's a bit weird since we're testing outside this abstraction boundary. We can at least get closer though by using data files. the manifest fields don't apply but that's a bit easier read then eq deletes and setting avro....
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Cleanup some of the TrackedFile tests [iceberg]
Posted by "gaborkaszab (via GitHub)" <gi...@apache.org>.
gaborkaszab commented on code in PR #17035:
URL: https://github.com/apache/iceberg/pull/17035#discussion_r3518288166
##########
core/src/test/java/org/apache/iceberg/TestTrackedFileStruct.java:
##########
@@ -317,14 +477,43 @@ void testContentStatsNullWhenNotSet() {
void testAllFileContentTypesSupported() {
for (FileContent content : FileContent.values()) {
TrackedFileStruct file = new TrackedFileStruct();
- file.set(CONTENT_TYPE_ORDINAL, content.id());
+ file.set(1, content.id());
assertThat(file.contentType()).isEqualTo(content);
}
}
@Test
void testJavaSerializationRoundTrip() throws IOException, ClassNotFoundException {
- TrackedFileStruct file = createFullTrackedFile();
+ TrackingStruct tracking =
+ new TrackingStruct(
+ EntryStatus.ADDED,
+ 42L,
+ null,
Review Comment:
Unresolving because the second part of the comment still stands:
"Also, I see creation of Tracking is spelled out in every test. Since we don't test that class here, isn't that just noise for the sake of all these tests? Maybe introduce a static member for that as for all the nested structures?"
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Cleanup some of the TrackedFile tests [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #17035:
URL: https://github.com/apache/iceberg/pull/17035#discussion_r3532463521
##########
core/src/test/java/org/apache/iceberg/TestTrackedFileStruct.java:
##########
@@ -36,254 +36,203 @@ class TestTrackedFileStruct {
Types.NestedField.optional(1000, "id_bucket", Types.IntegerType.get()),
Types.NestedField.optional(1001, "category", Types.StringType.get()));
- // Ordinals looked up from the TrackedFile schema so tests don't hard-code positions.
- private static final List<Types.NestedField> SCHEMA_FIELDS =
- TrackedFile.schemaWithContentStats(Types.StructType.of(), Types.StructType.of()).fields();
- private static final int TRACKING_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.TRACKING);
- private static final int CONTENT_TYPE_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.CONTENT_TYPE);
- private static final int FORMAT_VERSION_ORDINAL =
- SCHEMA_FIELDS.indexOf(TrackedFile.FORMAT_VERSION);
- private static final int LOCATION_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.LOCATION);
- private static final int FILE_FORMAT_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.FILE_FORMAT);
- private static final int RECORD_COUNT_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.RECORD_COUNT);
- private static final int FILE_SIZE_IN_BYTES_ORDINAL =
- SCHEMA_FIELDS.indexOf(TrackedFile.FILE_SIZE_IN_BYTES);
- private static final int SPEC_ID_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.SPEC_ID);
- private static final int PARTITION_ORDINAL = ordinalOf(TrackedFile.PARTITION_ID);
- private static final int SORT_ORDER_ID_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.SORT_ORDER_ID);
- private static final int DELETION_VECTOR_ORDINAL =
- SCHEMA_FIELDS.indexOf(TrackedFile.DELETION_VECTOR);
- private static final int MANIFEST_INFO_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.MANIFEST_INFO);
- private static final int KEY_METADATA_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.KEY_METADATA);
- private static final int SPLIT_OFFSETS_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.SPLIT_OFFSETS);
- private static final int EQUALITY_IDS_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.EQUALITY_IDS);
-
- // Ordinal of MetadataColumns.ROW_POSITION within TrackingStruct's BASE_TYPE,
- // which appends ROW_POSITION after the Tracking schema fields.
- private static final int MANIFEST_POS_ORDINAL = Tracking.schema().fields().size();
-
- private static int ordinalOf(int fieldId) {
- for (int i = 0; i < SCHEMA_FIELDS.size(); i++) {
- if (SCHEMA_FIELDS.get(i).fieldId() == fieldId) {
- return i;
- }
- }
- throw new IllegalArgumentException("Field not found in TrackedFile schema: " + fieldId);
- }
+ private static final List<Types.NestedField> FIELDS =
+ TrackedFile.schemaWithContentStats(PARTITION_TYPE, Types.StructType.of()).fields();
+
+ private static final Tracking TRACKING =
+ new TrackingStruct(EntryStatus.ADDED, 42L, 10L, 10L, 43L, 1000L, null, null);
+
+ private static final PartitionData PARTITION = newPartition(7, "music");
+
+ private static final DeletionVectorStruct DELETION_VECTOR =
+ DeletionVectorStruct.builder()
+ .location("s3://bucket/dv.puffin")
+ .offset(100L)
+ .sizeInBytes(50L)
+ .cardinality(5L)
+ .build();
+
+ private static final ManifestInfoStruct MANIFEST_INFO =
+ ManifestInfoStruct.builder()
+ .addedFilesCount(10)
+ .existingFilesCount(20)
+ .deletedFilesCount(3)
+ .replacedFilesCount(2)
+ .addedRowsCount(1000L)
+ .existingRowsCount(2000L)
+ .deletedRowsCount(300L)
+ .replacedRowsCount(200L)
+ .minSequenceNumber(5L)
+ .build();
@Test
- void testFieldAccess() {
- TrackedFileStruct file = new TrackedFileStruct();
- Tracking tracking = TrackingBuilder.added(42L).build();
- DeletionVectorStruct dv =
- DeletionVectorStruct.builder()
- .location("s3://bucket/dv.puffin")
- .offset(100L)
- .sizeInBytes(50L)
- .cardinality(5L)
- .build();
- ManifestInfoStruct info =
- ManifestInfoStruct.builder()
- .addedFilesCount(10)
- .existingFilesCount(20)
- .deletedFilesCount(3)
- .replacedFilesCount(2)
- .addedRowsCount(1000L)
- .existingRowsCount(2000L)
- .deletedRowsCount(300L)
- .replacedRowsCount(200L)
- .minSequenceNumber(5L)
- .build();
-
- file.set(TRACKING_ORDINAL, tracking);
- file.set(CONTENT_TYPE_ORDINAL, FileContent.EQUALITY_DELETES.id());
- file.set(FORMAT_VERSION_ORDINAL, FORMAT_VERSION_V4);
- file.set(LOCATION_ORDINAL, "s3://bucket/data/eq-delete.avro");
- file.set(FILE_FORMAT_ORDINAL, "avro");
- file.set(RECORD_COUNT_ORDINAL, 50L);
- file.set(FILE_SIZE_IN_BYTES_ORDINAL, 512L);
- file.set(SPEC_ID_ORDINAL, 1);
- file.set(SORT_ORDER_ID_ORDINAL, 5);
- file.set(DELETION_VECTOR_ORDINAL, dv);
- file.set(MANIFEST_INFO_ORDINAL, info);
- file.set(KEY_METADATA_ORDINAL, ByteBuffer.wrap(new byte[] {1, 2, 3}));
- file.set(SPLIT_OFFSETS_ORDINAL, ImmutableList.of(100L, 200L));
- file.set(EQUALITY_IDS_ORDINAL, ImmutableList.of(1, 2, 3));
-
- assertThat(file.tracking()).isNotNull();
- assertThat(file.tracking().status()).isEqualTo(EntryStatus.ADDED);
- assertThat(file.tracking().snapshotId()).isEqualTo(42L);
- assertThat(file.contentType()).isEqualTo(FileContent.EQUALITY_DELETES);
+ void fieldAccess() {
+ TrackedFileStruct file =
+ new TrackedFileStruct(
+ TRACKING,
+ FileContent.DATA,
+ FORMAT_VERSION_V4,
+ "s3://bucket/data/00000-0-file.parquet",
+ FileFormat.PARQUET,
+ PARTITION,
+ 50L,
+ 512L,
+ 1,
+ null,
+ 5,
+ DELETION_VECTOR,
+ MANIFEST_INFO,
+ ByteBuffer.wrap(new byte[] {1, 2, 3}),
+ ImmutableList.of(100L, 200L),
+ ImmutableList.of(1, 2, 3));
+
+ assertThat(file.tracking()).isSameAs(TRACKING);
+ assertThat(file.contentType()).isEqualTo(FileContent.DATA);
assertThat(file.formatVersion()).isEqualTo(FORMAT_VERSION_V4);
- assertThat(file.location()).isEqualTo("s3://bucket/data/eq-delete.avro");
- assertThat(file.fileFormat()).isEqualTo(FileFormat.AVRO);
+ assertThat(file.location()).isEqualTo("s3://bucket/data/00000-0-file.parquet");
+ assertThat(file.fileFormat()).isEqualTo(FileFormat.PARQUET);
+ assertThat(file.partition()).isSameAs(PARTITION);
assertThat(file.recordCount()).isEqualTo(50L);
assertThat(file.fileSizeInBytes()).isEqualTo(512L);
assertThat(file.specId()).isEqualTo(1);
assertThat(file.sortOrderId()).isEqualTo(5);
- assertThat(file.deletionVector()).isSameAs(dv);
- assertThat(file.manifestInfo()).isSameAs(info);
+ assertThat(file.deletionVector()).isSameAs(DELETION_VECTOR);
+ assertThat(file.manifestInfo()).isSameAs(MANIFEST_INFO);
assertThat(file.keyMetadata()).isEqualTo(ByteBuffer.wrap(new byte[] {1, 2, 3}));
assertThat(file.splitOffsets()).containsExactly(100L, 200L);
assertThat(file.equalityIds()).containsExactly(1, 2, 3);
- // should return EMPTY_PARTITION_DATA
- assertThat(file.partition()).isNotNull();
- assertThat(file.partition().size()).isEqualTo(0);
}
@Test
- void testReaderSideFields() {
+ void setByPosition() {
TrackedFileStruct file = new TrackedFileStruct();
-
- TrackingStruct tracking = new TrackingStruct();
- tracking.setManifestLocation("s3://bucket/metadata/manifest.avro");
- tracking.set(MANIFEST_POS_ORDINAL, 7L);
-
- file.set(TRACKING_ORDINAL, tracking);
- file.set(CONTENT_TYPE_ORDINAL, FileContent.DATA.id());
- file.set(LOCATION_ORDINAL, "test");
- file.set(FILE_FORMAT_ORDINAL, "parquet");
- file.set(RECORD_COUNT_ORDINAL, 0L);
- file.set(FILE_SIZE_IN_BYTES_ORDINAL, 0L);
-
- assertThat(file.tracking().manifestLocation()).isEqualTo("s3://bucket/metadata/manifest.avro");
- assertThat(file.tracking().manifestPos()).isEqualTo(7L);
- }
-
- @Test
- void projectionWithoutPartition() {
- // project only location (field ID 100) and file_size_in_bytes (field ID 104)
- Types.StructType projection =
- Types.StructType.of(TrackedFile.LOCATION, TrackedFile.FILE_SIZE_IN_BYTES);
-
- TrackedFileStruct file = new TrackedFileStruct(projection);
- assertThat(file.size()).isEqualTo(2);
- // should return EMPTY_PARTITION_DATA
- assertThat(file.partition()).isNotNull();
- assertThat(file.partition().size()).isEqualTo(0);
+ file.set(pos("tracking"), TRACKING);
+ file.set(pos("content_type"), FileContent.DATA.id());
+ file.set(pos("format_version"), FORMAT_VERSION_V4);
+ file.set(pos("location"), "s3://bucket/data/00000-0-file.parquet");
+ file.set(pos("file_format"), "parquet");
+ file.set(pos("record_count"), 50L);
+ file.set(pos("file_size_in_bytes"), 512L);
+ file.set(pos("spec_id"), 1);
+ file.set(pos("partition"), PARTITION);
+ file.set(pos("sort_order_id"), 5);
+ file.set(pos("deletion_vector"), DELETION_VECTOR);
+ file.set(pos("manifest_info"), MANIFEST_INFO);
+ file.set(pos("key_metadata"), ByteBuffer.wrap(new byte[] {1, 2, 3}));
+ file.set(pos("split_offsets"), ImmutableList.of(100L, 200L));
+ file.set(pos("equality_ids"), ImmutableList.of(1, 2, 3));
+
+ assertThat(file.tracking()).isSameAs(TRACKING);
+ assertThat(file.contentType()).isEqualTo(FileContent.DATA);
+ assertThat(file.formatVersion()).isEqualTo(FORMAT_VERSION_V4);
+ assertThat(file.location()).isEqualTo("s3://bucket/data/00000-0-file.parquet");
+ assertThat(file.fileFormat()).isEqualTo(FileFormat.PARQUET);
+ assertThat(file.recordCount()).isEqualTo(50L);
+ assertThat(file.fileSizeInBytes()).isEqualTo(512L);
+ assertThat(file.specId()).isEqualTo(1);
+ assertThat(file.partition()).isSameAs(PARTITION);
+ assertThat(file.sortOrderId()).isEqualTo(5);
+ assertThat(file.deletionVector()).isSameAs(DELETION_VECTOR);
+ assertThat(file.manifestInfo()).isSameAs(MANIFEST_INFO);
+ assertThat(file.keyMetadata()).isEqualTo(ByteBuffer.wrap(new byte[] {1, 2, 3}));
+ assertThat(file.splitOffsets()).containsExactly(100L, 200L);
+ assertThat(file.equalityIds()).containsExactly(1, 2, 3);
}
@Test
- void partitionAccess() {
- PartitionData partition = newPartition(5, "books");
-
- TrackedFileStruct file = new TrackedFileStruct();
- file.set(PARTITION_ORDINAL, partition);
-
- assertThat(file.partition()).isSameAs(partition);
- assertThat(file.partition().get(0, Integer.class)).isEqualTo(5);
- assertThat(file.partition().get(1, String.class)).isEqualTo("books");
+ void getByPosition() {
+ TrackedFileStruct file =
+ new TrackedFileStruct(
+ TRACKING,
+ FileContent.DATA,
+ FORMAT_VERSION_V4,
+ "s3://bucket/data/00000-0-file.parquet",
+ FileFormat.PARQUET,
+ PARTITION,
+ 50L,
+ 512L,
+ 1,
+ null,
+ 5,
+ DELETION_VECTOR,
+ MANIFEST_INFO,
+ ByteBuffer.wrap(new byte[] {1, 2, 3}),
+ ImmutableList.of(100L, 200L),
+ ImmutableList.of(1, 2, 3));
+
+ assertThat(file.get(pos("tracking"), Tracking.class)).isSameAs(TRACKING);
+ assertThat(file.get(pos("content_type"), Integer.class)).isEqualTo(FileContent.DATA.id());
+ assertThat(file.get(pos("format_version"), Integer.class)).isEqualTo(FORMAT_VERSION_V4);
+ assertThat(file.get(pos("location"), String.class))
+ .isEqualTo("s3://bucket/data/00000-0-file.parquet");
+ assertThat(file.get(pos("file_format"), String.class)).isEqualTo(FileFormat.PARQUET.toString());
+ assertThat(file.get(pos("record_count"), Long.class)).isEqualTo(50L);
+ assertThat(file.get(pos("file_size_in_bytes"), Long.class)).isEqualTo(512L);
+ assertThat(file.get(pos("spec_id"), Integer.class)).isEqualTo(1);
+ assertThat(file.get(pos("partition"), PartitionData.class)).isSameAs(PARTITION);
+ assertThat(file.get(pos("sort_order_id"), Integer.class)).isEqualTo(5);
+ assertThat(file.get(pos("deletion_vector"), DeletionVector.class)).isSameAs(DELETION_VECTOR);
+ assertThat(file.get(pos("manifest_info"), ManifestInfo.class)).isSameAs(MANIFEST_INFO);
+ assertThat(file.get(pos("key_metadata"), ByteBuffer.class))
+ .isEqualTo(ByteBuffer.wrap(new byte[] {1, 2, 3}));
+ assertThat(file.get(pos("split_offsets"), List.class)).containsExactly(100L, 200L);
+ assertThat(file.get(pos("equality_ids"), List.class)).containsExactly(1, 2, 3);
}
@Test
- void partitionIsCopied() {
- PartitionData partition = newPartition(5, "books");
- TrackedFileStruct file = createFullTrackedFile();
- file.set(PARTITION_ORDINAL, partition);
+ void copy() {
+ TrackedFileStruct file =
+ new TrackedFileStruct(
+ TRACKING,
+ FileContent.DATA,
+ FORMAT_VERSION_V4,
+ "s3://bucket/data/00000-0-file.parquet",
+ FileFormat.PARQUET,
+ PARTITION,
+ 50L,
+ 512L,
+ 1,
+ null,
+ 5,
+ DELETION_VECTOR,
+ MANIFEST_INFO,
+ ByteBuffer.wrap(new byte[] {1, 2, 3}),
+ ImmutableList.of(100L, 200L),
+ ImmutableList.of(1, 2, 3));
TrackedFile copy = file.copy();
- assertThat(copy.partition()).isNotNull().isNotSameAs(partition);
- assertThat(copy.partition()).isEqualTo(partition);
- assertThat(copy.partition().get(0, Integer.class)).isEqualTo(5);
- assertThat(copy.partition().get(1, String.class)).isEqualTo("books");
- }
-
- @Test
- void testCopy() {
- TrackedFileStruct file = createFullTrackedFile();
-
- TrackedFile copy = file.copy();
assertThat(copy).isInstanceOf(TrackedFileStruct.class);
-
- assertThat(copy.contentType()).isEqualTo(FileContent.DATA);
- assertThat(copy.location()).isEqualTo("s3://bucket/data/file.parquet");
- assertThat(copy.fileFormat()).isEqualTo(FileFormat.PARQUET);
assertThat(copy.tracking().status()).isEqualTo(EntryStatus.ADDED);
assertThat(copy.tracking().snapshotId()).isEqualTo(42L);
- assertThat(copy.deletionVector().location()).isEqualTo("s3://bucket/dv.puffin");
- assertThat(copy.specId()).isEqualTo(0);
- assertThat(copy.sortOrderId()).isEqualTo(1);
- assertThat(copy.recordCount()).isEqualTo(100L);
- assertThat(copy.fileSizeInBytes()).isEqualTo(1024L);
- assertThat(copy.formatVersion()).isEqualTo(FORMAT_VERSION_V4);
- assertThat(copy.keyMetadata()).isNotNull();
- assertThat(copy.splitOffsets()).containsExactly(50L);
- assertThat(copy.equalityIds()).isNull();
- assertThat(copy.tracking().manifestLocation()).isEqualTo("s3://bucket/manifest.avro");
- assertThat(copy.tracking().manifestPos()).isEqualTo(3L);
- assertThat(copy.partition()).isEqualTo(newPartition(7, "music"));
- }
-
- @Test
- void testCopyWithoutStats() {
- TrackedFileStruct file = createTrackedFileWithStats();
- assertThat(file.contentStats()).isNotNull();
-
- TrackedFile copy = file.copyWithoutStats();
-
assertThat(copy.contentType()).isEqualTo(FileContent.DATA);
- assertThat(copy.location()).isEqualTo("s3://bucket/data/file.parquet");
- assertThat(copy.contentStats()).isNull();
- }
-
- @Test
- void testCopyWithStatsFilters() {
- TrackedFileStruct file = createTrackedFileWithStats();
- Set<Integer> keepFieldIds = ImmutableSet.of(1);
-
- TrackedFile copy = file.copyWithStats(keepFieldIds);
-
- assertThat(copy.contentStats()).isNotNull();
- ContentStats stats = copy.contentStats();
- assertThat(stats.fieldStats()).hasSize(1);
- assertThat(stats.fieldStats().get(0).fieldId()).isEqualTo(1);
- }
-
- @Test
- void testCopyIsDeep() {
- TrackedFileStruct file = createFullTrackedFile();
-
- TrackedFile copy = file.copy();
+ assertThat(copy.formatVersion()).isEqualTo(FORMAT_VERSION_V4);
+ assertThat(copy.location()).isEqualTo("s3://bucket/data/00000-0-file.parquet");
+ assertThat(copy.fileFormat()).isEqualTo(FileFormat.PARQUET);
+ assertThat(copy.recordCount()).isEqualTo(50L);
+ assertThat(copy.fileSizeInBytes()).isEqualTo(512L);
+ assertThat(copy.specId()).isEqualTo(1);
+ assertThat(copy.sortOrderId()).isEqualTo(5);
+ assertThat(copy.deletionVector().location()).isEqualTo("s3://bucket/dv.puffin");
+ assertThat(copy.manifestInfo().addedFilesCount()).isEqualTo(10);
+ assertThat(copy.manifestInfo().addedRowsCount()).isEqualTo(1000L);
Review Comment:
Are we missing a copy method for `ManifestInfo`?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Cleanup some of the TrackedFile tests [iceberg]
Posted by "gaborkaszab (via GitHub)" <gi...@apache.org>.
gaborkaszab commented on code in PR #17035:
URL: https://github.com/apache/iceberg/pull/17035#discussion_r3535155178
##########
core/src/test/java/org/apache/iceberg/TestTrackedFileStruct.java:
##########
@@ -36,254 +36,203 @@ class TestTrackedFileStruct {
Types.NestedField.optional(1000, "id_bucket", Types.IntegerType.get()),
Types.NestedField.optional(1001, "category", Types.StringType.get()));
- // Ordinals looked up from the TrackedFile schema so tests don't hard-code positions.
- private static final List<Types.NestedField> SCHEMA_FIELDS =
- TrackedFile.schemaWithContentStats(Types.StructType.of(), Types.StructType.of()).fields();
- private static final int TRACKING_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.TRACKING);
- private static final int CONTENT_TYPE_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.CONTENT_TYPE);
- private static final int FORMAT_VERSION_ORDINAL =
- SCHEMA_FIELDS.indexOf(TrackedFile.FORMAT_VERSION);
- private static final int LOCATION_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.LOCATION);
- private static final int FILE_FORMAT_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.FILE_FORMAT);
- private static final int RECORD_COUNT_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.RECORD_COUNT);
- private static final int FILE_SIZE_IN_BYTES_ORDINAL =
- SCHEMA_FIELDS.indexOf(TrackedFile.FILE_SIZE_IN_BYTES);
- private static final int SPEC_ID_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.SPEC_ID);
- private static final int PARTITION_ORDINAL = ordinalOf(TrackedFile.PARTITION_ID);
- private static final int SORT_ORDER_ID_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.SORT_ORDER_ID);
- private static final int DELETION_VECTOR_ORDINAL =
- SCHEMA_FIELDS.indexOf(TrackedFile.DELETION_VECTOR);
- private static final int MANIFEST_INFO_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.MANIFEST_INFO);
- private static final int KEY_METADATA_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.KEY_METADATA);
- private static final int SPLIT_OFFSETS_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.SPLIT_OFFSETS);
- private static final int EQUALITY_IDS_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.EQUALITY_IDS);
-
- // Ordinal of MetadataColumns.ROW_POSITION within TrackingStruct's BASE_TYPE,
- // which appends ROW_POSITION after the Tracking schema fields.
- private static final int MANIFEST_POS_ORDINAL = Tracking.schema().fields().size();
-
- private static int ordinalOf(int fieldId) {
- for (int i = 0; i < SCHEMA_FIELDS.size(); i++) {
- if (SCHEMA_FIELDS.get(i).fieldId() == fieldId) {
- return i;
- }
- }
- throw new IllegalArgumentException("Field not found in TrackedFile schema: " + fieldId);
- }
+ private static final List<Types.NestedField> FIELDS =
+ TrackedFile.schemaWithContentStats(PARTITION_TYPE, Types.StructType.of()).fields();
+
+ private static final Tracking TRACKING =
+ new TrackingStruct(EntryStatus.ADDED, 42L, 10L, 10L, 43L, 1000L, null, null);
+
+ private static final PartitionData PARTITION = newPartition(7, "music");
+
+ private static final DeletionVectorStruct DELETION_VECTOR =
+ DeletionVectorStruct.builder()
+ .location("s3://bucket/dv.puffin")
+ .offset(100L)
+ .sizeInBytes(50L)
+ .cardinality(5L)
+ .build();
+
+ private static final ManifestInfoStruct MANIFEST_INFO =
+ ManifestInfoStruct.builder()
+ .addedFilesCount(10)
+ .existingFilesCount(20)
+ .deletedFilesCount(3)
+ .replacedFilesCount(2)
+ .addedRowsCount(1000L)
+ .existingRowsCount(2000L)
+ .deletedRowsCount(300L)
+ .replacedRowsCount(200L)
+ .minSequenceNumber(5L)
+ .build();
@Test
- void testFieldAccess() {
- TrackedFileStruct file = new TrackedFileStruct();
- Tracking tracking = TrackingBuilder.added(42L).build();
- DeletionVectorStruct dv =
- DeletionVectorStruct.builder()
- .location("s3://bucket/dv.puffin")
- .offset(100L)
- .sizeInBytes(50L)
- .cardinality(5L)
- .build();
- ManifestInfoStruct info =
- ManifestInfoStruct.builder()
- .addedFilesCount(10)
- .existingFilesCount(20)
- .deletedFilesCount(3)
- .replacedFilesCount(2)
- .addedRowsCount(1000L)
- .existingRowsCount(2000L)
- .deletedRowsCount(300L)
- .replacedRowsCount(200L)
- .minSequenceNumber(5L)
- .build();
-
- file.set(TRACKING_ORDINAL, tracking);
- file.set(CONTENT_TYPE_ORDINAL, FileContent.EQUALITY_DELETES.id());
- file.set(FORMAT_VERSION_ORDINAL, FORMAT_VERSION_V4);
- file.set(LOCATION_ORDINAL, "s3://bucket/data/eq-delete.avro");
- file.set(FILE_FORMAT_ORDINAL, "avro");
- file.set(RECORD_COUNT_ORDINAL, 50L);
- file.set(FILE_SIZE_IN_BYTES_ORDINAL, 512L);
- file.set(SPEC_ID_ORDINAL, 1);
- file.set(SORT_ORDER_ID_ORDINAL, 5);
- file.set(DELETION_VECTOR_ORDINAL, dv);
- file.set(MANIFEST_INFO_ORDINAL, info);
- file.set(KEY_METADATA_ORDINAL, ByteBuffer.wrap(new byte[] {1, 2, 3}));
- file.set(SPLIT_OFFSETS_ORDINAL, ImmutableList.of(100L, 200L));
- file.set(EQUALITY_IDS_ORDINAL, ImmutableList.of(1, 2, 3));
-
- assertThat(file.tracking()).isNotNull();
- assertThat(file.tracking().status()).isEqualTo(EntryStatus.ADDED);
- assertThat(file.tracking().snapshotId()).isEqualTo(42L);
- assertThat(file.contentType()).isEqualTo(FileContent.EQUALITY_DELETES);
+ void fieldAccess() {
+ TrackedFileStruct file =
+ new TrackedFileStruct(
+ TRACKING,
+ FileContent.DATA,
+ FORMAT_VERSION_V4,
+ "s3://bucket/data/00000-0-file.parquet",
+ FileFormat.PARQUET,
+ PARTITION,
+ 50L,
+ 512L,
+ 1,
+ null,
+ 5,
+ DELETION_VECTOR,
+ MANIFEST_INFO,
+ ByteBuffer.wrap(new byte[] {1, 2, 3}),
+ ImmutableList.of(100L, 200L),
+ ImmutableList.of(1, 2, 3));
+
+ assertThat(file.tracking()).isSameAs(TRACKING);
+ assertThat(file.contentType()).isEqualTo(FileContent.DATA);
assertThat(file.formatVersion()).isEqualTo(FORMAT_VERSION_V4);
- assertThat(file.location()).isEqualTo("s3://bucket/data/eq-delete.avro");
- assertThat(file.fileFormat()).isEqualTo(FileFormat.AVRO);
+ assertThat(file.location()).isEqualTo("s3://bucket/data/00000-0-file.parquet");
+ assertThat(file.fileFormat()).isEqualTo(FileFormat.PARQUET);
+ assertThat(file.partition()).isSameAs(PARTITION);
assertThat(file.recordCount()).isEqualTo(50L);
assertThat(file.fileSizeInBytes()).isEqualTo(512L);
assertThat(file.specId()).isEqualTo(1);
assertThat(file.sortOrderId()).isEqualTo(5);
- assertThat(file.deletionVector()).isSameAs(dv);
- assertThat(file.manifestInfo()).isSameAs(info);
+ assertThat(file.deletionVector()).isSameAs(DELETION_VECTOR);
+ assertThat(file.manifestInfo()).isSameAs(MANIFEST_INFO);
assertThat(file.keyMetadata()).isEqualTo(ByteBuffer.wrap(new byte[] {1, 2, 3}));
assertThat(file.splitOffsets()).containsExactly(100L, 200L);
assertThat(file.equalityIds()).containsExactly(1, 2, 3);
- // should return EMPTY_PARTITION_DATA
- assertThat(file.partition()).isNotNull();
- assertThat(file.partition().size()).isEqualTo(0);
}
@Test
- void testReaderSideFields() {
+ void setByPosition() {
TrackedFileStruct file = new TrackedFileStruct();
-
- TrackingStruct tracking = new TrackingStruct();
- tracking.setManifestLocation("s3://bucket/metadata/manifest.avro");
- tracking.set(MANIFEST_POS_ORDINAL, 7L);
-
- file.set(TRACKING_ORDINAL, tracking);
- file.set(CONTENT_TYPE_ORDINAL, FileContent.DATA.id());
- file.set(LOCATION_ORDINAL, "test");
- file.set(FILE_FORMAT_ORDINAL, "parquet");
- file.set(RECORD_COUNT_ORDINAL, 0L);
- file.set(FILE_SIZE_IN_BYTES_ORDINAL, 0L);
-
- assertThat(file.tracking().manifestLocation()).isEqualTo("s3://bucket/metadata/manifest.avro");
- assertThat(file.tracking().manifestPos()).isEqualTo(7L);
- }
-
- @Test
- void projectionWithoutPartition() {
- // project only location (field ID 100) and file_size_in_bytes (field ID 104)
- Types.StructType projection =
- Types.StructType.of(TrackedFile.LOCATION, TrackedFile.FILE_SIZE_IN_BYTES);
-
- TrackedFileStruct file = new TrackedFileStruct(projection);
- assertThat(file.size()).isEqualTo(2);
- // should return EMPTY_PARTITION_DATA
- assertThat(file.partition()).isNotNull();
- assertThat(file.partition().size()).isEqualTo(0);
+ file.set(pos("tracking"), TRACKING);
+ file.set(pos("content_type"), FileContent.DATA.id());
+ file.set(pos("format_version"), FORMAT_VERSION_V4);
+ file.set(pos("location"), "s3://bucket/data/00000-0-file.parquet");
+ file.set(pos("file_format"), "parquet");
+ file.set(pos("record_count"), 50L);
+ file.set(pos("file_size_in_bytes"), 512L);
+ file.set(pos("spec_id"), 1);
Review Comment:
I know there was a comment that ContentStats are being restructured. However, if removing the getter/setter test coverage now, would we remember adding the test for that field later on? For me having coverage now that breaks later is better, than keeping in mind to add it later.
##########
core/src/test/java/org/apache/iceberg/TestTrackingStruct.java:
##########
@@ -495,6 +496,16 @@ void testInternalSetIgnoresUnknownOrdinal() {
assertThat(tracking.replacedPositions()).isEqualTo(ByteBuffer.wrap(new byte[] {3, 4}));
}
+ @Test
+ void testReaderSideFields() {
Review Comment:
FYI, I've already opened a PR for the TrackingStruct counterpart: https://github.com/apache/iceberg/pull/17041
##########
core/src/test/java/org/apache/iceberg/TestTrackedFileStruct.java:
##########
@@ -36,254 +36,203 @@ class TestTrackedFileStruct {
Types.NestedField.optional(1000, "id_bucket", Types.IntegerType.get()),
Types.NestedField.optional(1001, "category", Types.StringType.get()));
- // Ordinals looked up from the TrackedFile schema so tests don't hard-code positions.
- private static final List<Types.NestedField> SCHEMA_FIELDS =
- TrackedFile.schemaWithContentStats(Types.StructType.of(), Types.StructType.of()).fields();
- private static final int TRACKING_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.TRACKING);
- private static final int CONTENT_TYPE_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.CONTENT_TYPE);
- private static final int FORMAT_VERSION_ORDINAL =
- SCHEMA_FIELDS.indexOf(TrackedFile.FORMAT_VERSION);
- private static final int LOCATION_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.LOCATION);
- private static final int FILE_FORMAT_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.FILE_FORMAT);
- private static final int RECORD_COUNT_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.RECORD_COUNT);
- private static final int FILE_SIZE_IN_BYTES_ORDINAL =
- SCHEMA_FIELDS.indexOf(TrackedFile.FILE_SIZE_IN_BYTES);
- private static final int SPEC_ID_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.SPEC_ID);
- private static final int PARTITION_ORDINAL = ordinalOf(TrackedFile.PARTITION_ID);
- private static final int SORT_ORDER_ID_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.SORT_ORDER_ID);
- private static final int DELETION_VECTOR_ORDINAL =
- SCHEMA_FIELDS.indexOf(TrackedFile.DELETION_VECTOR);
- private static final int MANIFEST_INFO_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.MANIFEST_INFO);
- private static final int KEY_METADATA_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.KEY_METADATA);
- private static final int SPLIT_OFFSETS_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.SPLIT_OFFSETS);
- private static final int EQUALITY_IDS_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.EQUALITY_IDS);
-
- // Ordinal of MetadataColumns.ROW_POSITION within TrackingStruct's BASE_TYPE,
- // which appends ROW_POSITION after the Tracking schema fields.
- private static final int MANIFEST_POS_ORDINAL = Tracking.schema().fields().size();
-
- private static int ordinalOf(int fieldId) {
- for (int i = 0; i < SCHEMA_FIELDS.size(); i++) {
- if (SCHEMA_FIELDS.get(i).fieldId() == fieldId) {
- return i;
- }
- }
- throw new IllegalArgumentException("Field not found in TrackedFile schema: " + fieldId);
- }
+ private static final List<Types.NestedField> FIELDS =
+ TrackedFile.schemaWithContentStats(PARTITION_TYPE, Types.StructType.of()).fields();
+
+ private static final Tracking TRACKING =
+ new TrackingStruct(EntryStatus.ADDED, 42L, 10L, 10L, 43L, 1000L, null, null);
+
+ private static final PartitionData PARTITION = newPartition(7, "music");
+
+ private static final DeletionVectorStruct DELETION_VECTOR =
+ DeletionVectorStruct.builder()
+ .location("s3://bucket/dv.puffin")
+ .offset(100L)
+ .sizeInBytes(50L)
+ .cardinality(5L)
+ .build();
+
+ private static final ManifestInfoStruct MANIFEST_INFO =
+ ManifestInfoStruct.builder()
+ .addedFilesCount(10)
+ .existingFilesCount(20)
+ .deletedFilesCount(3)
+ .replacedFilesCount(2)
+ .addedRowsCount(1000L)
+ .existingRowsCount(2000L)
+ .deletedRowsCount(300L)
+ .replacedRowsCount(200L)
+ .minSequenceNumber(5L)
+ .build();
@Test
- void testFieldAccess() {
- TrackedFileStruct file = new TrackedFileStruct();
- Tracking tracking = TrackingBuilder.added(42L).build();
- DeletionVectorStruct dv =
- DeletionVectorStruct.builder()
- .location("s3://bucket/dv.puffin")
- .offset(100L)
- .sizeInBytes(50L)
- .cardinality(5L)
- .build();
- ManifestInfoStruct info =
- ManifestInfoStruct.builder()
- .addedFilesCount(10)
- .existingFilesCount(20)
- .deletedFilesCount(3)
- .replacedFilesCount(2)
- .addedRowsCount(1000L)
- .existingRowsCount(2000L)
- .deletedRowsCount(300L)
- .replacedRowsCount(200L)
- .minSequenceNumber(5L)
- .build();
-
- file.set(TRACKING_ORDINAL, tracking);
- file.set(CONTENT_TYPE_ORDINAL, FileContent.EQUALITY_DELETES.id());
- file.set(FORMAT_VERSION_ORDINAL, FORMAT_VERSION_V4);
- file.set(LOCATION_ORDINAL, "s3://bucket/data/eq-delete.avro");
- file.set(FILE_FORMAT_ORDINAL, "avro");
- file.set(RECORD_COUNT_ORDINAL, 50L);
- file.set(FILE_SIZE_IN_BYTES_ORDINAL, 512L);
- file.set(SPEC_ID_ORDINAL, 1);
- file.set(SORT_ORDER_ID_ORDINAL, 5);
- file.set(DELETION_VECTOR_ORDINAL, dv);
- file.set(MANIFEST_INFO_ORDINAL, info);
- file.set(KEY_METADATA_ORDINAL, ByteBuffer.wrap(new byte[] {1, 2, 3}));
- file.set(SPLIT_OFFSETS_ORDINAL, ImmutableList.of(100L, 200L));
- file.set(EQUALITY_IDS_ORDINAL, ImmutableList.of(1, 2, 3));
-
- assertThat(file.tracking()).isNotNull();
- assertThat(file.tracking().status()).isEqualTo(EntryStatus.ADDED);
- assertThat(file.tracking().snapshotId()).isEqualTo(42L);
- assertThat(file.contentType()).isEqualTo(FileContent.EQUALITY_DELETES);
+ void fieldAccess() {
+ TrackedFileStruct file =
+ new TrackedFileStruct(
+ TRACKING,
+ FileContent.DATA,
+ FORMAT_VERSION_V4,
+ "s3://bucket/data/00000-0-file.parquet",
+ FileFormat.PARQUET,
+ PARTITION,
+ 50L,
+ 512L,
+ 1,
+ null,
+ 5,
+ DELETION_VECTOR,
+ MANIFEST_INFO,
+ ByteBuffer.wrap(new byte[] {1, 2, 3}),
+ ImmutableList.of(100L, 200L),
+ ImmutableList.of(1, 2, 3));
+
+ assertThat(file.tracking()).isSameAs(TRACKING);
+ assertThat(file.contentType()).isEqualTo(FileContent.DATA);
assertThat(file.formatVersion()).isEqualTo(FORMAT_VERSION_V4);
- assertThat(file.location()).isEqualTo("s3://bucket/data/eq-delete.avro");
- assertThat(file.fileFormat()).isEqualTo(FileFormat.AVRO);
+ assertThat(file.location()).isEqualTo("s3://bucket/data/00000-0-file.parquet");
+ assertThat(file.fileFormat()).isEqualTo(FileFormat.PARQUET);
+ assertThat(file.partition()).isSameAs(PARTITION);
assertThat(file.recordCount()).isEqualTo(50L);
assertThat(file.fileSizeInBytes()).isEqualTo(512L);
assertThat(file.specId()).isEqualTo(1);
assertThat(file.sortOrderId()).isEqualTo(5);
- assertThat(file.deletionVector()).isSameAs(dv);
- assertThat(file.manifestInfo()).isSameAs(info);
+ assertThat(file.deletionVector()).isSameAs(DELETION_VECTOR);
+ assertThat(file.manifestInfo()).isSameAs(MANIFEST_INFO);
assertThat(file.keyMetadata()).isEqualTo(ByteBuffer.wrap(new byte[] {1, 2, 3}));
assertThat(file.splitOffsets()).containsExactly(100L, 200L);
assertThat(file.equalityIds()).containsExactly(1, 2, 3);
- // should return EMPTY_PARTITION_DATA
- assertThat(file.partition()).isNotNull();
- assertThat(file.partition().size()).isEqualTo(0);
}
@Test
- void testReaderSideFields() {
Review Comment:
FYI, I have opened the `TrackingStruct` counterpart of this: https://github.com/apache/iceberg/pull/17041
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Cleanup some of the TrackedFile tests [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #17035:
URL: https://github.com/apache/iceberg/pull/17035#discussion_r3532445199
##########
core/src/test/java/org/apache/iceberg/TestTrackedFileStruct.java:
##########
@@ -36,254 +36,203 @@ class TestTrackedFileStruct {
Types.NestedField.optional(1000, "id_bucket", Types.IntegerType.get()),
Types.NestedField.optional(1001, "category", Types.StringType.get()));
- // Ordinals looked up from the TrackedFile schema so tests don't hard-code positions.
- private static final List<Types.NestedField> SCHEMA_FIELDS =
- TrackedFile.schemaWithContentStats(Types.StructType.of(), Types.StructType.of()).fields();
- private static final int TRACKING_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.TRACKING);
- private static final int CONTENT_TYPE_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.CONTENT_TYPE);
- private static final int FORMAT_VERSION_ORDINAL =
- SCHEMA_FIELDS.indexOf(TrackedFile.FORMAT_VERSION);
- private static final int LOCATION_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.LOCATION);
- private static final int FILE_FORMAT_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.FILE_FORMAT);
- private static final int RECORD_COUNT_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.RECORD_COUNT);
- private static final int FILE_SIZE_IN_BYTES_ORDINAL =
- SCHEMA_FIELDS.indexOf(TrackedFile.FILE_SIZE_IN_BYTES);
- private static final int SPEC_ID_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.SPEC_ID);
- private static final int PARTITION_ORDINAL = ordinalOf(TrackedFile.PARTITION_ID);
- private static final int SORT_ORDER_ID_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.SORT_ORDER_ID);
- private static final int DELETION_VECTOR_ORDINAL =
- SCHEMA_FIELDS.indexOf(TrackedFile.DELETION_VECTOR);
- private static final int MANIFEST_INFO_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.MANIFEST_INFO);
- private static final int KEY_METADATA_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.KEY_METADATA);
- private static final int SPLIT_OFFSETS_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.SPLIT_OFFSETS);
- private static final int EQUALITY_IDS_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.EQUALITY_IDS);
-
- // Ordinal of MetadataColumns.ROW_POSITION within TrackingStruct's BASE_TYPE,
- // which appends ROW_POSITION after the Tracking schema fields.
- private static final int MANIFEST_POS_ORDINAL = Tracking.schema().fields().size();
-
- private static int ordinalOf(int fieldId) {
- for (int i = 0; i < SCHEMA_FIELDS.size(); i++) {
- if (SCHEMA_FIELDS.get(i).fieldId() == fieldId) {
- return i;
- }
- }
- throw new IllegalArgumentException("Field not found in TrackedFile schema: " + fieldId);
- }
+ private static final List<Types.NestedField> FIELDS =
+ TrackedFile.schemaWithContentStats(PARTITION_TYPE, Types.StructType.of()).fields();
+
+ private static final Tracking TRACKING =
+ new TrackingStruct(EntryStatus.ADDED, 42L, 10L, 10L, 43L, 1000L, null, null);
+
+ private static final PartitionData PARTITION = newPartition(7, "music");
+
+ private static final DeletionVectorStruct DELETION_VECTOR =
+ DeletionVectorStruct.builder()
+ .location("s3://bucket/dv.puffin")
+ .offset(100L)
+ .sizeInBytes(50L)
+ .cardinality(5L)
+ .build();
+
+ private static final ManifestInfoStruct MANIFEST_INFO =
+ ManifestInfoStruct.builder()
+ .addedFilesCount(10)
+ .existingFilesCount(20)
+ .deletedFilesCount(3)
+ .replacedFilesCount(2)
+ .addedRowsCount(1000L)
+ .existingRowsCount(2000L)
+ .deletedRowsCount(300L)
+ .replacedRowsCount(200L)
+ .minSequenceNumber(5L)
+ .build();
@Test
- void testFieldAccess() {
- TrackedFileStruct file = new TrackedFileStruct();
- Tracking tracking = TrackingBuilder.added(42L).build();
- DeletionVectorStruct dv =
- DeletionVectorStruct.builder()
- .location("s3://bucket/dv.puffin")
- .offset(100L)
- .sizeInBytes(50L)
- .cardinality(5L)
- .build();
- ManifestInfoStruct info =
- ManifestInfoStruct.builder()
- .addedFilesCount(10)
- .existingFilesCount(20)
- .deletedFilesCount(3)
- .replacedFilesCount(2)
- .addedRowsCount(1000L)
- .existingRowsCount(2000L)
- .deletedRowsCount(300L)
- .replacedRowsCount(200L)
- .minSequenceNumber(5L)
- .build();
-
- file.set(TRACKING_ORDINAL, tracking);
- file.set(CONTENT_TYPE_ORDINAL, FileContent.EQUALITY_DELETES.id());
- file.set(FORMAT_VERSION_ORDINAL, FORMAT_VERSION_V4);
- file.set(LOCATION_ORDINAL, "s3://bucket/data/eq-delete.avro");
- file.set(FILE_FORMAT_ORDINAL, "avro");
- file.set(RECORD_COUNT_ORDINAL, 50L);
- file.set(FILE_SIZE_IN_BYTES_ORDINAL, 512L);
- file.set(SPEC_ID_ORDINAL, 1);
- file.set(SORT_ORDER_ID_ORDINAL, 5);
- file.set(DELETION_VECTOR_ORDINAL, dv);
- file.set(MANIFEST_INFO_ORDINAL, info);
- file.set(KEY_METADATA_ORDINAL, ByteBuffer.wrap(new byte[] {1, 2, 3}));
- file.set(SPLIT_OFFSETS_ORDINAL, ImmutableList.of(100L, 200L));
- file.set(EQUALITY_IDS_ORDINAL, ImmutableList.of(1, 2, 3));
-
- assertThat(file.tracking()).isNotNull();
- assertThat(file.tracking().status()).isEqualTo(EntryStatus.ADDED);
- assertThat(file.tracking().snapshotId()).isEqualTo(42L);
- assertThat(file.contentType()).isEqualTo(FileContent.EQUALITY_DELETES);
+ void fieldAccess() {
Review Comment:
The convention used throughout the codebase is to start test methods with `test` so they are easily distinguishable from helper methods. Are we no longer doing that?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Cleanup some of the TrackedFile tests [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #17035:
URL: https://github.com/apache/iceberg/pull/17035#discussion_r3538311006
##########
core/src/test/java/org/apache/iceberg/TestTrackedFileStruct.java:
##########
@@ -294,112 +243,52 @@ void testProjectedStructLike() {
}
@Test
- void testContentStatsReturnedWhenPresent() {
- TrackedFileStruct file = createTrackedFileWithStats();
- assertThat(file.contentStats()).isNotNull();
- assertThat(file.contentStats().fieldStats()).hasSize(2);
- }
-
- @Test
- void testContentStatsNullWhenNotSet() {
+ void structLikeSize() {
TrackedFileStruct file = new TrackedFileStruct();
- file.set(CONTENT_TYPE_ORDINAL, FileContent.DATA.id());
- file.set(LOCATION_ORDINAL, "test");
- file.set(FILE_FORMAT_ORDINAL, "parquet");
- file.set(RECORD_COUNT_ORDINAL, 0L);
- file.set(FILE_SIZE_IN_BYTES_ORDINAL, 0L);
- file.set(SPEC_ID_ORDINAL, 0);
-
- assertThat(file.contentStats()).isNull();
- }
-
- @Test
- void testAllFileContentTypesSupported() {
- for (FileContent content : FileContent.values()) {
- TrackedFileStruct file = new TrackedFileStruct();
- file.set(CONTENT_TYPE_ORDINAL, content.id());
- assertThat(file.contentType()).isEqualTo(content);
- }
+ assertThat(file.size()).isEqualTo(16);
}
- @Test
- void testJavaSerializationRoundTrip() throws IOException, ClassNotFoundException {
- TrackedFileStruct file = createFullTrackedFile();
-
- TrackedFileStruct deserialized = TestHelpers.roundTripSerialize(file);
+ @ParameterizedTest
+ @MethodSource("org.apache.iceberg.TestHelpers#serializers")
+ void serializationRoundTrip(RoundTripSerializer<TrackedFileStruct> serializer) throws Exception {
+ TrackedFileStruct file =
+ new TrackedFileStruct(
+ TRACKING,
+ FileContent.DATA,
+ FORMAT_VERSION_V4,
+ "s3://bucket/data/file.parquet",
+ FileFormat.PARQUET,
+ PARTITION,
+ 100L,
+ 1024L,
+ 7,
+ null,
+ 1,
+ DELETION_VECTOR,
+ MANIFEST_INFO,
+ ByteBuffer.wrap(new byte[] {1, 2, 3}),
+ ImmutableList.of(50L),
+ ImmutableList.of(1, 2, 3));
+
+ TrackedFileStruct deserialized = serializer.apply(file);
- assertThat(deserialized.contentType()).isEqualTo(FileContent.DATA);
- assertThat(deserialized.formatVersion()).isEqualTo(FORMAT_VERSION_V4);
- assertThat(deserialized.location()).isEqualTo("s3://bucket/data/file.parquet");
- assertThat(deserialized.fileFormat()).isEqualTo(FileFormat.PARQUET);
- assertThat(deserialized.recordCount()).isEqualTo(100L);
- assertThat(deserialized.fileSizeInBytes()).isEqualTo(1024L);
- assertThat(deserialized.specId()).isEqualTo(0);
- assertThat(deserialized.sortOrderId()).isEqualTo(1);
assertThat(deserialized.tracking().status()).isEqualTo(EntryStatus.ADDED);
assertThat(deserialized.tracking().snapshotId()).isEqualTo(42L);
- assertThat(deserialized.deletionVector().location()).isEqualTo("s3://bucket/dv.puffin");
- assertThat(deserialized.keyMetadata()).isEqualTo(ByteBuffer.wrap(new byte[] {1, 2, 3}));
- assertThat(deserialized.splitOffsets()).containsExactly(50L);
- assertThat(deserialized.tracking().manifestPos()).isEqualTo(3L);
- assertThat(deserialized.tracking().manifestLocation()).isEqualTo("s3://bucket/manifest.avro");
- assertThat(deserialized.partition()).isEqualTo(newPartition(7, "music"));
- }
-
- @Test
- void testKryoSerializationRoundTrip() throws IOException {
- TrackedFileStruct file = createFullTrackedFile();
-
- TrackedFileStruct deserialized = TestHelpers.KryoHelpers.roundTripSerialize(file);
-
assertThat(deserialized.contentType()).isEqualTo(FileContent.DATA);
assertThat(deserialized.formatVersion()).isEqualTo(FORMAT_VERSION_V4);
assertThat(deserialized.location()).isEqualTo("s3://bucket/data/file.parquet");
assertThat(deserialized.fileFormat()).isEqualTo(FileFormat.PARQUET);
+ assertThat(deserialized.partition()).isEqualTo(PARTITION);
Review Comment:
Using a struct like comparator now
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Cleanup some of the TrackedFile tests [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #17035:
URL: https://github.com/apache/iceberg/pull/17035#discussion_r3509715654
##########
core/src/test/java/org/apache/iceberg/TestTrackedFileStruct.java:
##########
@@ -121,16 +125,99 @@ void testFieldAccess() {
assertThat(file.fileSizeInBytes()).isEqualTo(512L);
assertThat(file.specId()).isEqualTo(1);
assertThat(file.sortOrderId()).isEqualTo(5);
- assertThat(file.deletionVector()).isSameAs(dv);
- assertThat(file.manifestInfo()).isSameAs(info);
+ assertThat(file.deletionVector()).isSameAs(DELETION_VECTOR);
+ assertThat(file.manifestInfo()).isSameAs(MANIFEST_INFO);
assertThat(file.keyMetadata()).isEqualTo(ByteBuffer.wrap(new byte[] {1, 2, 3}));
assertThat(file.splitOffsets()).containsExactly(100L, 200L);
assertThat(file.equalityIds()).containsExactly(1, 2, 3);
- // should return EMPTY_PARTITION_DATA
+ // partition defaults to EMPTY_PARTITION_DATA when not provided
assertThat(file.partition()).isNotNull();
assertThat(file.partition().size()).isEqualTo(0);
}
+ @Test
+ void testSetByPosition() {
+ Tracking tracking = TrackingBuilder.added(42L).build();
+ PartitionData partition = newPartition(7, "music");
+
+ TrackedFileStruct file = new TrackedFileStruct();
+ file.set(0, tracking);
+ file.set(1, FileContent.EQUALITY_DELETES.id());
Review Comment:
Ok looks like it was just taking from the previous test code, we should use a realistic example here
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Cleanup some of the TrackedFile tests [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #17035:
URL: https://github.com/apache/iceberg/pull/17035#discussion_r3532488440
##########
core/src/test/java/org/apache/iceberg/TestTrackingStruct.java:
##########
@@ -495,6 +496,16 @@ void testInternalSetIgnoresUnknownOrdinal() {
assertThat(tracking.replacedPositions()).isEqualTo(ByteBuffer.wrap(new byte[] {3, 4}));
}
+ @Test
+ void testReaderSideFields() {
Review Comment:
The tests for `Tracking` should mirror the `TrackedFile` tests, except that we can omit the serialization ones (covered by `TrackedFile` tests). I would probably do that in a separate PR, so it makes sense not to change this right now.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Cleanup some of the TrackedFile tests [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #17035:
URL: https://github.com/apache/iceberg/pull/17035#discussion_r3532476359
##########
core/src/test/java/org/apache/iceberg/TestTrackedFileStruct.java:
##########
@@ -294,112 +243,52 @@ void testProjectedStructLike() {
}
@Test
- void testContentStatsReturnedWhenPresent() {
- TrackedFileStruct file = createTrackedFileWithStats();
- assertThat(file.contentStats()).isNotNull();
- assertThat(file.contentStats().fieldStats()).hasSize(2);
- }
-
- @Test
- void testContentStatsNullWhenNotSet() {
+ void structLikeSize() {
TrackedFileStruct file = new TrackedFileStruct();
- file.set(CONTENT_TYPE_ORDINAL, FileContent.DATA.id());
- file.set(LOCATION_ORDINAL, "test");
- file.set(FILE_FORMAT_ORDINAL, "parquet");
- file.set(RECORD_COUNT_ORDINAL, 0L);
- file.set(FILE_SIZE_IN_BYTES_ORDINAL, 0L);
- file.set(SPEC_ID_ORDINAL, 0);
-
- assertThat(file.contentStats()).isNull();
- }
-
- @Test
- void testAllFileContentTypesSupported() {
- for (FileContent content : FileContent.values()) {
- TrackedFileStruct file = new TrackedFileStruct();
- file.set(CONTENT_TYPE_ORDINAL, content.id());
- assertThat(file.contentType()).isEqualTo(content);
- }
+ assertThat(file.size()).isEqualTo(16);
}
- @Test
- void testJavaSerializationRoundTrip() throws IOException, ClassNotFoundException {
- TrackedFileStruct file = createFullTrackedFile();
-
- TrackedFileStruct deserialized = TestHelpers.roundTripSerialize(file);
+ @ParameterizedTest
+ @MethodSource("org.apache.iceberg.TestHelpers#serializers")
+ void serializationRoundTrip(RoundTripSerializer<TrackedFileStruct> serializer) throws Exception {
+ TrackedFileStruct file =
+ new TrackedFileStruct(
+ TRACKING,
+ FileContent.DATA,
+ FORMAT_VERSION_V4,
+ "s3://bucket/data/file.parquet",
+ FileFormat.PARQUET,
+ PARTITION,
+ 100L,
+ 1024L,
+ 7,
+ null,
+ 1,
+ DELETION_VECTOR,
+ MANIFEST_INFO,
+ ByteBuffer.wrap(new byte[] {1, 2, 3}),
+ ImmutableList.of(50L),
+ ImmutableList.of(1, 2, 3));
+
+ TrackedFileStruct deserialized = serializer.apply(file);
- assertThat(deserialized.contentType()).isEqualTo(FileContent.DATA);
- assertThat(deserialized.formatVersion()).isEqualTo(FORMAT_VERSION_V4);
- assertThat(deserialized.location()).isEqualTo("s3://bucket/data/file.parquet");
- assertThat(deserialized.fileFormat()).isEqualTo(FileFormat.PARQUET);
- assertThat(deserialized.recordCount()).isEqualTo(100L);
- assertThat(deserialized.fileSizeInBytes()).isEqualTo(1024L);
- assertThat(deserialized.specId()).isEqualTo(0);
- assertThat(deserialized.sortOrderId()).isEqualTo(1);
assertThat(deserialized.tracking().status()).isEqualTo(EntryStatus.ADDED);
assertThat(deserialized.tracking().snapshotId()).isEqualTo(42L);
Review Comment:
Probably worth a comment that this is only spot checking the contents of `TRACKING`.
We could also use a `StructLikeComparator` to check all fields. Is there a downside to checking them all?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Cleanup some of the TrackedFile tests [iceberg]
Posted by "stevenzwu (via GitHub)" <gi...@apache.org>.
stevenzwu commented on code in PR #17035:
URL: https://github.com/apache/iceberg/pull/17035#discussion_r3510455831
##########
core/src/test/java/org/apache/iceberg/TestTrackedFileStruct.java:
##########
@@ -317,14 +477,43 @@ void testContentStatsNullWhenNotSet() {
void testAllFileContentTypesSupported() {
for (FileContent content : FileContent.values()) {
TrackedFileStruct file = new TrackedFileStruct();
- file.set(CONTENT_TYPE_ORDINAL, content.id());
+ file.set(1, content.id());
assertThat(file.contentType()).isEqualTo(content);
}
}
@Test
void testJavaSerializationRoundTrip() throws IOException, ClassNotFoundException {
- TrackedFileStruct file = createFullTrackedFile();
+ TrackingStruct tracking =
+ new TrackingStruct(
+ EntryStatus.ADDED,
+ 42L,
+ null,
Review Comment:
nit: since we set manifest location and position, it is for the read path of committed entry. we probably should populate the seq numbers and first row id. since we are testing serialization round-trip, we might just fill in and assert all applicable fields. so together, I am thinking about these fields.
```
Long dataSequenceNumber,
Long fileSequenceNumber,
Long dvSnapshotId,
Long firstRowId,
byte[] deletedPositions,
byte[] replacedPositions
```
##########
core/src/test/java/org/apache/iceberg/TestTrackedFileStruct.java:
##########
@@ -348,7 +537,36 @@ void testJavaSerializationRoundTrip() throws IOException, ClassNotFoundException
@Test
void testKryoSerializationRoundTrip() throws IOException {
Review Comment:
maybe this can be merged with the previous serdes test and use a parameterized function arg.
```
TestHelpers.roundTripSerialize
TestHelpers.KryoHelpers.roundTripSerialize
```
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Cleanup some of the TrackedFile tests [iceberg]
Posted by "gaborkaszab (via GitHub)" <gi...@apache.org>.
gaborkaszab commented on code in PR #17035:
URL: https://github.com/apache/iceberg/pull/17035#discussion_r3518450468
##########
core/src/test/java/org/apache/iceberg/TestTrackedFileStruct.java:
##########
@@ -348,7 +537,36 @@ void testJavaSerializationRoundTrip() throws IOException, ClassNotFoundException
@Test
void testKryoSerializationRoundTrip() throws IOException {
Review Comment:
In fact there is already a method source param for this:
`@MethodSource("org.apache.iceberg.TestHelpers#serializers")`
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Cleanup some of the TrackedFile tests [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #17035:
URL: https://github.com/apache/iceberg/pull/17035#discussion_r3509840961
##########
core/src/test/java/org/apache/iceberg/TestTrackedFileStruct.java:
##########
@@ -229,7 +355,24 @@ void testCopyWithoutStats() {
@Test
void testCopyWithStatsFilters() {
- TrackedFileStruct file = createTrackedFileWithStats();
+ TrackedFileStruct file =
Review Comment:
While it's duplicative compared to using the helper, at this level of abstraction we are going for testing explicit crud/copyh stuff, and so it's helpful to see what exactly is being set rather than having to go into the helper.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Cleanup some of the TrackedFile tests [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on PR #17035:
URL: https://github.com/apache/iceberg/pull/17035#issuecomment-4861268061
cc @anoopj @gaborkaszab
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Cleanup some of the TrackedFile tests [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #17035:
URL: https://github.com/apache/iceberg/pull/17035#discussion_r3532477545
##########
core/src/test/java/org/apache/iceberg/TestTrackedFileStruct.java:
##########
@@ -294,112 +243,52 @@ void testProjectedStructLike() {
}
@Test
- void testContentStatsReturnedWhenPresent() {
- TrackedFileStruct file = createTrackedFileWithStats();
- assertThat(file.contentStats()).isNotNull();
- assertThat(file.contentStats().fieldStats()).hasSize(2);
- }
-
- @Test
- void testContentStatsNullWhenNotSet() {
+ void structLikeSize() {
TrackedFileStruct file = new TrackedFileStruct();
- file.set(CONTENT_TYPE_ORDINAL, FileContent.DATA.id());
- file.set(LOCATION_ORDINAL, "test");
- file.set(FILE_FORMAT_ORDINAL, "parquet");
- file.set(RECORD_COUNT_ORDINAL, 0L);
- file.set(FILE_SIZE_IN_BYTES_ORDINAL, 0L);
- file.set(SPEC_ID_ORDINAL, 0);
-
- assertThat(file.contentStats()).isNull();
- }
-
- @Test
- void testAllFileContentTypesSupported() {
- for (FileContent content : FileContent.values()) {
- TrackedFileStruct file = new TrackedFileStruct();
- file.set(CONTENT_TYPE_ORDINAL, content.id());
- assertThat(file.contentType()).isEqualTo(content);
- }
+ assertThat(file.size()).isEqualTo(16);
}
- @Test
- void testJavaSerializationRoundTrip() throws IOException, ClassNotFoundException {
- TrackedFileStruct file = createFullTrackedFile();
-
- TrackedFileStruct deserialized = TestHelpers.roundTripSerialize(file);
+ @ParameterizedTest
+ @MethodSource("org.apache.iceberg.TestHelpers#serializers")
+ void serializationRoundTrip(RoundTripSerializer<TrackedFileStruct> serializer) throws Exception {
+ TrackedFileStruct file =
+ new TrackedFileStruct(
+ TRACKING,
+ FileContent.DATA,
+ FORMAT_VERSION_V4,
+ "s3://bucket/data/file.parquet",
+ FileFormat.PARQUET,
+ PARTITION,
+ 100L,
+ 1024L,
+ 7,
+ null,
+ 1,
+ DELETION_VECTOR,
+ MANIFEST_INFO,
+ ByteBuffer.wrap(new byte[] {1, 2, 3}),
+ ImmutableList.of(50L),
+ ImmutableList.of(1, 2, 3));
+
+ TrackedFileStruct deserialized = serializer.apply(file);
- assertThat(deserialized.contentType()).isEqualTo(FileContent.DATA);
- assertThat(deserialized.formatVersion()).isEqualTo(FORMAT_VERSION_V4);
- assertThat(deserialized.location()).isEqualTo("s3://bucket/data/file.parquet");
- assertThat(deserialized.fileFormat()).isEqualTo(FileFormat.PARQUET);
- assertThat(deserialized.recordCount()).isEqualTo(100L);
- assertThat(deserialized.fileSizeInBytes()).isEqualTo(1024L);
- assertThat(deserialized.specId()).isEqualTo(0);
- assertThat(deserialized.sortOrderId()).isEqualTo(1);
assertThat(deserialized.tracking().status()).isEqualTo(EntryStatus.ADDED);
assertThat(deserialized.tracking().snapshotId()).isEqualTo(42L);
- assertThat(deserialized.deletionVector().location()).isEqualTo("s3://bucket/dv.puffin");
- assertThat(deserialized.keyMetadata()).isEqualTo(ByteBuffer.wrap(new byte[] {1, 2, 3}));
- assertThat(deserialized.splitOffsets()).containsExactly(50L);
- assertThat(deserialized.tracking().manifestPos()).isEqualTo(3L);
- assertThat(deserialized.tracking().manifestLocation()).isEqualTo("s3://bucket/manifest.avro");
- assertThat(deserialized.partition()).isEqualTo(newPartition(7, "music"));
- }
-
- @Test
- void testKryoSerializationRoundTrip() throws IOException {
- TrackedFileStruct file = createFullTrackedFile();
-
- TrackedFileStruct deserialized = TestHelpers.KryoHelpers.roundTripSerialize(file);
-
assertThat(deserialized.contentType()).isEqualTo(FileContent.DATA);
assertThat(deserialized.formatVersion()).isEqualTo(FORMAT_VERSION_V4);
assertThat(deserialized.location()).isEqualTo("s3://bucket/data/file.parquet");
assertThat(deserialized.fileFormat()).isEqualTo(FileFormat.PARQUET);
+ assertThat(deserialized.partition()).isEqualTo(PARTITION);
Review Comment:
This may work right now, but we don't want to rely on `equals` here.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Cleanup some of the TrackedFile tests [iceberg]
Posted by "gaborkaszab (via GitHub)" <gi...@apache.org>.
gaborkaszab commented on code in PR #17035:
URL: https://github.com/apache/iceberg/pull/17035#discussion_r3518303711
##########
core/src/test/java/org/apache/iceberg/TestTrackedFileStruct.java:
##########
@@ -348,7 +537,36 @@ void testJavaSerializationRoundTrip() throws IOException, ClassNotFoundException
@Test
void testKryoSerializationRoundTrip() throws IOException {
- TrackedFileStruct file = createFullTrackedFile();
+ TrackingStruct tracking =
+ new TrackingStruct(
+ EntryStatus.ADDED,
+ 42L,
+ null,
+ null,
+ null,
+ null,
+ null,
+ null,
+ "s3://bucket/manifest.avro",
+ 3L);
+ TrackedFileStruct file =
+ new TrackedFileStruct(
+ tracking,
+ FileContent.DATA,
+ FORMAT_VERSION_V4,
+ "s3://bucket/data/file.parquet",
+ FileFormat.PARQUET,
+ newPartition(7, "music"),
+ 100L,
+ 1024L,
+ 7,
+ null,
+ 1,
+ DELETION_VECTOR,
+ null,
+ ByteBuffer.wrap(new byte[] {1, 2, 3}),
+ ImmutableList.of(50L),
+ null);
Review Comment:
Shouldn't we set all the fields for a serialization test? There are some null values passed to the constructor, maybe setting values for them makes sense to see they are the same after SerDe
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Cleanup some of the TrackedFile tests [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #17035:
URL: https://github.com/apache/iceberg/pull/17035#discussion_r3509741471
##########
core/src/test/java/org/apache/iceberg/TestTrackedFileStruct.java:
##########
@@ -256,23 +341,6 @@ void testStructLikeSize() {
assertThat(file.size()).isEqualTo(16);
}
- @Test
- void testStructLikeGetSet() {
Review Comment:
Not needed anymore since we have a single test for verifying set and a single test for verifying get
##########
core/src/test/java/org/apache/iceberg/TestTrackedFileStruct.java:
##########
@@ -36,83 +36,62 @@ class TestTrackedFileStruct {
Types.NestedField.optional(1000, "id_bucket", Types.IntegerType.get()),
Types.NestedField.optional(1001, "category", Types.StringType.get()));
- // Ordinals looked up from the TrackedFile schema so tests don't hard-code positions.
- private static final List<Types.NestedField> SCHEMA_FIELDS =
- TrackedFile.schemaWithContentStats(Types.StructType.of(), Types.StructType.of()).fields();
- private static final int TRACKING_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.TRACKING);
- private static final int CONTENT_TYPE_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.CONTENT_TYPE);
- private static final int FORMAT_VERSION_ORDINAL =
- SCHEMA_FIELDS.indexOf(TrackedFile.FORMAT_VERSION);
- private static final int LOCATION_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.LOCATION);
- private static final int FILE_FORMAT_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.FILE_FORMAT);
- private static final int RECORD_COUNT_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.RECORD_COUNT);
- private static final int FILE_SIZE_IN_BYTES_ORDINAL =
- SCHEMA_FIELDS.indexOf(TrackedFile.FILE_SIZE_IN_BYTES);
- private static final int SPEC_ID_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.SPEC_ID);
- private static final int PARTITION_ORDINAL = ordinalOf(TrackedFile.PARTITION_ID);
- private static final int SORT_ORDER_ID_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.SORT_ORDER_ID);
- private static final int DELETION_VECTOR_ORDINAL =
- SCHEMA_FIELDS.indexOf(TrackedFile.DELETION_VECTOR);
- private static final int MANIFEST_INFO_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.MANIFEST_INFO);
- private static final int KEY_METADATA_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.KEY_METADATA);
- private static final int SPLIT_OFFSETS_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.SPLIT_OFFSETS);
- private static final int EQUALITY_IDS_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.EQUALITY_IDS);
-
- // Ordinal of MetadataColumns.ROW_POSITION within TrackingStruct's BASE_TYPE,
- // which appends ROW_POSITION after the Tracking schema fields.
- private static final int MANIFEST_POS_ORDINAL = Tracking.schema().fields().size();
-
- private static int ordinalOf(int fieldId) {
- for (int i = 0; i < SCHEMA_FIELDS.size(); i++) {
- if (SCHEMA_FIELDS.get(i).fieldId() == fieldId) {
- return i;
- }
- }
- throw new IllegalArgumentException("Field not found in TrackedFile schema: " + fieldId);
- }
+ private static final DeletionVectorStruct DELETION_VECTOR =
+ DeletionVectorStruct.builder()
+ .location("s3://bucket/dv.puffin")
+ .offset(100L)
+ .sizeInBytes(50L)
+ .cardinality(5L)
+ .build();
+
+ private static final ManifestInfoStruct MANIFEST_INFO =
+ ManifestInfoStruct.builder()
+ .addedFilesCount(10)
+ .existingFilesCount(20)
+ .deletedFilesCount(3)
+ .replacedFilesCount(2)
+ .addedRowsCount(1000L)
+ .existingRowsCount(2000L)
+ .deletedRowsCount(300L)
+ .replacedRowsCount(200L)
+ .minSequenceNumber(5L)
+ .build();
+
+ private static final ContentStats CONTENT_STATS =
Review Comment:
This is still using the builder for some of the nested fields within TrackedFile but I think that's fine, we're largely concerned about the explicitness of TrackedFile tests in this change.
##########
core/src/test/java/org/apache/iceberg/TestTrackedFileStruct.java:
##########
@@ -41,22 +41,13 @@ class TestTrackedFileStruct {
TrackedFile.schemaWithContentStats(Types.StructType.of(), Types.StructType.of()).fields();
private static final int TRACKING_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.TRACKING);
private static final int CONTENT_TYPE_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.CONTENT_TYPE);
- private static final int FORMAT_VERSION_ORDINAL =
- SCHEMA_FIELDS.indexOf(TrackedFile.FORMAT_VERSION);
private static final int LOCATION_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.LOCATION);
private static final int FILE_FORMAT_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.FILE_FORMAT);
private static final int RECORD_COUNT_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.RECORD_COUNT);
private static final int FILE_SIZE_IN_BYTES_ORDINAL =
SCHEMA_FIELDS.indexOf(TrackedFile.FILE_SIZE_IN_BYTES);
private static final int SPEC_ID_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.SPEC_ID);
private static final int PARTITION_ORDINAL = ordinalOf(TrackedFile.PARTITION_ID);
- private static final int SORT_ORDER_ID_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.SORT_ORDER_ID);
- private static final int DELETION_VECTOR_ORDINAL =
- SCHEMA_FIELDS.indexOf(TrackedFile.DELETION_VECTOR);
- private static final int MANIFEST_INFO_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.MANIFEST_INFO);
- private static final int KEY_METADATA_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.KEY_METADATA);
- private static final int SPLIT_OFFSETS_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.SPLIT_OFFSETS);
- private static final int EQUALITY_IDS_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.EQUALITY_IDS);
Review Comment:
Removing these ordinals because they're unused now.
##########
core/src/test/java/org/apache/iceberg/TestTrackedFileStruct.java:
##########
@@ -177,16 +264,14 @@ void partitionAccess() {
@Test
void partitionIsCopied() {
- PartitionData partition = newPartition(5, "books");
TrackedFileStruct file = createFullTrackedFile();
- file.set(PARTITION_ORDINAL, partition);
TrackedFile copy = file.copy();
- assertThat(copy.partition()).isNotNull().isNotSameAs(partition);
Review Comment:
I'm not sure I understand the reason for this assertion, doesn't the next one imply it?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Cleanup some of the TrackedFile tests [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #17035:
URL: https://github.com/apache/iceberg/pull/17035#discussion_r3509818112
##########
core/src/test/java/org/apache/iceberg/TestTrackedFileStruct.java:
##########
@@ -202,7 +311,7 @@ void testCopy() {
assertThat(copy.tracking().status()).isEqualTo(EntryStatus.ADDED);
assertThat(copy.tracking().snapshotId()).isEqualTo(42L);
assertThat(copy.deletionVector().location()).isEqualTo("s3://bucket/dv.puffin");
- assertThat(copy.specId()).isEqualTo(0);
+ assertThat(copy.specId()).isEqualTo(7);
Review Comment:
We build the initial file with spec 7. I feel like we should use non-zero/non-"default" values in these tests to make sure we're not just getting lucky on any internal state.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Cleanup some of the TrackedFile tests [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #17035:
URL: https://github.com/apache/iceberg/pull/17035#discussion_r3509713285
##########
core/src/test/java/org/apache/iceberg/TestTrackedFileStruct.java:
##########
@@ -121,16 +125,99 @@ void testFieldAccess() {
assertThat(file.fileSizeInBytes()).isEqualTo(512L);
assertThat(file.specId()).isEqualTo(1);
assertThat(file.sortOrderId()).isEqualTo(5);
- assertThat(file.deletionVector()).isSameAs(dv);
- assertThat(file.manifestInfo()).isSameAs(info);
+ assertThat(file.deletionVector()).isSameAs(DELETION_VECTOR);
+ assertThat(file.manifestInfo()).isSameAs(MANIFEST_INFO);
assertThat(file.keyMetadata()).isEqualTo(ByteBuffer.wrap(new byte[] {1, 2, 3}));
assertThat(file.splitOffsets()).containsExactly(100L, 200L);
assertThat(file.equalityIds()).containsExactly(1, 2, 3);
- // should return EMPTY_PARTITION_DATA
+ // partition defaults to EMPTY_PARTITION_DATA when not provided
assertThat(file.partition()).isNotNull();
assertThat(file.partition().size()).isEqualTo(0);
}
+ @Test
+ void testSetByPosition() {
+ Tracking tracking = TrackingBuilder.added(42L).build();
+ PartitionData partition = newPartition(7, "music");
+
+ TrackedFileStruct file = new TrackedFileStruct();
+ file.set(0, tracking);
+ file.set(1, FileContent.EQUALITY_DELETES.id());
Review Comment:
AI slop, dunno why it chose to use equality deletes for this...I'll fix it up to be a realistic example
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Cleanup some of the TrackedFile tests [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #17035:
URL: https://github.com/apache/iceberg/pull/17035#discussion_r3532435902
##########
core/src/test/java/org/apache/iceberg/TestTrackedFileStruct.java:
##########
@@ -36,254 +36,203 @@ class TestTrackedFileStruct {
Types.NestedField.optional(1000, "id_bucket", Types.IntegerType.get()),
Types.NestedField.optional(1001, "category", Types.StringType.get()));
- // Ordinals looked up from the TrackedFile schema so tests don't hard-code positions.
- private static final List<Types.NestedField> SCHEMA_FIELDS =
- TrackedFile.schemaWithContentStats(Types.StructType.of(), Types.StructType.of()).fields();
- private static final int TRACKING_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.TRACKING);
- private static final int CONTENT_TYPE_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.CONTENT_TYPE);
- private static final int FORMAT_VERSION_ORDINAL =
- SCHEMA_FIELDS.indexOf(TrackedFile.FORMAT_VERSION);
- private static final int LOCATION_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.LOCATION);
- private static final int FILE_FORMAT_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.FILE_FORMAT);
- private static final int RECORD_COUNT_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.RECORD_COUNT);
- private static final int FILE_SIZE_IN_BYTES_ORDINAL =
- SCHEMA_FIELDS.indexOf(TrackedFile.FILE_SIZE_IN_BYTES);
- private static final int SPEC_ID_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.SPEC_ID);
- private static final int PARTITION_ORDINAL = ordinalOf(TrackedFile.PARTITION_ID);
- private static final int SORT_ORDER_ID_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.SORT_ORDER_ID);
- private static final int DELETION_VECTOR_ORDINAL =
- SCHEMA_FIELDS.indexOf(TrackedFile.DELETION_VECTOR);
- private static final int MANIFEST_INFO_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.MANIFEST_INFO);
- private static final int KEY_METADATA_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.KEY_METADATA);
- private static final int SPLIT_OFFSETS_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.SPLIT_OFFSETS);
- private static final int EQUALITY_IDS_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.EQUALITY_IDS);
-
- // Ordinal of MetadataColumns.ROW_POSITION within TrackingStruct's BASE_TYPE,
- // which appends ROW_POSITION after the Tracking schema fields.
- private static final int MANIFEST_POS_ORDINAL = Tracking.schema().fields().size();
-
- private static int ordinalOf(int fieldId) {
- for (int i = 0; i < SCHEMA_FIELDS.size(); i++) {
- if (SCHEMA_FIELDS.get(i).fieldId() == fieldId) {
- return i;
- }
- }
- throw new IllegalArgumentException("Field not found in TrackedFile schema: " + fieldId);
- }
+ private static final List<Types.NestedField> FIELDS =
+ TrackedFile.schemaWithContentStats(PARTITION_TYPE, Types.StructType.of()).fields();
+
+ private static final Tracking TRACKING =
+ new TrackingStruct(EntryStatus.ADDED, 42L, 10L, 10L, 43L, 1000L, null, null);
+
+ private static final PartitionData PARTITION = newPartition(7, "music");
+
+ private static final DeletionVectorStruct DELETION_VECTOR =
+ DeletionVectorStruct.builder()
+ .location("s3://bucket/dv.puffin")
+ .offset(100L)
+ .sizeInBytes(50L)
+ .cardinality(5L)
+ .build();
+
+ private static final ManifestInfoStruct MANIFEST_INFO =
+ ManifestInfoStruct.builder()
+ .addedFilesCount(10)
+ .existingFilesCount(20)
+ .deletedFilesCount(3)
+ .replacedFilesCount(2)
+ .addedRowsCount(1000L)
+ .existingRowsCount(2000L)
+ .deletedRowsCount(300L)
+ .replacedRowsCount(200L)
+ .minSequenceNumber(5L)
+ .build();
@Test
- void testFieldAccess() {
- TrackedFileStruct file = new TrackedFileStruct();
- Tracking tracking = TrackingBuilder.added(42L).build();
- DeletionVectorStruct dv =
- DeletionVectorStruct.builder()
- .location("s3://bucket/dv.puffin")
- .offset(100L)
- .sizeInBytes(50L)
- .cardinality(5L)
- .build();
- ManifestInfoStruct info =
- ManifestInfoStruct.builder()
- .addedFilesCount(10)
- .existingFilesCount(20)
- .deletedFilesCount(3)
- .replacedFilesCount(2)
- .addedRowsCount(1000L)
- .existingRowsCount(2000L)
- .deletedRowsCount(300L)
- .replacedRowsCount(200L)
- .minSequenceNumber(5L)
- .build();
-
- file.set(TRACKING_ORDINAL, tracking);
- file.set(CONTENT_TYPE_ORDINAL, FileContent.EQUALITY_DELETES.id());
- file.set(FORMAT_VERSION_ORDINAL, FORMAT_VERSION_V4);
- file.set(LOCATION_ORDINAL, "s3://bucket/data/eq-delete.avro");
- file.set(FILE_FORMAT_ORDINAL, "avro");
- file.set(RECORD_COUNT_ORDINAL, 50L);
- file.set(FILE_SIZE_IN_BYTES_ORDINAL, 512L);
- file.set(SPEC_ID_ORDINAL, 1);
- file.set(SORT_ORDER_ID_ORDINAL, 5);
- file.set(DELETION_VECTOR_ORDINAL, dv);
- file.set(MANIFEST_INFO_ORDINAL, info);
- file.set(KEY_METADATA_ORDINAL, ByteBuffer.wrap(new byte[] {1, 2, 3}));
- file.set(SPLIT_OFFSETS_ORDINAL, ImmutableList.of(100L, 200L));
- file.set(EQUALITY_IDS_ORDINAL, ImmutableList.of(1, 2, 3));
-
- assertThat(file.tracking()).isNotNull();
- assertThat(file.tracking().status()).isEqualTo(EntryStatus.ADDED);
- assertThat(file.tracking().snapshotId()).isEqualTo(42L);
- assertThat(file.contentType()).isEqualTo(FileContent.EQUALITY_DELETES);
+ void fieldAccess() {
+ TrackedFileStruct file =
+ new TrackedFileStruct(
+ TRACKING,
+ FileContent.DATA,
+ FORMAT_VERSION_V4,
+ "s3://bucket/data/00000-0-file.parquet",
+ FileFormat.PARQUET,
+ PARTITION,
+ 50L,
+ 512L,
+ 1,
+ null,
+ 5,
+ DELETION_VECTOR,
+ MANIFEST_INFO,
+ ByteBuffer.wrap(new byte[] {1, 2, 3}),
+ ImmutableList.of(100L, 200L),
+ ImmutableList.of(1, 2, 3));
+
+ assertThat(file.tracking()).isSameAs(TRACKING);
+ assertThat(file.contentType()).isEqualTo(FileContent.DATA);
assertThat(file.formatVersion()).isEqualTo(FORMAT_VERSION_V4);
- assertThat(file.location()).isEqualTo("s3://bucket/data/eq-delete.avro");
- assertThat(file.fileFormat()).isEqualTo(FileFormat.AVRO);
+ assertThat(file.location()).isEqualTo("s3://bucket/data/00000-0-file.parquet");
+ assertThat(file.fileFormat()).isEqualTo(FileFormat.PARQUET);
+ assertThat(file.partition()).isSameAs(PARTITION);
assertThat(file.recordCount()).isEqualTo(50L);
assertThat(file.fileSizeInBytes()).isEqualTo(512L);
assertThat(file.specId()).isEqualTo(1);
assertThat(file.sortOrderId()).isEqualTo(5);
- assertThat(file.deletionVector()).isSameAs(dv);
- assertThat(file.manifestInfo()).isSameAs(info);
+ assertThat(file.deletionVector()).isSameAs(DELETION_VECTOR);
+ assertThat(file.manifestInfo()).isSameAs(MANIFEST_INFO);
assertThat(file.keyMetadata()).isEqualTo(ByteBuffer.wrap(new byte[] {1, 2, 3}));
assertThat(file.splitOffsets()).containsExactly(100L, 200L);
assertThat(file.equalityIds()).containsExactly(1, 2, 3);
- // should return EMPTY_PARTITION_DATA
- assertThat(file.partition()).isNotNull();
- assertThat(file.partition().size()).isEqualTo(0);
}
@Test
- void testReaderSideFields() {
Review Comment:
Moved to TestTrackingStruct
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Cleanup some of the TrackedFile tests [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #17035:
URL: https://github.com/apache/iceberg/pull/17035#discussion_r3532466458
##########
core/src/test/java/org/apache/iceberg/TestTrackedFileStruct.java:
##########
@@ -294,112 +243,52 @@ void testProjectedStructLike() {
}
@Test
- void testContentStatsReturnedWhenPresent() {
- TrackedFileStruct file = createTrackedFileWithStats();
- assertThat(file.contentStats()).isNotNull();
- assertThat(file.contentStats().fieldStats()).hasSize(2);
- }
-
- @Test
- void testContentStatsNullWhenNotSet() {
+ void structLikeSize() {
Review Comment:
On second look I don't really see the value in this test. Say we add or remove a top level field, we cause a needless failure in this class? I'm not sure what it's buying us. cc @rdblue thoughts on this ?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Cleanup some of the TrackedFile tests [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #17035:
URL: https://github.com/apache/iceberg/pull/17035#discussion_r3538325348
##########
core/src/test/java/org/apache/iceberg/TestTrackedFileStruct.java:
##########
@@ -294,112 +243,52 @@ void testProjectedStructLike() {
}
@Test
- void testContentStatsReturnedWhenPresent() {
- TrackedFileStruct file = createTrackedFileWithStats();
- assertThat(file.contentStats()).isNotNull();
- assertThat(file.contentStats().fieldStats()).hasSize(2);
- }
-
- @Test
- void testContentStatsNullWhenNotSet() {
+ void structLikeSize() {
TrackedFileStruct file = new TrackedFileStruct();
- file.set(CONTENT_TYPE_ORDINAL, FileContent.DATA.id());
- file.set(LOCATION_ORDINAL, "test");
- file.set(FILE_FORMAT_ORDINAL, "parquet");
- file.set(RECORD_COUNT_ORDINAL, 0L);
- file.set(FILE_SIZE_IN_BYTES_ORDINAL, 0L);
- file.set(SPEC_ID_ORDINAL, 0);
-
- assertThat(file.contentStats()).isNull();
- }
-
- @Test
- void testAllFileContentTypesSupported() {
- for (FileContent content : FileContent.values()) {
- TrackedFileStruct file = new TrackedFileStruct();
- file.set(CONTENT_TYPE_ORDINAL, content.id());
- assertThat(file.contentType()).isEqualTo(content);
- }
+ assertThat(file.size()).isEqualTo(16);
}
- @Test
- void testJavaSerializationRoundTrip() throws IOException, ClassNotFoundException {
- TrackedFileStruct file = createFullTrackedFile();
-
- TrackedFileStruct deserialized = TestHelpers.roundTripSerialize(file);
+ @ParameterizedTest
+ @MethodSource("org.apache.iceberg.TestHelpers#serializers")
+ void serializationRoundTrip(RoundTripSerializer<TrackedFileStruct> serializer) throws Exception {
+ TrackedFileStruct file =
+ new TrackedFileStruct(
+ TRACKING,
+ FileContent.DATA,
+ FORMAT_VERSION_V4,
+ "s3://bucket/data/file.parquet",
+ FileFormat.PARQUET,
+ PARTITION,
+ 100L,
+ 1024L,
+ 7,
+ null,
+ 1,
+ DELETION_VECTOR,
+ MANIFEST_INFO,
+ ByteBuffer.wrap(new byte[] {1, 2, 3}),
+ ImmutableList.of(50L),
+ ImmutableList.of(1, 2, 3));
+
+ TrackedFileStruct deserialized = serializer.apply(file);
- assertThat(deserialized.contentType()).isEqualTo(FileContent.DATA);
- assertThat(deserialized.formatVersion()).isEqualTo(FORMAT_VERSION_V4);
- assertThat(deserialized.location()).isEqualTo("s3://bucket/data/file.parquet");
- assertThat(deserialized.fileFormat()).isEqualTo(FileFormat.PARQUET);
- assertThat(deserialized.recordCount()).isEqualTo(100L);
- assertThat(deserialized.fileSizeInBytes()).isEqualTo(1024L);
- assertThat(deserialized.specId()).isEqualTo(0);
- assertThat(deserialized.sortOrderId()).isEqualTo(1);
assertThat(deserialized.tracking().status()).isEqualTo(EntryStatus.ADDED);
assertThat(deserialized.tracking().snapshotId()).isEqualTo(42L);
Review Comment:
no downside, in fact we arguably should just assert all the fields to make sure the whole structure round trips as expected. Using a struct like comparator for the comparison now.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Cleanup some of the TrackedFile tests [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #17035:
URL: https://github.com/apache/iceberg/pull/17035#discussion_r3509846072
##########
core/src/test/java/org/apache/iceberg/TestTrackedFileStruct.java:
##########
@@ -36,93 +36,72 @@ class TestTrackedFileStruct {
Types.NestedField.optional(1000, "id_bucket", Types.IntegerType.get()),
Types.NestedField.optional(1001, "category", Types.StringType.get()));
- // Ordinals looked up from the TrackedFile schema so tests don't hard-code positions.
- private static final List<Types.NestedField> SCHEMA_FIELDS =
- TrackedFile.schemaWithContentStats(Types.StructType.of(), Types.StructType.of()).fields();
- private static final int TRACKING_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.TRACKING);
- private static final int CONTENT_TYPE_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.CONTENT_TYPE);
- private static final int FORMAT_VERSION_ORDINAL =
- SCHEMA_FIELDS.indexOf(TrackedFile.FORMAT_VERSION);
- private static final int LOCATION_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.LOCATION);
- private static final int FILE_FORMAT_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.FILE_FORMAT);
- private static final int RECORD_COUNT_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.RECORD_COUNT);
- private static final int FILE_SIZE_IN_BYTES_ORDINAL =
- SCHEMA_FIELDS.indexOf(TrackedFile.FILE_SIZE_IN_BYTES);
- private static final int SPEC_ID_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.SPEC_ID);
- private static final int PARTITION_ORDINAL = ordinalOf(TrackedFile.PARTITION_ID);
- private static final int SORT_ORDER_ID_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.SORT_ORDER_ID);
- private static final int DELETION_VECTOR_ORDINAL =
- SCHEMA_FIELDS.indexOf(TrackedFile.DELETION_VECTOR);
- private static final int MANIFEST_INFO_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.MANIFEST_INFO);
- private static final int KEY_METADATA_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.KEY_METADATA);
- private static final int SPLIT_OFFSETS_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.SPLIT_OFFSETS);
- private static final int EQUALITY_IDS_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.EQUALITY_IDS);
-
- // Ordinal of MetadataColumns.ROW_POSITION within TrackingStruct's BASE_TYPE,
- // which appends ROW_POSITION after the Tracking schema fields.
- private static final int MANIFEST_POS_ORDINAL = Tracking.schema().fields().size();
-
- private static int ordinalOf(int fieldId) {
- for (int i = 0; i < SCHEMA_FIELDS.size(); i++) {
- if (SCHEMA_FIELDS.get(i).fieldId() == fieldId) {
- return i;
- }
- }
- throw new IllegalArgumentException("Field not found in TrackedFile schema: " + fieldId);
- }
+ private static final DeletionVectorStruct DELETION_VECTOR =
Review Comment:
commented below: This is still using the builder for some of the nested fields within TrackedFile but I think that's fine, we're largely concerned about the explicitness of TrackedFile tests in this change? So I think it's OK to reuse the builder abstractions for those nested structures.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Cleanup some of the TrackedFile tests [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #17035:
URL: https://github.com/apache/iceberg/pull/17035#discussion_r3509805088
##########
core/src/test/java/org/apache/iceberg/TestTrackedFileStruct.java:
##########
@@ -121,16 +125,99 @@ void testFieldAccess() {
assertThat(file.fileSizeInBytes()).isEqualTo(512L);
assertThat(file.specId()).isEqualTo(1);
assertThat(file.sortOrderId()).isEqualTo(5);
- assertThat(file.deletionVector()).isSameAs(dv);
- assertThat(file.manifestInfo()).isSameAs(info);
+ assertThat(file.deletionVector()).isSameAs(DELETION_VECTOR);
+ assertThat(file.manifestInfo()).isSameAs(MANIFEST_INFO);
assertThat(file.keyMetadata()).isEqualTo(ByteBuffer.wrap(new byte[] {1, 2, 3}));
assertThat(file.splitOffsets()).containsExactly(100L, 200L);
assertThat(file.equalityIds()).containsExactly(1, 2, 3);
- // should return EMPTY_PARTITION_DATA
+ // partition defaults to EMPTY_PARTITION_DATA when not provided
assertThat(file.partition()).isNotNull();
assertThat(file.partition().size()).isEqualTo(0);
}
+ @Test
+ void testSetByPosition() {
+ Tracking tracking = TrackingBuilder.added(42L).build();
+ PartitionData partition = newPartition(7, "music");
+
+ TrackedFileStruct file = new TrackedFileStruct();
+ file.set(0, tracking);
+ file.set(1, FileContent.EQUALITY_DELETES.id());
Review Comment:
Well we're not going to get 100% realistic if we have a single setter/getter test, just because we want to test all the fields . we could split across multiple tests but then it's a bit weird since we're testing outside this abstraction boundary. I'll just leave this as is so it's easier to review then.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Cleanup some of the TrackedFile tests [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on PR #17035:
URL: https://github.com/apache/iceberg/pull/17035#issuecomment-4908197136
Merged! Thanks, @amogh-jahagirdar!
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Cleanup some of the TrackedFile tests [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue merged PR #17035:
URL: https://github.com/apache/iceberg/pull/17035
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Cleanup some of the TrackedFile tests [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #17035:
URL: https://github.com/apache/iceberg/pull/17035#discussion_r3538352668
##########
core/src/test/java/org/apache/iceberg/TestTrackedFileStruct.java:
##########
@@ -36,254 +36,203 @@ class TestTrackedFileStruct {
Types.NestedField.optional(1000, "id_bucket", Types.IntegerType.get()),
Types.NestedField.optional(1001, "category", Types.StringType.get()));
- // Ordinals looked up from the TrackedFile schema so tests don't hard-code positions.
- private static final List<Types.NestedField> SCHEMA_FIELDS =
- TrackedFile.schemaWithContentStats(Types.StructType.of(), Types.StructType.of()).fields();
- private static final int TRACKING_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.TRACKING);
- private static final int CONTENT_TYPE_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.CONTENT_TYPE);
- private static final int FORMAT_VERSION_ORDINAL =
- SCHEMA_FIELDS.indexOf(TrackedFile.FORMAT_VERSION);
- private static final int LOCATION_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.LOCATION);
- private static final int FILE_FORMAT_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.FILE_FORMAT);
- private static final int RECORD_COUNT_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.RECORD_COUNT);
- private static final int FILE_SIZE_IN_BYTES_ORDINAL =
- SCHEMA_FIELDS.indexOf(TrackedFile.FILE_SIZE_IN_BYTES);
- private static final int SPEC_ID_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.SPEC_ID);
- private static final int PARTITION_ORDINAL = ordinalOf(TrackedFile.PARTITION_ID);
- private static final int SORT_ORDER_ID_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.SORT_ORDER_ID);
- private static final int DELETION_VECTOR_ORDINAL =
- SCHEMA_FIELDS.indexOf(TrackedFile.DELETION_VECTOR);
- private static final int MANIFEST_INFO_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.MANIFEST_INFO);
- private static final int KEY_METADATA_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.KEY_METADATA);
- private static final int SPLIT_OFFSETS_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.SPLIT_OFFSETS);
- private static final int EQUALITY_IDS_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.EQUALITY_IDS);
-
- // Ordinal of MetadataColumns.ROW_POSITION within TrackingStruct's BASE_TYPE,
- // which appends ROW_POSITION after the Tracking schema fields.
- private static final int MANIFEST_POS_ORDINAL = Tracking.schema().fields().size();
-
- private static int ordinalOf(int fieldId) {
- for (int i = 0; i < SCHEMA_FIELDS.size(); i++) {
- if (SCHEMA_FIELDS.get(i).fieldId() == fieldId) {
- return i;
- }
- }
- throw new IllegalArgumentException("Field not found in TrackedFile schema: " + fieldId);
- }
+ private static final List<Types.NestedField> FIELDS =
+ TrackedFile.schemaWithContentStats(PARTITION_TYPE, Types.StructType.of()).fields();
+
+ private static final Tracking TRACKING =
+ new TrackingStruct(EntryStatus.ADDED, 42L, 10L, 10L, 43L, 1000L, null, null);
+
+ private static final PartitionData PARTITION = newPartition(7, "music");
+
+ private static final DeletionVectorStruct DELETION_VECTOR =
+ DeletionVectorStruct.builder()
+ .location("s3://bucket/dv.puffin")
+ .offset(100L)
+ .sizeInBytes(50L)
+ .cardinality(5L)
+ .build();
+
+ private static final ManifestInfoStruct MANIFEST_INFO =
+ ManifestInfoStruct.builder()
+ .addedFilesCount(10)
+ .existingFilesCount(20)
+ .deletedFilesCount(3)
+ .replacedFilesCount(2)
+ .addedRowsCount(1000L)
+ .existingRowsCount(2000L)
+ .deletedRowsCount(300L)
+ .replacedRowsCount(200L)
+ .minSequenceNumber(5L)
+ .build();
@Test
- void testFieldAccess() {
- TrackedFileStruct file = new TrackedFileStruct();
- Tracking tracking = TrackingBuilder.added(42L).build();
- DeletionVectorStruct dv =
- DeletionVectorStruct.builder()
- .location("s3://bucket/dv.puffin")
- .offset(100L)
- .sizeInBytes(50L)
- .cardinality(5L)
- .build();
- ManifestInfoStruct info =
- ManifestInfoStruct.builder()
- .addedFilesCount(10)
- .existingFilesCount(20)
- .deletedFilesCount(3)
- .replacedFilesCount(2)
- .addedRowsCount(1000L)
- .existingRowsCount(2000L)
- .deletedRowsCount(300L)
- .replacedRowsCount(200L)
- .minSequenceNumber(5L)
- .build();
-
- file.set(TRACKING_ORDINAL, tracking);
- file.set(CONTENT_TYPE_ORDINAL, FileContent.EQUALITY_DELETES.id());
- file.set(FORMAT_VERSION_ORDINAL, FORMAT_VERSION_V4);
- file.set(LOCATION_ORDINAL, "s3://bucket/data/eq-delete.avro");
- file.set(FILE_FORMAT_ORDINAL, "avro");
- file.set(RECORD_COUNT_ORDINAL, 50L);
- file.set(FILE_SIZE_IN_BYTES_ORDINAL, 512L);
- file.set(SPEC_ID_ORDINAL, 1);
- file.set(SORT_ORDER_ID_ORDINAL, 5);
- file.set(DELETION_VECTOR_ORDINAL, dv);
- file.set(MANIFEST_INFO_ORDINAL, info);
- file.set(KEY_METADATA_ORDINAL, ByteBuffer.wrap(new byte[] {1, 2, 3}));
- file.set(SPLIT_OFFSETS_ORDINAL, ImmutableList.of(100L, 200L));
- file.set(EQUALITY_IDS_ORDINAL, ImmutableList.of(1, 2, 3));
-
- assertThat(file.tracking()).isNotNull();
- assertThat(file.tracking().status()).isEqualTo(EntryStatus.ADDED);
- assertThat(file.tracking().snapshotId()).isEqualTo(42L);
- assertThat(file.contentType()).isEqualTo(FileContent.EQUALITY_DELETES);
+ void fieldAccess() {
+ TrackedFileStruct file =
+ new TrackedFileStruct(
+ TRACKING,
+ FileContent.DATA,
+ FORMAT_VERSION_V4,
+ "s3://bucket/data/00000-0-file.parquet",
+ FileFormat.PARQUET,
+ PARTITION,
+ 50L,
+ 512L,
+ 1,
+ null,
+ 5,
+ DELETION_VECTOR,
+ MANIFEST_INFO,
+ ByteBuffer.wrap(new byte[] {1, 2, 3}),
+ ImmutableList.of(100L, 200L),
+ ImmutableList.of(1, 2, 3));
+
+ assertThat(file.tracking()).isSameAs(TRACKING);
+ assertThat(file.contentType()).isEqualTo(FileContent.DATA);
assertThat(file.formatVersion()).isEqualTo(FORMAT_VERSION_V4);
- assertThat(file.location()).isEqualTo("s3://bucket/data/eq-delete.avro");
- assertThat(file.fileFormat()).isEqualTo(FileFormat.AVRO);
+ assertThat(file.location()).isEqualTo("s3://bucket/data/00000-0-file.parquet");
+ assertThat(file.fileFormat()).isEqualTo(FileFormat.PARQUET);
+ assertThat(file.partition()).isSameAs(PARTITION);
assertThat(file.recordCount()).isEqualTo(50L);
assertThat(file.fileSizeInBytes()).isEqualTo(512L);
assertThat(file.specId()).isEqualTo(1);
assertThat(file.sortOrderId()).isEqualTo(5);
- assertThat(file.deletionVector()).isSameAs(dv);
- assertThat(file.manifestInfo()).isSameAs(info);
+ assertThat(file.deletionVector()).isSameAs(DELETION_VECTOR);
+ assertThat(file.manifestInfo()).isSameAs(MANIFEST_INFO);
assertThat(file.keyMetadata()).isEqualTo(ByteBuffer.wrap(new byte[] {1, 2, 3}));
assertThat(file.splitOffsets()).containsExactly(100L, 200L);
assertThat(file.equalityIds()).containsExactly(1, 2, 3);
- // should return EMPTY_PARTITION_DATA
- assertThat(file.partition()).isNotNull();
- assertThat(file.partition().size()).isEqualTo(0);
}
@Test
- void testReaderSideFields() {
+ void setByPosition() {
TrackedFileStruct file = new TrackedFileStruct();
-
- TrackingStruct tracking = new TrackingStruct();
- tracking.setManifestLocation("s3://bucket/metadata/manifest.avro");
- tracking.set(MANIFEST_POS_ORDINAL, 7L);
-
- file.set(TRACKING_ORDINAL, tracking);
- file.set(CONTENT_TYPE_ORDINAL, FileContent.DATA.id());
- file.set(LOCATION_ORDINAL, "test");
- file.set(FILE_FORMAT_ORDINAL, "parquet");
- file.set(RECORD_COUNT_ORDINAL, 0L);
- file.set(FILE_SIZE_IN_BYTES_ORDINAL, 0L);
-
- assertThat(file.tracking().manifestLocation()).isEqualTo("s3://bucket/metadata/manifest.avro");
- assertThat(file.tracking().manifestPos()).isEqualTo(7L);
- }
-
- @Test
- void projectionWithoutPartition() {
- // project only location (field ID 100) and file_size_in_bytes (field ID 104)
- Types.StructType projection =
- Types.StructType.of(TrackedFile.LOCATION, TrackedFile.FILE_SIZE_IN_BYTES);
-
- TrackedFileStruct file = new TrackedFileStruct(projection);
- assertThat(file.size()).isEqualTo(2);
- // should return EMPTY_PARTITION_DATA
- assertThat(file.partition()).isNotNull();
- assertThat(file.partition().size()).isEqualTo(0);
+ file.set(pos("tracking"), TRACKING);
+ file.set(pos("content_type"), FileContent.DATA.id());
+ file.set(pos("format_version"), FORMAT_VERSION_V4);
+ file.set(pos("location"), "s3://bucket/data/00000-0-file.parquet");
+ file.set(pos("file_format"), "parquet");
+ file.set(pos("record_count"), 50L);
+ file.set(pos("file_size_in_bytes"), 512L);
+ file.set(pos("spec_id"), 1);
+ file.set(pos("partition"), PARTITION);
+ file.set(pos("sort_order_id"), 5);
+ file.set(pos("deletion_vector"), DELETION_VECTOR);
+ file.set(pos("manifest_info"), MANIFEST_INFO);
+ file.set(pos("key_metadata"), ByteBuffer.wrap(new byte[] {1, 2, 3}));
+ file.set(pos("split_offsets"), ImmutableList.of(100L, 200L));
+ file.set(pos("equality_ids"), ImmutableList.of(1, 2, 3));
+
+ assertThat(file.tracking()).isSameAs(TRACKING);
+ assertThat(file.contentType()).isEqualTo(FileContent.DATA);
+ assertThat(file.formatVersion()).isEqualTo(FORMAT_VERSION_V4);
+ assertThat(file.location()).isEqualTo("s3://bucket/data/00000-0-file.parquet");
+ assertThat(file.fileFormat()).isEqualTo(FileFormat.PARQUET);
+ assertThat(file.recordCount()).isEqualTo(50L);
+ assertThat(file.fileSizeInBytes()).isEqualTo(512L);
+ assertThat(file.specId()).isEqualTo(1);
+ assertThat(file.partition()).isSameAs(PARTITION);
+ assertThat(file.sortOrderId()).isEqualTo(5);
+ assertThat(file.deletionVector()).isSameAs(DELETION_VECTOR);
+ assertThat(file.manifestInfo()).isSameAs(MANIFEST_INFO);
+ assertThat(file.keyMetadata()).isEqualTo(ByteBuffer.wrap(new byte[] {1, 2, 3}));
+ assertThat(file.splitOffsets()).containsExactly(100L, 200L);
+ assertThat(file.equalityIds()).containsExactly(1, 2, 3);
}
@Test
- void partitionAccess() {
- PartitionData partition = newPartition(5, "books");
-
- TrackedFileStruct file = new TrackedFileStruct();
- file.set(PARTITION_ORDINAL, partition);
-
- assertThat(file.partition()).isSameAs(partition);
- assertThat(file.partition().get(0, Integer.class)).isEqualTo(5);
- assertThat(file.partition().get(1, String.class)).isEqualTo("books");
+ void getByPosition() {
+ TrackedFileStruct file =
+ new TrackedFileStruct(
+ TRACKING,
+ FileContent.DATA,
+ FORMAT_VERSION_V4,
+ "s3://bucket/data/00000-0-file.parquet",
+ FileFormat.PARQUET,
+ PARTITION,
+ 50L,
+ 512L,
+ 1,
+ null,
+ 5,
+ DELETION_VECTOR,
+ MANIFEST_INFO,
+ ByteBuffer.wrap(new byte[] {1, 2, 3}),
+ ImmutableList.of(100L, 200L),
+ ImmutableList.of(1, 2, 3));
+
+ assertThat(file.get(pos("tracking"), Tracking.class)).isSameAs(TRACKING);
+ assertThat(file.get(pos("content_type"), Integer.class)).isEqualTo(FileContent.DATA.id());
+ assertThat(file.get(pos("format_version"), Integer.class)).isEqualTo(FORMAT_VERSION_V4);
+ assertThat(file.get(pos("location"), String.class))
+ .isEqualTo("s3://bucket/data/00000-0-file.parquet");
+ assertThat(file.get(pos("file_format"), String.class)).isEqualTo(FileFormat.PARQUET.toString());
+ assertThat(file.get(pos("record_count"), Long.class)).isEqualTo(50L);
+ assertThat(file.get(pos("file_size_in_bytes"), Long.class)).isEqualTo(512L);
+ assertThat(file.get(pos("spec_id"), Integer.class)).isEqualTo(1);
+ assertThat(file.get(pos("partition"), PartitionData.class)).isSameAs(PARTITION);
+ assertThat(file.get(pos("sort_order_id"), Integer.class)).isEqualTo(5);
+ assertThat(file.get(pos("deletion_vector"), DeletionVector.class)).isSameAs(DELETION_VECTOR);
+ assertThat(file.get(pos("manifest_info"), ManifestInfo.class)).isSameAs(MANIFEST_INFO);
+ assertThat(file.get(pos("key_metadata"), ByteBuffer.class))
+ .isEqualTo(ByteBuffer.wrap(new byte[] {1, 2, 3}));
+ assertThat(file.get(pos("split_offsets"), List.class)).containsExactly(100L, 200L);
+ assertThat(file.get(pos("equality_ids"), List.class)).containsExactly(1, 2, 3);
}
@Test
- void partitionIsCopied() {
- PartitionData partition = newPartition(5, "books");
- TrackedFileStruct file = createFullTrackedFile();
- file.set(PARTITION_ORDINAL, partition);
+ void copy() {
+ TrackedFileStruct file =
+ new TrackedFileStruct(
+ TRACKING,
+ FileContent.DATA,
+ FORMAT_VERSION_V4,
+ "s3://bucket/data/00000-0-file.parquet",
+ FileFormat.PARQUET,
+ PARTITION,
+ 50L,
+ 512L,
+ 1,
+ null,
+ 5,
+ DELETION_VECTOR,
+ MANIFEST_INFO,
+ ByteBuffer.wrap(new byte[] {1, 2, 3}),
+ ImmutableList.of(100L, 200L),
+ ImmutableList.of(1, 2, 3));
TrackedFile copy = file.copy();
- assertThat(copy.partition()).isNotNull().isNotSameAs(partition);
- assertThat(copy.partition()).isEqualTo(partition);
- assertThat(copy.partition().get(0, Integer.class)).isEqualTo(5);
- assertThat(copy.partition().get(1, String.class)).isEqualTo("books");
- }
-
- @Test
- void testCopy() {
- TrackedFileStruct file = createFullTrackedFile();
-
- TrackedFile copy = file.copy();
assertThat(copy).isInstanceOf(TrackedFileStruct.class);
-
- assertThat(copy.contentType()).isEqualTo(FileContent.DATA);
- assertThat(copy.location()).isEqualTo("s3://bucket/data/file.parquet");
- assertThat(copy.fileFormat()).isEqualTo(FileFormat.PARQUET);
assertThat(copy.tracking().status()).isEqualTo(EntryStatus.ADDED);
assertThat(copy.tracking().snapshotId()).isEqualTo(42L);
- assertThat(copy.deletionVector().location()).isEqualTo("s3://bucket/dv.puffin");
- assertThat(copy.specId()).isEqualTo(0);
- assertThat(copy.sortOrderId()).isEqualTo(1);
- assertThat(copy.recordCount()).isEqualTo(100L);
- assertThat(copy.fileSizeInBytes()).isEqualTo(1024L);
- assertThat(copy.formatVersion()).isEqualTo(FORMAT_VERSION_V4);
- assertThat(copy.keyMetadata()).isNotNull();
- assertThat(copy.splitOffsets()).containsExactly(50L);
- assertThat(copy.equalityIds()).isNull();
- assertThat(copy.tracking().manifestLocation()).isEqualTo("s3://bucket/manifest.avro");
- assertThat(copy.tracking().manifestPos()).isEqualTo(3L);
- assertThat(copy.partition()).isEqualTo(newPartition(7, "music"));
- }
-
- @Test
- void testCopyWithoutStats() {
- TrackedFileStruct file = createTrackedFileWithStats();
- assertThat(file.contentStats()).isNotNull();
-
- TrackedFile copy = file.copyWithoutStats();
-
assertThat(copy.contentType()).isEqualTo(FileContent.DATA);
- assertThat(copy.location()).isEqualTo("s3://bucket/data/file.parquet");
- assertThat(copy.contentStats()).isNull();
- }
-
- @Test
- void testCopyWithStatsFilters() {
- TrackedFileStruct file = createTrackedFileWithStats();
- Set<Integer> keepFieldIds = ImmutableSet.of(1);
-
- TrackedFile copy = file.copyWithStats(keepFieldIds);
-
- assertThat(copy.contentStats()).isNotNull();
- ContentStats stats = copy.contentStats();
- assertThat(stats.fieldStats()).hasSize(1);
- assertThat(stats.fieldStats().get(0).fieldId()).isEqualTo(1);
- }
-
- @Test
- void testCopyIsDeep() {
- TrackedFileStruct file = createFullTrackedFile();
-
- TrackedFile copy = file.copy();
+ assertThat(copy.formatVersion()).isEqualTo(FORMAT_VERSION_V4);
+ assertThat(copy.location()).isEqualTo("s3://bucket/data/00000-0-file.parquet");
+ assertThat(copy.fileFormat()).isEqualTo(FileFormat.PARQUET);
+ assertThat(copy.recordCount()).isEqualTo(50L);
+ assertThat(copy.fileSizeInBytes()).isEqualTo(512L);
+ assertThat(copy.specId()).isEqualTo(1);
+ assertThat(copy.sortOrderId()).isEqualTo(5);
+ assertThat(copy.deletionVector().location()).isEqualTo("s3://bucket/dv.puffin");
+ assertThat(copy.manifestInfo().addedFilesCount()).isEqualTo(10);
+ assertThat(copy.manifestInfo().addedRowsCount()).isEqualTo(1000L);
Review Comment:
No we actually already have one, miss on my part, I've went ahead and tightened up the assertions here to verify the deep copy on ManifestInfo.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Cleanup some of the TrackedFile tests [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #17035:
URL: https://github.com/apache/iceberg/pull/17035#discussion_r3538292354
##########
core/src/test/java/org/apache/iceberg/TestTrackedFileStruct.java:
##########
@@ -36,254 +36,203 @@ class TestTrackedFileStruct {
Types.NestedField.optional(1000, "id_bucket", Types.IntegerType.get()),
Types.NestedField.optional(1001, "category", Types.StringType.get()));
- // Ordinals looked up from the TrackedFile schema so tests don't hard-code positions.
- private static final List<Types.NestedField> SCHEMA_FIELDS =
- TrackedFile.schemaWithContentStats(Types.StructType.of(), Types.StructType.of()).fields();
- private static final int TRACKING_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.TRACKING);
- private static final int CONTENT_TYPE_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.CONTENT_TYPE);
- private static final int FORMAT_VERSION_ORDINAL =
- SCHEMA_FIELDS.indexOf(TrackedFile.FORMAT_VERSION);
- private static final int LOCATION_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.LOCATION);
- private static final int FILE_FORMAT_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.FILE_FORMAT);
- private static final int RECORD_COUNT_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.RECORD_COUNT);
- private static final int FILE_SIZE_IN_BYTES_ORDINAL =
- SCHEMA_FIELDS.indexOf(TrackedFile.FILE_SIZE_IN_BYTES);
- private static final int SPEC_ID_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.SPEC_ID);
- private static final int PARTITION_ORDINAL = ordinalOf(TrackedFile.PARTITION_ID);
- private static final int SORT_ORDER_ID_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.SORT_ORDER_ID);
- private static final int DELETION_VECTOR_ORDINAL =
- SCHEMA_FIELDS.indexOf(TrackedFile.DELETION_VECTOR);
- private static final int MANIFEST_INFO_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.MANIFEST_INFO);
- private static final int KEY_METADATA_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.KEY_METADATA);
- private static final int SPLIT_OFFSETS_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.SPLIT_OFFSETS);
- private static final int EQUALITY_IDS_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.EQUALITY_IDS);
-
- // Ordinal of MetadataColumns.ROW_POSITION within TrackingStruct's BASE_TYPE,
- // which appends ROW_POSITION after the Tracking schema fields.
- private static final int MANIFEST_POS_ORDINAL = Tracking.schema().fields().size();
-
- private static int ordinalOf(int fieldId) {
- for (int i = 0; i < SCHEMA_FIELDS.size(); i++) {
- if (SCHEMA_FIELDS.get(i).fieldId() == fieldId) {
- return i;
- }
- }
- throw new IllegalArgumentException("Field not found in TrackedFile schema: " + fieldId);
- }
+ private static final List<Types.NestedField> FIELDS =
+ TrackedFile.schemaWithContentStats(PARTITION_TYPE, Types.StructType.of()).fields();
+
+ private static final Tracking TRACKING =
+ new TrackingStruct(EntryStatus.ADDED, 42L, 10L, 10L, 43L, 1000L, null, null);
+
+ private static final PartitionData PARTITION = newPartition(7, "music");
+
+ private static final DeletionVectorStruct DELETION_VECTOR =
+ DeletionVectorStruct.builder()
+ .location("s3://bucket/dv.puffin")
+ .offset(100L)
+ .sizeInBytes(50L)
+ .cardinality(5L)
+ .build();
+
+ private static final ManifestInfoStruct MANIFEST_INFO =
+ ManifestInfoStruct.builder()
+ .addedFilesCount(10)
+ .existingFilesCount(20)
+ .deletedFilesCount(3)
+ .replacedFilesCount(2)
+ .addedRowsCount(1000L)
+ .existingRowsCount(2000L)
+ .deletedRowsCount(300L)
+ .replacedRowsCount(200L)
+ .minSequenceNumber(5L)
+ .build();
@Test
- void testFieldAccess() {
- TrackedFileStruct file = new TrackedFileStruct();
- Tracking tracking = TrackingBuilder.added(42L).build();
- DeletionVectorStruct dv =
- DeletionVectorStruct.builder()
- .location("s3://bucket/dv.puffin")
- .offset(100L)
- .sizeInBytes(50L)
- .cardinality(5L)
- .build();
- ManifestInfoStruct info =
- ManifestInfoStruct.builder()
- .addedFilesCount(10)
- .existingFilesCount(20)
- .deletedFilesCount(3)
- .replacedFilesCount(2)
- .addedRowsCount(1000L)
- .existingRowsCount(2000L)
- .deletedRowsCount(300L)
- .replacedRowsCount(200L)
- .minSequenceNumber(5L)
- .build();
-
- file.set(TRACKING_ORDINAL, tracking);
- file.set(CONTENT_TYPE_ORDINAL, FileContent.EQUALITY_DELETES.id());
- file.set(FORMAT_VERSION_ORDINAL, FORMAT_VERSION_V4);
- file.set(LOCATION_ORDINAL, "s3://bucket/data/eq-delete.avro");
- file.set(FILE_FORMAT_ORDINAL, "avro");
- file.set(RECORD_COUNT_ORDINAL, 50L);
- file.set(FILE_SIZE_IN_BYTES_ORDINAL, 512L);
- file.set(SPEC_ID_ORDINAL, 1);
- file.set(SORT_ORDER_ID_ORDINAL, 5);
- file.set(DELETION_VECTOR_ORDINAL, dv);
- file.set(MANIFEST_INFO_ORDINAL, info);
- file.set(KEY_METADATA_ORDINAL, ByteBuffer.wrap(new byte[] {1, 2, 3}));
- file.set(SPLIT_OFFSETS_ORDINAL, ImmutableList.of(100L, 200L));
- file.set(EQUALITY_IDS_ORDINAL, ImmutableList.of(1, 2, 3));
-
- assertThat(file.tracking()).isNotNull();
- assertThat(file.tracking().status()).isEqualTo(EntryStatus.ADDED);
- assertThat(file.tracking().snapshotId()).isEqualTo(42L);
- assertThat(file.contentType()).isEqualTo(FileContent.EQUALITY_DELETES);
+ void fieldAccess() {
+ TrackedFileStruct file =
+ new TrackedFileStruct(
+ TRACKING,
+ FileContent.DATA,
+ FORMAT_VERSION_V4,
+ "s3://bucket/data/00000-0-file.parquet",
+ FileFormat.PARQUET,
+ PARTITION,
+ 50L,
+ 512L,
+ 1,
+ null,
+ 5,
+ DELETION_VECTOR,
+ MANIFEST_INFO,
+ ByteBuffer.wrap(new byte[] {1, 2, 3}),
+ ImmutableList.of(100L, 200L),
+ ImmutableList.of(1, 2, 3));
+
+ assertThat(file.tracking()).isSameAs(TRACKING);
+ assertThat(file.contentType()).isEqualTo(FileContent.DATA);
assertThat(file.formatVersion()).isEqualTo(FORMAT_VERSION_V4);
- assertThat(file.location()).isEqualTo("s3://bucket/data/eq-delete.avro");
- assertThat(file.fileFormat()).isEqualTo(FileFormat.AVRO);
+ assertThat(file.location()).isEqualTo("s3://bucket/data/00000-0-file.parquet");
+ assertThat(file.fileFormat()).isEqualTo(FileFormat.PARQUET);
+ assertThat(file.partition()).isSameAs(PARTITION);
assertThat(file.recordCount()).isEqualTo(50L);
assertThat(file.fileSizeInBytes()).isEqualTo(512L);
assertThat(file.specId()).isEqualTo(1);
assertThat(file.sortOrderId()).isEqualTo(5);
- assertThat(file.deletionVector()).isSameAs(dv);
- assertThat(file.manifestInfo()).isSameAs(info);
+ assertThat(file.deletionVector()).isSameAs(DELETION_VECTOR);
+ assertThat(file.manifestInfo()).isSameAs(MANIFEST_INFO);
assertThat(file.keyMetadata()).isEqualTo(ByteBuffer.wrap(new byte[] {1, 2, 3}));
assertThat(file.splitOffsets()).containsExactly(100L, 200L);
assertThat(file.equalityIds()).containsExactly(1, 2, 3);
- // should return EMPTY_PARTITION_DATA
- assertThat(file.partition()).isNotNull();
- assertThat(file.partition().size()).isEqualTo(0);
}
@Test
- void testReaderSideFields() {
+ void setByPosition() {
TrackedFileStruct file = new TrackedFileStruct();
-
- TrackingStruct tracking = new TrackingStruct();
- tracking.setManifestLocation("s3://bucket/metadata/manifest.avro");
- tracking.set(MANIFEST_POS_ORDINAL, 7L);
-
- file.set(TRACKING_ORDINAL, tracking);
- file.set(CONTENT_TYPE_ORDINAL, FileContent.DATA.id());
- file.set(LOCATION_ORDINAL, "test");
- file.set(FILE_FORMAT_ORDINAL, "parquet");
- file.set(RECORD_COUNT_ORDINAL, 0L);
- file.set(FILE_SIZE_IN_BYTES_ORDINAL, 0L);
-
- assertThat(file.tracking().manifestLocation()).isEqualTo("s3://bucket/metadata/manifest.avro");
- assertThat(file.tracking().manifestPos()).isEqualTo(7L);
- }
-
- @Test
- void projectionWithoutPartition() {
- // project only location (field ID 100) and file_size_in_bytes (field ID 104)
- Types.StructType projection =
- Types.StructType.of(TrackedFile.LOCATION, TrackedFile.FILE_SIZE_IN_BYTES);
-
- TrackedFileStruct file = new TrackedFileStruct(projection);
- assertThat(file.size()).isEqualTo(2);
- // should return EMPTY_PARTITION_DATA
- assertThat(file.partition()).isNotNull();
- assertThat(file.partition().size()).isEqualTo(0);
+ file.set(pos("tracking"), TRACKING);
+ file.set(pos("content_type"), FileContent.DATA.id());
+ file.set(pos("format_version"), FORMAT_VERSION_V4);
+ file.set(pos("location"), "s3://bucket/data/00000-0-file.parquet");
+ file.set(pos("file_format"), "parquet");
+ file.set(pos("record_count"), 50L);
+ file.set(pos("file_size_in_bytes"), 512L);
+ file.set(pos("spec_id"), 1);
Review Comment:
ContentStats is being restructured, but we only wanted to remove the copyWithoutStats test and any of the other tests which depend on richer stats specific APIs. We still need to test the actual setter/getter of content stats which somehow got dropped! I added those back
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Cleanup some of the TrackedFile tests [iceberg]
Posted by "gaborkaszab (via GitHub)" <gi...@apache.org>.
gaborkaszab commented on code in PR #17035:
URL: https://github.com/apache/iceberg/pull/17035#discussion_r3518254095
##########
core/src/test/java/org/apache/iceberg/TestTrackedFileStruct.java:
##########
Review Comment:
The [other PR](https://github.com/apache/iceberg/pull/17000) that makes `partition` optional isn't merged, so currently we still return `EMPTY_PARTITION_DATA` here. Seems correct as of now, the linked PR will adjust this.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Cleanup some of the TrackedFile tests [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #17035:
URL: https://github.com/apache/iceberg/pull/17035#discussion_r3532464630
##########
core/src/test/java/org/apache/iceberg/TestTrackedFileStruct.java:
##########
@@ -36,254 +36,203 @@ class TestTrackedFileStruct {
Types.NestedField.optional(1000, "id_bucket", Types.IntegerType.get()),
Types.NestedField.optional(1001, "category", Types.StringType.get()));
- // Ordinals looked up from the TrackedFile schema so tests don't hard-code positions.
- private static final List<Types.NestedField> SCHEMA_FIELDS =
- TrackedFile.schemaWithContentStats(Types.StructType.of(), Types.StructType.of()).fields();
- private static final int TRACKING_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.TRACKING);
- private static final int CONTENT_TYPE_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.CONTENT_TYPE);
- private static final int FORMAT_VERSION_ORDINAL =
- SCHEMA_FIELDS.indexOf(TrackedFile.FORMAT_VERSION);
- private static final int LOCATION_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.LOCATION);
- private static final int FILE_FORMAT_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.FILE_FORMAT);
- private static final int RECORD_COUNT_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.RECORD_COUNT);
- private static final int FILE_SIZE_IN_BYTES_ORDINAL =
- SCHEMA_FIELDS.indexOf(TrackedFile.FILE_SIZE_IN_BYTES);
- private static final int SPEC_ID_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.SPEC_ID);
- private static final int PARTITION_ORDINAL = ordinalOf(TrackedFile.PARTITION_ID);
- private static final int SORT_ORDER_ID_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.SORT_ORDER_ID);
- private static final int DELETION_VECTOR_ORDINAL =
- SCHEMA_FIELDS.indexOf(TrackedFile.DELETION_VECTOR);
- private static final int MANIFEST_INFO_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.MANIFEST_INFO);
- private static final int KEY_METADATA_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.KEY_METADATA);
- private static final int SPLIT_OFFSETS_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.SPLIT_OFFSETS);
- private static final int EQUALITY_IDS_ORDINAL = SCHEMA_FIELDS.indexOf(TrackedFile.EQUALITY_IDS);
-
- // Ordinal of MetadataColumns.ROW_POSITION within TrackingStruct's BASE_TYPE,
- // which appends ROW_POSITION after the Tracking schema fields.
- private static final int MANIFEST_POS_ORDINAL = Tracking.schema().fields().size();
-
- private static int ordinalOf(int fieldId) {
- for (int i = 0; i < SCHEMA_FIELDS.size(); i++) {
- if (SCHEMA_FIELDS.get(i).fieldId() == fieldId) {
- return i;
- }
- }
- throw new IllegalArgumentException("Field not found in TrackedFile schema: " + fieldId);
- }
+ private static final List<Types.NestedField> FIELDS =
+ TrackedFile.schemaWithContentStats(PARTITION_TYPE, Types.StructType.of()).fields();
+
+ private static final Tracking TRACKING =
+ new TrackingStruct(EntryStatus.ADDED, 42L, 10L, 10L, 43L, 1000L, null, null);
+
+ private static final PartitionData PARTITION = newPartition(7, "music");
+
+ private static final DeletionVectorStruct DELETION_VECTOR =
+ DeletionVectorStruct.builder()
+ .location("s3://bucket/dv.puffin")
+ .offset(100L)
+ .sizeInBytes(50L)
+ .cardinality(5L)
+ .build();
+
+ private static final ManifestInfoStruct MANIFEST_INFO =
+ ManifestInfoStruct.builder()
+ .addedFilesCount(10)
+ .existingFilesCount(20)
+ .deletedFilesCount(3)
+ .replacedFilesCount(2)
+ .addedRowsCount(1000L)
+ .existingRowsCount(2000L)
+ .deletedRowsCount(300L)
+ .replacedRowsCount(200L)
+ .minSequenceNumber(5L)
+ .build();
@Test
- void testFieldAccess() {
- TrackedFileStruct file = new TrackedFileStruct();
- Tracking tracking = TrackingBuilder.added(42L).build();
- DeletionVectorStruct dv =
- DeletionVectorStruct.builder()
- .location("s3://bucket/dv.puffin")
- .offset(100L)
- .sizeInBytes(50L)
- .cardinality(5L)
- .build();
- ManifestInfoStruct info =
- ManifestInfoStruct.builder()
- .addedFilesCount(10)
- .existingFilesCount(20)
- .deletedFilesCount(3)
- .replacedFilesCount(2)
- .addedRowsCount(1000L)
- .existingRowsCount(2000L)
- .deletedRowsCount(300L)
- .replacedRowsCount(200L)
- .minSequenceNumber(5L)
- .build();
-
- file.set(TRACKING_ORDINAL, tracking);
- file.set(CONTENT_TYPE_ORDINAL, FileContent.EQUALITY_DELETES.id());
- file.set(FORMAT_VERSION_ORDINAL, FORMAT_VERSION_V4);
- file.set(LOCATION_ORDINAL, "s3://bucket/data/eq-delete.avro");
- file.set(FILE_FORMAT_ORDINAL, "avro");
- file.set(RECORD_COUNT_ORDINAL, 50L);
- file.set(FILE_SIZE_IN_BYTES_ORDINAL, 512L);
- file.set(SPEC_ID_ORDINAL, 1);
- file.set(SORT_ORDER_ID_ORDINAL, 5);
- file.set(DELETION_VECTOR_ORDINAL, dv);
- file.set(MANIFEST_INFO_ORDINAL, info);
- file.set(KEY_METADATA_ORDINAL, ByteBuffer.wrap(new byte[] {1, 2, 3}));
- file.set(SPLIT_OFFSETS_ORDINAL, ImmutableList.of(100L, 200L));
- file.set(EQUALITY_IDS_ORDINAL, ImmutableList.of(1, 2, 3));
-
- assertThat(file.tracking()).isNotNull();
- assertThat(file.tracking().status()).isEqualTo(EntryStatus.ADDED);
- assertThat(file.tracking().snapshotId()).isEqualTo(42L);
- assertThat(file.contentType()).isEqualTo(FileContent.EQUALITY_DELETES);
+ void fieldAccess() {
+ TrackedFileStruct file =
+ new TrackedFileStruct(
+ TRACKING,
+ FileContent.DATA,
+ FORMAT_VERSION_V4,
+ "s3://bucket/data/00000-0-file.parquet",
+ FileFormat.PARQUET,
+ PARTITION,
+ 50L,
+ 512L,
+ 1,
+ null,
+ 5,
+ DELETION_VECTOR,
+ MANIFEST_INFO,
+ ByteBuffer.wrap(new byte[] {1, 2, 3}),
+ ImmutableList.of(100L, 200L),
+ ImmutableList.of(1, 2, 3));
+
+ assertThat(file.tracking()).isSameAs(TRACKING);
+ assertThat(file.contentType()).isEqualTo(FileContent.DATA);
assertThat(file.formatVersion()).isEqualTo(FORMAT_VERSION_V4);
- assertThat(file.location()).isEqualTo("s3://bucket/data/eq-delete.avro");
- assertThat(file.fileFormat()).isEqualTo(FileFormat.AVRO);
+ assertThat(file.location()).isEqualTo("s3://bucket/data/00000-0-file.parquet");
+ assertThat(file.fileFormat()).isEqualTo(FileFormat.PARQUET);
+ assertThat(file.partition()).isSameAs(PARTITION);
assertThat(file.recordCount()).isEqualTo(50L);
assertThat(file.fileSizeInBytes()).isEqualTo(512L);
assertThat(file.specId()).isEqualTo(1);
assertThat(file.sortOrderId()).isEqualTo(5);
- assertThat(file.deletionVector()).isSameAs(dv);
- assertThat(file.manifestInfo()).isSameAs(info);
+ assertThat(file.deletionVector()).isSameAs(DELETION_VECTOR);
+ assertThat(file.manifestInfo()).isSameAs(MANIFEST_INFO);
assertThat(file.keyMetadata()).isEqualTo(ByteBuffer.wrap(new byte[] {1, 2, 3}));
assertThat(file.splitOffsets()).containsExactly(100L, 200L);
assertThat(file.equalityIds()).containsExactly(1, 2, 3);
- // should return EMPTY_PARTITION_DATA
- assertThat(file.partition()).isNotNull();
- assertThat(file.partition().size()).isEqualTo(0);
}
@Test
- void testReaderSideFields() {
+ void setByPosition() {
TrackedFileStruct file = new TrackedFileStruct();
-
- TrackingStruct tracking = new TrackingStruct();
- tracking.setManifestLocation("s3://bucket/metadata/manifest.avro");
- tracking.set(MANIFEST_POS_ORDINAL, 7L);
-
- file.set(TRACKING_ORDINAL, tracking);
- file.set(CONTENT_TYPE_ORDINAL, FileContent.DATA.id());
- file.set(LOCATION_ORDINAL, "test");
- file.set(FILE_FORMAT_ORDINAL, "parquet");
- file.set(RECORD_COUNT_ORDINAL, 0L);
- file.set(FILE_SIZE_IN_BYTES_ORDINAL, 0L);
-
- assertThat(file.tracking().manifestLocation()).isEqualTo("s3://bucket/metadata/manifest.avro");
- assertThat(file.tracking().manifestPos()).isEqualTo(7L);
- }
-
- @Test
- void projectionWithoutPartition() {
- // project only location (field ID 100) and file_size_in_bytes (field ID 104)
- Types.StructType projection =
- Types.StructType.of(TrackedFile.LOCATION, TrackedFile.FILE_SIZE_IN_BYTES);
-
- TrackedFileStruct file = new TrackedFileStruct(projection);
- assertThat(file.size()).isEqualTo(2);
- // should return EMPTY_PARTITION_DATA
- assertThat(file.partition()).isNotNull();
- assertThat(file.partition().size()).isEqualTo(0);
+ file.set(pos("tracking"), TRACKING);
+ file.set(pos("content_type"), FileContent.DATA.id());
+ file.set(pos("format_version"), FORMAT_VERSION_V4);
+ file.set(pos("location"), "s3://bucket/data/00000-0-file.parquet");
+ file.set(pos("file_format"), "parquet");
+ file.set(pos("record_count"), 50L);
+ file.set(pos("file_size_in_bytes"), 512L);
+ file.set(pos("spec_id"), 1);
+ file.set(pos("partition"), PARTITION);
+ file.set(pos("sort_order_id"), 5);
+ file.set(pos("deletion_vector"), DELETION_VECTOR);
+ file.set(pos("manifest_info"), MANIFEST_INFO);
+ file.set(pos("key_metadata"), ByteBuffer.wrap(new byte[] {1, 2, 3}));
+ file.set(pos("split_offsets"), ImmutableList.of(100L, 200L));
+ file.set(pos("equality_ids"), ImmutableList.of(1, 2, 3));
+
+ assertThat(file.tracking()).isSameAs(TRACKING);
+ assertThat(file.contentType()).isEqualTo(FileContent.DATA);
+ assertThat(file.formatVersion()).isEqualTo(FORMAT_VERSION_V4);
+ assertThat(file.location()).isEqualTo("s3://bucket/data/00000-0-file.parquet");
+ assertThat(file.fileFormat()).isEqualTo(FileFormat.PARQUET);
+ assertThat(file.recordCount()).isEqualTo(50L);
+ assertThat(file.fileSizeInBytes()).isEqualTo(512L);
+ assertThat(file.specId()).isEqualTo(1);
+ assertThat(file.partition()).isSameAs(PARTITION);
+ assertThat(file.sortOrderId()).isEqualTo(5);
+ assertThat(file.deletionVector()).isSameAs(DELETION_VECTOR);
+ assertThat(file.manifestInfo()).isSameAs(MANIFEST_INFO);
+ assertThat(file.keyMetadata()).isEqualTo(ByteBuffer.wrap(new byte[] {1, 2, 3}));
+ assertThat(file.splitOffsets()).containsExactly(100L, 200L);
+ assertThat(file.equalityIds()).containsExactly(1, 2, 3);
}
@Test
- void partitionAccess() {
- PartitionData partition = newPartition(5, "books");
-
- TrackedFileStruct file = new TrackedFileStruct();
- file.set(PARTITION_ORDINAL, partition);
-
- assertThat(file.partition()).isSameAs(partition);
- assertThat(file.partition().get(0, Integer.class)).isEqualTo(5);
- assertThat(file.partition().get(1, String.class)).isEqualTo("books");
+ void getByPosition() {
+ TrackedFileStruct file =
+ new TrackedFileStruct(
+ TRACKING,
+ FileContent.DATA,
+ FORMAT_VERSION_V4,
+ "s3://bucket/data/00000-0-file.parquet",
+ FileFormat.PARQUET,
+ PARTITION,
+ 50L,
+ 512L,
+ 1,
+ null,
+ 5,
+ DELETION_VECTOR,
+ MANIFEST_INFO,
+ ByteBuffer.wrap(new byte[] {1, 2, 3}),
+ ImmutableList.of(100L, 200L),
+ ImmutableList.of(1, 2, 3));
+
+ assertThat(file.get(pos("tracking"), Tracking.class)).isSameAs(TRACKING);
+ assertThat(file.get(pos("content_type"), Integer.class)).isEqualTo(FileContent.DATA.id());
+ assertThat(file.get(pos("format_version"), Integer.class)).isEqualTo(FORMAT_VERSION_V4);
+ assertThat(file.get(pos("location"), String.class))
+ .isEqualTo("s3://bucket/data/00000-0-file.parquet");
+ assertThat(file.get(pos("file_format"), String.class)).isEqualTo(FileFormat.PARQUET.toString());
+ assertThat(file.get(pos("record_count"), Long.class)).isEqualTo(50L);
+ assertThat(file.get(pos("file_size_in_bytes"), Long.class)).isEqualTo(512L);
+ assertThat(file.get(pos("spec_id"), Integer.class)).isEqualTo(1);
+ assertThat(file.get(pos("partition"), PartitionData.class)).isSameAs(PARTITION);
+ assertThat(file.get(pos("sort_order_id"), Integer.class)).isEqualTo(5);
+ assertThat(file.get(pos("deletion_vector"), DeletionVector.class)).isSameAs(DELETION_VECTOR);
+ assertThat(file.get(pos("manifest_info"), ManifestInfo.class)).isSameAs(MANIFEST_INFO);
+ assertThat(file.get(pos("key_metadata"), ByteBuffer.class))
+ .isEqualTo(ByteBuffer.wrap(new byte[] {1, 2, 3}));
+ assertThat(file.get(pos("split_offsets"), List.class)).containsExactly(100L, 200L);
+ assertThat(file.get(pos("equality_ids"), List.class)).containsExactly(1, 2, 3);
}
@Test
- void partitionIsCopied() {
- PartitionData partition = newPartition(5, "books");
- TrackedFileStruct file = createFullTrackedFile();
- file.set(PARTITION_ORDINAL, partition);
+ void copy() {
+ TrackedFileStruct file =
+ new TrackedFileStruct(
+ TRACKING,
+ FileContent.DATA,
+ FORMAT_VERSION_V4,
+ "s3://bucket/data/00000-0-file.parquet",
+ FileFormat.PARQUET,
+ PARTITION,
+ 50L,
+ 512L,
+ 1,
+ null,
+ 5,
+ DELETION_VECTOR,
+ MANIFEST_INFO,
+ ByteBuffer.wrap(new byte[] {1, 2, 3}),
+ ImmutableList.of(100L, 200L),
+ ImmutableList.of(1, 2, 3));
TrackedFile copy = file.copy();
- assertThat(copy.partition()).isNotNull().isNotSameAs(partition);
- assertThat(copy.partition()).isEqualTo(partition);
- assertThat(copy.partition().get(0, Integer.class)).isEqualTo(5);
- assertThat(copy.partition().get(1, String.class)).isEqualTo("books");
- }
-
- @Test
- void testCopy() {
- TrackedFileStruct file = createFullTrackedFile();
-
- TrackedFile copy = file.copy();
assertThat(copy).isInstanceOf(TrackedFileStruct.class);
-
- assertThat(copy.contentType()).isEqualTo(FileContent.DATA);
- assertThat(copy.location()).isEqualTo("s3://bucket/data/file.parquet");
- assertThat(copy.fileFormat()).isEqualTo(FileFormat.PARQUET);
assertThat(copy.tracking().status()).isEqualTo(EntryStatus.ADDED);
assertThat(copy.tracking().snapshotId()).isEqualTo(42L);
- assertThat(copy.deletionVector().location()).isEqualTo("s3://bucket/dv.puffin");
- assertThat(copy.specId()).isEqualTo(0);
- assertThat(copy.sortOrderId()).isEqualTo(1);
- assertThat(copy.recordCount()).isEqualTo(100L);
- assertThat(copy.fileSizeInBytes()).isEqualTo(1024L);
- assertThat(copy.formatVersion()).isEqualTo(FORMAT_VERSION_V4);
- assertThat(copy.keyMetadata()).isNotNull();
- assertThat(copy.splitOffsets()).containsExactly(50L);
- assertThat(copy.equalityIds()).isNull();
- assertThat(copy.tracking().manifestLocation()).isEqualTo("s3://bucket/manifest.avro");
- assertThat(copy.tracking().manifestPos()).isEqualTo(3L);
- assertThat(copy.partition()).isEqualTo(newPartition(7, "music"));
- }
-
- @Test
- void testCopyWithoutStats() {
- TrackedFileStruct file = createTrackedFileWithStats();
- assertThat(file.contentStats()).isNotNull();
-
- TrackedFile copy = file.copyWithoutStats();
-
assertThat(copy.contentType()).isEqualTo(FileContent.DATA);
- assertThat(copy.location()).isEqualTo("s3://bucket/data/file.parquet");
- assertThat(copy.contentStats()).isNull();
- }
-
- @Test
- void testCopyWithStatsFilters() {
- TrackedFileStruct file = createTrackedFileWithStats();
- Set<Integer> keepFieldIds = ImmutableSet.of(1);
-
- TrackedFile copy = file.copyWithStats(keepFieldIds);
-
- assertThat(copy.contentStats()).isNotNull();
- ContentStats stats = copy.contentStats();
- assertThat(stats.fieldStats()).hasSize(1);
- assertThat(stats.fieldStats().get(0).fieldId()).isEqualTo(1);
- }
-
- @Test
- void testCopyIsDeep() {
- TrackedFileStruct file = createFullTrackedFile();
-
- TrackedFile copy = file.copy();
+ assertThat(copy.formatVersion()).isEqualTo(FORMAT_VERSION_V4);
+ assertThat(copy.location()).isEqualTo("s3://bucket/data/00000-0-file.parquet");
+ assertThat(copy.fileFormat()).isEqualTo(FileFormat.PARQUET);
+ assertThat(copy.recordCount()).isEqualTo(50L);
+ assertThat(copy.fileSizeInBytes()).isEqualTo(512L);
+ assertThat(copy.specId()).isEqualTo(1);
+ assertThat(copy.sortOrderId()).isEqualTo(5);
+ assertThat(copy.deletionVector().location()).isEqualTo("s3://bucket/dv.puffin");
+ assertThat(copy.manifestInfo().addedFilesCount()).isEqualTo(10);
+ assertThat(copy.manifestInfo().addedRowsCount()).isEqualTo(1000L);
+ assertThat(copy.keyMetadata()).isEqualTo(ByteBuffer.wrap(new byte[] {1, 2, 3}));
+ assertThat(copy.splitOffsets()).containsExactly(100L, 200L);
+ assertThat(copy.equalityIds()).containsExactly(1, 2, 3);
- // keyMetadata should be a deep copy
+ assertThat(copy.partition()).isNotSameAs(file.partition()).isEqualTo(file.partition());
Review Comment:
Should we be similarly testing other fields that get deep copied? My guess is yes, but we don't have copy methods?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Cleanup some of the TrackedFile tests [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #17035:
URL: https://github.com/apache/iceberg/pull/17035#discussion_r3509805088
##########
core/src/test/java/org/apache/iceberg/TestTrackedFileStruct.java:
##########
@@ -121,16 +125,99 @@ void testFieldAccess() {
assertThat(file.fileSizeInBytes()).isEqualTo(512L);
assertThat(file.specId()).isEqualTo(1);
assertThat(file.sortOrderId()).isEqualTo(5);
- assertThat(file.deletionVector()).isSameAs(dv);
- assertThat(file.manifestInfo()).isSameAs(info);
+ assertThat(file.deletionVector()).isSameAs(DELETION_VECTOR);
+ assertThat(file.manifestInfo()).isSameAs(MANIFEST_INFO);
assertThat(file.keyMetadata()).isEqualTo(ByteBuffer.wrap(new byte[] {1, 2, 3}));
assertThat(file.splitOffsets()).containsExactly(100L, 200L);
assertThat(file.equalityIds()).containsExactly(1, 2, 3);
- // should return EMPTY_PARTITION_DATA
+ // partition defaults to EMPTY_PARTITION_DATA when not provided
assertThat(file.partition()).isNotNull();
assertThat(file.partition().size()).isEqualTo(0);
}
+ @Test
+ void testSetByPosition() {
+ Tracking tracking = TrackingBuilder.added(42L).build();
+ PartitionData partition = newPartition(7, "music");
+
+ TrackedFileStruct file = new TrackedFileStruct();
+ file.set(0, tracking);
+ file.set(1, FileContent.EQUALITY_DELETES.id());
Review Comment:
Well we're not going to get 100% realistic if we have a single setter/getter test, just because we want to test all the fields . we could split across multiple tests but then it's a bit weird since we're testing outside this abstraction boundary. We can at least get closer though by using data files. the manifest fields don't apply but that's a bit easier read then eq deletes and setting avro....It's also better to be a bit more realistic in the examples for any AI code later on because it won't throw off the model more.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Cleanup some of the TrackedFile tests [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #17035:
URL: https://github.com/apache/iceberg/pull/17035#discussion_r3509805088
##########
core/src/test/java/org/apache/iceberg/TestTrackedFileStruct.java:
##########
@@ -121,16 +125,99 @@ void testFieldAccess() {
assertThat(file.fileSizeInBytes()).isEqualTo(512L);
assertThat(file.specId()).isEqualTo(1);
assertThat(file.sortOrderId()).isEqualTo(5);
- assertThat(file.deletionVector()).isSameAs(dv);
- assertThat(file.manifestInfo()).isSameAs(info);
+ assertThat(file.deletionVector()).isSameAs(DELETION_VECTOR);
+ assertThat(file.manifestInfo()).isSameAs(MANIFEST_INFO);
assertThat(file.keyMetadata()).isEqualTo(ByteBuffer.wrap(new byte[] {1, 2, 3}));
assertThat(file.splitOffsets()).containsExactly(100L, 200L);
assertThat(file.equalityIds()).containsExactly(1, 2, 3);
- // should return EMPTY_PARTITION_DATA
+ // partition defaults to EMPTY_PARTITION_DATA when not provided
assertThat(file.partition()).isNotNull();
assertThat(file.partition().size()).isEqualTo(0);
}
+ @Test
+ void testSetByPosition() {
+ Tracking tracking = TrackingBuilder.added(42L).build();
+ PartitionData partition = newPartition(7, "music");
+
+ TrackedFileStruct file = new TrackedFileStruct();
+ file.set(0, tracking);
+ file.set(1, FileContent.EQUALITY_DELETES.id());
Review Comment:
Well we're not going to get 100% realistic if we have a single setter/getter test, just because we want to test all the fields . we could split across multiple tests but then it's a bit weird since we're testing outside this abstraction boundary.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Cleanup some of the TrackedFile tests [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #17035:
URL: https://github.com/apache/iceberg/pull/17035#discussion_r3509834370
##########
core/src/test/java/org/apache/iceberg/TestTrackedFileStruct.java:
##########
@@ -132,19 +111,105 @@ void testFieldAccess() {
}
@Test
- void testReaderSideFields() {
+ void testSetByPosition() {
+ Tracking tracking =
+ new TrackingStruct(EntryStatus.ADDED, 42L, null, null, null, null, null, null, null, -1L);
+ PartitionData partition = newPartition(7, "music");
+
TrackedFileStruct file = new TrackedFileStruct();
+ file.set(0, tracking);
+ file.set(1, FileContent.DATA.id());
+ file.set(2, FORMAT_VERSION_V4);
+ file.set(3, "s3://bucket/data/00000-0-file.parquet");
+ file.set(4, "parquet");
+ file.set(5, 50L);
+ file.set(6, 512L);
+ file.set(7, 1);
+ file.set(8, partition);
+ file.set(9, CONTENT_STATS);
+ file.set(10, 5);
+ file.set(11, DELETION_VECTOR);
+ file.set(12, MANIFEST_INFO);
+ file.set(13, ByteBuffer.wrap(new byte[] {1, 2, 3}));
+ file.set(14, ImmutableList.of(100L, 200L));
+ file.set(15, ImmutableList.of(1, 2, 3));
+
+ assertThat(file.tracking()).isSameAs(tracking);
+ assertThat(file.contentType()).isEqualTo(FileContent.DATA);
+ assertThat(file.formatVersion()).isEqualTo(FORMAT_VERSION_V4);
+ assertThat(file.location()).isEqualTo("s3://bucket/data/00000-0-file.parquet");
+ assertThat(file.fileFormat()).isEqualTo(FileFormat.PARQUET);
+ assertThat(file.recordCount()).isEqualTo(50L);
+ assertThat(file.fileSizeInBytes()).isEqualTo(512L);
+ assertThat(file.specId()).isEqualTo(1);
+ assertThat(file.partition()).isSameAs(partition);
+ assertThat(file.contentStats()).isSameAs(CONTENT_STATS);
+ assertThat(file.sortOrderId()).isEqualTo(5);
+ assertThat(file.deletionVector()).isSameAs(DELETION_VECTOR);
+ assertThat(file.manifestInfo()).isSameAs(MANIFEST_INFO);
+ assertThat(file.keyMetadata()).isEqualTo(ByteBuffer.wrap(new byte[] {1, 2, 3}));
+ assertThat(file.splitOffsets()).containsExactly(100L, 200L);
+ assertThat(file.equalityIds()).containsExactly(1, 2, 3);
+ }
+
+ @Test
+ void testGetByPosition() {
+ Tracking tracking =
+ new TrackingStruct(EntryStatus.ADDED, 42L, null, null, null, null, null, null, null, -1L);
+ PartitionData partition = newPartition(7, "music");
+ TrackedFileStruct file =
+ new TrackedFileStruct(
+ tracking,
+ FileContent.DATA,
+ FORMAT_VERSION_V4,
+ "s3://bucket/data/00000-0-file.parquet",
+ FileFormat.PARQUET,
+ partition,
+ 50L,
+ 512L,
+ 1,
+ CONTENT_STATS,
+ 5,
+ DELETION_VECTOR,
+ MANIFEST_INFO,
+ ByteBuffer.wrap(new byte[] {1, 2, 3}),
+ ImmutableList.of(100L, 200L),
+ ImmutableList.of(1, 2, 3));
+
+ assertThat(file.get(0, Tracking.class)).isSameAs(tracking);
+ assertThat(file.get(1, Integer.class)).isEqualTo(FileContent.DATA.id());
+ assertThat(file.get(2, Integer.class)).isEqualTo(FORMAT_VERSION_V4);
+ assertThat(file.get(3, String.class)).isEqualTo("s3://bucket/data/00000-0-file.parquet");
+ assertThat(file.get(4, String.class)).isEqualTo(FileFormat.PARQUET.toString());
+ assertThat(file.get(5, Long.class)).isEqualTo(50L);
+ assertThat(file.get(6, Long.class)).isEqualTo(512L);
+ assertThat(file.get(7, Integer.class)).isEqualTo(1);
+ assertThat(file.get(8, PartitionData.class)).isSameAs(partition);
+ assertThat(file.get(9, ContentStats.class)).isSameAs(CONTENT_STATS);
+ assertThat(file.get(10, Integer.class)).isEqualTo(5);
+ assertThat(file.get(11, DeletionVector.class)).isSameAs(DELETION_VECTOR);
+ assertThat(file.get(12, ManifestInfo.class)).isSameAs(MANIFEST_INFO);
+ assertThat(file.get(13, ByteBuffer.class)).isEqualTo(ByteBuffer.wrap(new byte[] {1, 2, 3}));
+ assertThat(file.get(14, List.class)).containsExactly(100L, 200L);
+ assertThat(file.get(15, List.class)).containsExactly(1, 2, 3);
+ }
+
+ @Test
+ void testReaderSideFields() {
+ // manifest_location and manifest_pos are populated by readers, not written to manifests.
+ // manifest_pos is ROW_POSITION, appended after the tracking schema fields (position 8).
Review Comment:
AI comment but I think it's useful context, it's not super obvious why we have this separate test without this comment.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Cleanup some of the TrackedFile tests [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #17035:
URL: https://github.com/apache/iceberg/pull/17035#discussion_r3539334766
##########
core/src/test/java/org/apache/iceberg/TestTrackedFileStruct.java:
##########
@@ -348,7 +537,36 @@ void testJavaSerializationRoundTrip() throws IOException, ClassNotFoundException
@Test
void testKryoSerializationRoundTrip() throws IOException {
- TrackedFileStruct file = createFullTrackedFile();
+ TrackingStruct tracking =
+ new TrackingStruct(
+ EntryStatus.ADDED,
+ 42L,
+ null,
+ null,
+ null,
+ null,
+ null,
+ null,
+ "s3://bucket/manifest.avro",
+ 3L);
+ TrackedFileStruct file =
+ new TrackedFileStruct(
+ tracking,
+ FileContent.DATA,
+ FORMAT_VERSION_V4,
+ "s3://bucket/data/file.parquet",
+ FileFormat.PARQUET,
+ newPartition(7, "music"),
+ 100L,
+ 1024L,
+ 7,
+ null,
+ 1,
+ DELETION_VECTOR,
+ null,
+ ByteBuffer.wrap(new byte[] {1, 2, 3}),
+ ImmutableList.of(50L),
+ null);
Review Comment:
In general, yes.
The exception is that I think it is okay to use `null` for struct fields, as long as those structs have their own serializability test.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Cleanup some of the TrackedFile tests [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #17035:
URL: https://github.com/apache/iceberg/pull/17035#discussion_r3539324436
##########
core/src/test/java/org/apache/iceberg/TestTrackedFileStruct.java:
##########
@@ -294,112 +243,52 @@ void testProjectedStructLike() {
}
@Test
- void testContentStatsReturnedWhenPresent() {
- TrackedFileStruct file = createTrackedFileWithStats();
- assertThat(file.contentStats()).isNotNull();
- assertThat(file.contentStats().fieldStats()).hasSize(2);
- }
-
- @Test
- void testContentStatsNullWhenNotSet() {
+ void structLikeSize() {
Review Comment:
I almost made a similar comment yesterday. I think there is little value, but I think the way this is written, it will start failing if you add a field to `TrackedFile.schemaWithContentStats` but not this class. That's a good thing so I think it's worth having this.
Also, we need to rename `TrackedFile.schemaWithContentStats` because it has a partition and content stats now. I'll follow up.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org