Skip to content

Core: Use mocks in TrackedFile tests#17133

Merged
rdblue merged 4 commits into
apache:mainfrom
rdblue:update-tracked-file-tests
Jul 13, 2026
Merged

Core: Use mocks in TrackedFile tests#17133
rdblue merged 4 commits into
apache:mainfrom
rdblue:update-tracked-file-tests

Conversation

@rdblue

@rdblue rdblue commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

This updates TestTrackedFileStruct to use mock objects for plumbing tests rather than creating instances of TrackingStruct and PartitionData.

@rdblue rdblue requested a review from amogh-jahagirdar July 7, 2026 20:19
@github-actions github-actions Bot added the core label Jul 7, 2026

/** Mutable {@link StructLike} implementation of {@link TrackedFile}. */
class TrackedFileStruct extends SupportsIndexProjection implements TrackedFile, Serializable {
private static final Types.StructType EMPTY_STRUCT_TYPE = Types.StructType.of();

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This struct is still used in the BASE_TYPE just below.

Comment thread core/src/main/java/org/apache/iceberg/TrackedFileStruct.java
Comment thread core/src/main/java/org/apache/iceberg/TrackedFileStruct.java Outdated
Comment thread core/src/test/java/org/apache/iceberg/TestTrackedFileStruct.java
Comment thread core/src/test/java/org/apache/iceberg/TestTrackedFileStruct.java Outdated
@rdblue rdblue force-pushed the update-tracked-file-tests branch from 0f2d8c1 to 83d6309 Compare July 8, 2026 18:41

@stevenzwu stevenzwu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just one nit comment

assertThat(copy.partition()).isSameAs(PARTITION_COPY);

// mutable fields are deep-copied, not shared with the original
assertThat(copy.tracking()).isNotSameAs(file.tracking());

@stevenzwu stevenzwu Jul 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: it might make more sense to keep the two isSameAs assertions here to check they are deep copied and not shared with the original object, as the comment above described.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My rationale is that isSameAs(x) implies isNotSameAs(y) if x is not the same as y because the relation is transitive. By verifying isSameAs, this is already ensuring it is the result of copy.

We could go further and use this as a validation that copy produces a different object (it does not always) but that is the responsibility of the tests for TrackingStruct, not TrackedFileStruct.

.withFieldStats(BaseFieldStats.builder().fieldId(1).build())
.withFieldStats(BaseFieldStats.builder().fieldId(2).build())
.build();
private static final ContentStats CONTENT_STATS = Mockito.mock(ContentStats.class);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note this conflicts with #17159 (https://github.com/apache/iceberg/pull/17159/changes#diff-2e1fd2a7ed0bab2997b0ccc363eb220bc17cbe6e8db8ba6a199826091d8b4b09L75-R82). I'll rebase and fix this depending on which one makes it in first.

@rdblue rdblue merged commit 9484b57 into apache:main Jul 13, 2026
36 checks passed
@rdblue

rdblue commented Jul 13, 2026

Copy link
Copy Markdown
Contributor Author

Merged. Thanks for reviewing, @stevenzwu, @gaborkaszab, @anoopj, and @amogh-jahagirdar!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

5 participants