You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "rdblue (via GitHub)" <gi...@apache.org> on 2026/07/07 20:19:35 UTC
[PR] Core: Use mocks in TrackedFile tests [iceberg]
rdblue opened a new pull request, #17133:
URL: https://github.com/apache/iceberg/pull/17133
This updates `TestTrackedFileStruct` to use mock objects for plumbing tests rather than creating instances of `TrackingStruct` and `PartitionData`.
I also found that `null` is not currently supported as a partition when testing, so I removed where an empty partition data is injected. We've decided to use `null` when the partition spec is unpartitioned.
--
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: Use mocks in TrackedFile tests [iceberg]
Posted by "stevenzwu (via GitHub)" <gi...@apache.org>.
stevenzwu commented on code in PR #17133:
URL: https://github.com/apache/iceberg/pull/17133#discussion_r3547662305
##########
core/src/test/java/org/apache/iceberg/TestTrackedFileStruct.java:
##########
@@ -240,12 +231,9 @@ void copy() {
assertThat(copy.keyMetadata()).isEqualTo(ByteBuffer.wrap(new byte[] {1, 2, 3}));
assertThat(copy.splitOffsets()).containsExactly(100L, 200L);
assertThat(copy.equalityIds()).containsExactly(1, 2, 3);
-
- assertThat(PARTITION_COMPARATOR.compare(copy.partition(), PARTITION)).isEqualTo(0);
+ assertThat(copy.partition()).isSameAs(PARTITION_COPY);
// mutable fields are deep-copied, not shared with the original
- assertThat(copy.tracking()).isNotSameAs(file.tracking());
Review Comment:
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.
--
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: Use mocks in TrackedFile tests [iceberg]
Posted by "stevenzwu (via GitHub)" <gi...@apache.org>.
stevenzwu commented on code in PR #17133:
URL: https://github.com/apache/iceberg/pull/17133#discussion_r3541179813
##########
core/src/main/java/org/apache/iceberg/TrackedFileStruct.java:
##########
@@ -75,7 +68,7 @@ public PartitionData copy() {
private Tracking tracking = null;
private long recordCount = -1L;
private long fileSizeInBytes = -1L;
- private PartitionData partitionData = EMPTY_PARTITION_DATA;
+ private PartitionData partitionData = null;
Review Comment:
It is easy to maintain the contract of `DataFile` returns an empty partition instead of `null`. And there isn't much gain for breaking the contract. It is probably better to maintain the same behavior for compatibility.
I thought Amogh is going to update the spec PR that the partition tuple field is optional.
--
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: Use mocks in TrackedFile tests [iceberg]
Posted by "gaborkaszab (via GitHub)" <gi...@apache.org>.
gaborkaszab commented on code in PR #17133:
URL: https://github.com/apache/iceberg/pull/17133#discussion_r3543933740
##########
core/src/test/java/org/apache/iceberg/TestTrackedFileStruct.java:
##########
@@ -240,12 +231,9 @@ void copy() {
assertThat(copy.keyMetadata()).isEqualTo(ByteBuffer.wrap(new byte[] {1, 2, 3}));
assertThat(copy.splitOffsets()).containsExactly(100L, 200L);
assertThat(copy.equalityIds()).containsExactly(1, 2, 3);
-
- assertThat(PARTITION_COMPARATOR.compare(copy.partition(), PARTITION)).isEqualTo(0);
+ assertThat(copy.partition()).isSameAs(PARTITION_COPY);
// mutable fields are deep-copied, not shared with the original
- assertThat(copy.tracking()).isNotSameAs(file.tracking());
- assertThat(copy.partition()).isNotSameAs(file.partition());
assertThat(copy.deletionVector()).isNotSameAs(file.deletionVector());
Review Comment:
Would it make sense to introduce mocks for the below nested fields too? `Tracking` and `partition` doesn't seem any special from this aspect.
--
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: Use mocks in TrackedFile tests [iceberg]
Posted by "gaborkaszab (via GitHub)" <gi...@apache.org>.
gaborkaszab commented on code in PR #17133:
URL: https://github.com/apache/iceberg/pull/17133#discussion_r3551747307
##########
core/src/test/java/org/apache/iceberg/TestTrackedFileStruct.java:
##########
@@ -240,12 +231,9 @@ void copy() {
assertThat(copy.keyMetadata()).isEqualTo(ByteBuffer.wrap(new byte[] {1, 2, 3}));
assertThat(copy.splitOffsets()).containsExactly(100L, 200L);
assertThat(copy.equalityIds()).containsExactly(1, 2, 3);
-
- assertThat(PARTITION_COMPARATOR.compare(copy.partition(), PARTITION)).isEqualTo(0);
+ assertThat(copy.partition()).isSameAs(PARTITION_COPY);
// mutable fields are deep-copied, not shared with the original
- assertThat(copy.tracking()).isNotSameAs(file.tracking());
- assertThat(copy.partition()).isNotSameAs(file.partition());
assertThat(copy.deletionVector()).isNotSameAs(file.deletionVector());
Review Comment:
`DeletionVector` and `ManifestInfo` both have their `copy()` function. See [here](https://github.com/apache/iceberg/blob/main/core/src/main/java/org/apache/iceberg/DeletionVectorStruct.java#L83) and [here](https://github.com/apache/iceberg/blob/main/core/src/main/java/org/apache/iceberg/ManifestInfoStruct.java#L159).
`keyMetadata` is a byte array not a class, we won't be able to do so for this, but for the other 2 members we could.
--
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: Use mocks in TrackedFile tests [iceberg]
Posted by "stevenzwu (via GitHub)" <gi...@apache.org>.
stevenzwu commented on code in PR #17133:
URL: https://github.com/apache/iceberg/pull/17133#discussion_r3539962055
##########
core/src/main/java/org/apache/iceberg/TrackedFileStruct.java:
##########
@@ -75,7 +68,7 @@ public PartitionData copy() {
private Tracking tracking = null;
private long recordCount = -1L;
private long fileSizeInBytes = -1L;
- private PartitionData partitionData = EMPTY_PARTITION_DATA;
+ private PartitionData partitionData = null;
Review Comment:
Breaks `TestTrackedFileAdapters.testUnpartitionedFilePartitionIsEmpty` — `DataFile.partition()` needs to stay non-null for unpartitioned files. #17000 does the boundary translation (adds `PartitionData.EMPTY`, returns it from the adapter when the internal partition is null).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: 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: Use mocks in TrackedFile tests [iceberg]
Posted by "gaborkaszab (via GitHub)" <gi...@apache.org>.
gaborkaszab commented on code in PR #17133:
URL: https://github.com/apache/iceberg/pull/17133#discussion_r3543798307
##########
core/src/main/java/org/apache/iceberg/TrackedFileStruct.java:
##########
@@ -33,13 +33,6 @@
/** 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();
- private static final PartitionData EMPTY_PARTITION_DATA =
Review Comment:
The changes in this file would be gone if we merged https://github.com/apache/iceberg/pull/17000
--
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: Use mocks in TrackedFile tests [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #17133:
URL: https://github.com/apache/iceberg/pull/17133#discussion_r3573801470
##########
core/src/test/java/org/apache/iceberg/TestTrackedFileStruct.java:
##########
@@ -240,12 +231,9 @@ void copy() {
assertThat(copy.keyMetadata()).isEqualTo(ByteBuffer.wrap(new byte[] {1, 2, 3}));
assertThat(copy.splitOffsets()).containsExactly(100L, 200L);
assertThat(copy.equalityIds()).containsExactly(1, 2, 3);
-
- assertThat(PARTITION_COMPARATOR.compare(copy.partition(), PARTITION)).isEqualTo(0);
+ assertThat(copy.partition()).isSameAs(PARTITION_COPY);
// mutable fields are deep-copied, not shared with the original
- assertThat(copy.tracking()).isNotSameAs(file.tracking());
Review Comment:
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`.
--
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: Use mocks in TrackedFile tests [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #17133:
URL: https://github.com/apache/iceberg/pull/17133#discussion_r3545483612
##########
core/src/test/java/org/apache/iceberg/TestTrackedFileStruct.java:
##########
@@ -240,12 +231,9 @@ void copy() {
assertThat(copy.keyMetadata()).isEqualTo(ByteBuffer.wrap(new byte[] {1, 2, 3}));
assertThat(copy.splitOffsets()).containsExactly(100L, 200L);
assertThat(copy.equalityIds()).containsExactly(1, 2, 3);
-
- assertThat(PARTITION_COMPARATOR.compare(copy.partition(), PARTITION)).isEqualTo(0);
+ assertThat(copy.partition()).isSameAs(PARTITION_COPY);
// mutable fields are deep-copied, not shared with the original
- assertThat(copy.tracking()).isNotSameAs(file.tracking());
- assertThat(copy.partition()).isNotSameAs(file.partition());
assertThat(copy.deletionVector()).isNotSameAs(file.deletionVector());
Review Comment:
The issue with this is that we do not currently have `copy` methods for them. I think that we should follow up to do 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: Use mocks in TrackedFile tests [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #17133:
URL: https://github.com/apache/iceberg/pull/17133#discussion_r3546586172
##########
core/src/main/java/org/apache/iceberg/TrackedFileStruct.java:
##########
@@ -75,7 +68,7 @@ public PartitionData copy() {
private Tracking tracking = null;
private long recordCount = -1L;
private long fileSizeInBytes = -1L;
- private PartitionData partitionData = EMPTY_PARTITION_DATA;
+ private PartitionData partitionData = null;
Review Comment:
Rebased on top of #17000.
--
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: Use mocks in TrackedFile tests [iceberg]
Posted by "anoopj (via GitHub)" <gi...@apache.org>.
anoopj commented on code in PR #17133:
URL: https://github.com/apache/iceberg/pull/17133#discussion_r3540575441
##########
core/src/main/java/org/apache/iceberg/TrackedFileStruct.java:
##########
@@ -75,7 +68,7 @@ public PartitionData copy() {
private Tracking tracking = null;
private long recordCount = -1L;
private long fileSizeInBytes = -1L;
- private PartitionData partitionData = EMPTY_PARTITION_DATA;
+ private PartitionData partitionData = null;
Review Comment:
If we want to maintain the contract that `DataFile` continues to return empty partition instead of `null`, we can potentially handle it in the adapter. The open question is whether we want to continue to maintain that contract.
Also, we need to capture the null partition tuple for unpartitioned files either in the spec or the AMT proposal doc.
--
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: Use mocks in TrackedFile tests [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #17133:
URL: https://github.com/apache/iceberg/pull/17133#discussion_r3539411807
##########
core/src/main/java/org/apache/iceberg/TrackedFileStruct.java:
##########
@@ -33,13 +33,6 @@
/** 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();
Review Comment:
This struct is still used in the `BASE_TYPE` just below.
--
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: Use mocks in TrackedFile tests [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #17133:
URL: https://github.com/apache/iceberg/pull/17133#discussion_r3545476077
##########
core/src/main/java/org/apache/iceberg/TrackedFileStruct.java:
##########
@@ -33,13 +33,6 @@
/** 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();
- private static final PartitionData EMPTY_PARTITION_DATA =
Review Comment:
Merged #17000. Thanks, @gaborkaszab!
I'll rebase this PR.
--
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: Use mocks in TrackedFile tests [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on PR #17133:
URL: https://github.com/apache/iceberg/pull/17133#issuecomment-4963301276
Merged. Thanks for reviewing, @stevenzwu, @gaborkaszab, @anoopj, and @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: Use mocks in TrackedFile tests [iceberg]
Posted by "gaborkaszab (via GitHub)" <gi...@apache.org>.
gaborkaszab commented on code in PR #17133:
URL: https://github.com/apache/iceberg/pull/17133#discussion_r3543843408
##########
core/src/test/java/org/apache/iceberg/TestTrackedFileStruct.java:
##########
@@ -280,12 +268,12 @@ void structLikeSize() {
void serializationRoundTrip(RoundTripSerializer<TrackedFileStruct> serializer) throws Exception {
TrackedFileStruct file =
new TrackedFileStruct(
- TRACKING,
+ null, // TrackingStruct has its own serialization tests
Review Comment:
Technically these serialization test changes are irrelevant for the PR intent: "Use mocks in TrackedFile tests". Should we rephrase the PR title for this to fit more naturally?
--
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: Use mocks in TrackedFile tests [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #17133:
URL: https://github.com/apache/iceberg/pull/17133#discussion_r3573871931
##########
core/src/test/java/org/apache/iceberg/TestTrackedFileStruct.java:
##########
@@ -240,12 +231,9 @@ void copy() {
assertThat(copy.keyMetadata()).isEqualTo(ByteBuffer.wrap(new byte[] {1, 2, 3}));
assertThat(copy.splitOffsets()).containsExactly(100L, 200L);
assertThat(copy.equalityIds()).containsExactly(1, 2, 3);
-
- assertThat(PARTITION_COMPARATOR.compare(copy.partition(), PARTITION)).isEqualTo(0);
+ assertThat(copy.partition()).isSameAs(PARTITION_COPY);
// mutable fields are deep-copied, not shared with the original
- assertThat(copy.tracking()).isNotSameAs(file.tracking());
- assertThat(copy.partition()).isNotSameAs(file.partition());
assertThat(copy.deletionVector()).isNotSameAs(file.deletionVector());
Review Comment:
Updated! I didn't realize those interfaces had `copy` methods. Good catch.
--
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: Use mocks in TrackedFile tests [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue merged PR #17133:
URL: https://github.com/apache/iceberg/pull/17133
--
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: Use mocks in TrackedFile tests [iceberg]
Posted by "stevenzwu (via GitHub)" <gi...@apache.org>.
stevenzwu commented on code in PR #17133:
URL: https://github.com/apache/iceberg/pull/17133#discussion_r3547662305
##########
core/src/test/java/org/apache/iceberg/TestTrackedFileStruct.java:
##########
@@ -240,12 +231,9 @@ void copy() {
assertThat(copy.keyMetadata()).isEqualTo(ByteBuffer.wrap(new byte[] {1, 2, 3}));
assertThat(copy.splitOffsets()).containsExactly(100L, 200L);
assertThat(copy.equalityIds()).containsExactly(1, 2, 3);
-
- assertThat(PARTITION_COMPARATOR.compare(copy.partition(), PARTITION)).isEqualTo(0);
+ assertThat(copy.partition()).isSameAs(PARTITION_COPY);
// mutable fields are deep-copied, not shared with the original
- assertThat(copy.tracking()).isNotSameAs(file.tracking());
Review Comment:
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.
--
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: Use mocks in TrackedFile tests [iceberg]
Posted by "gaborkaszab (via GitHub)" <gi...@apache.org>.
gaborkaszab commented on code in PR #17133:
URL: https://github.com/apache/iceberg/pull/17133#discussion_r3543833539
##########
core/src/test/java/org/apache/iceberg/TestTrackedFileStruct.java:
##########
@@ -240,12 +231,9 @@ void copy() {
assertThat(copy.keyMetadata()).isEqualTo(ByteBuffer.wrap(new byte[] {1, 2, 3}));
assertThat(copy.splitOffsets()).containsExactly(100L, 200L);
assertThat(copy.equalityIds()).containsExactly(1, 2, 3);
-
- assertThat(PARTITION_COMPARATOR.compare(copy.partition(), PARTITION)).isEqualTo(0);
+ assertThat(copy.partition()).isSameAs(PARTITION_COPY);
Review Comment:
For the Partition copy we use `isSameAs` while for the Tracking copy we use `isEqualTo`. Use `isSameAs` for both?
##########
core/src/test/java/org/apache/iceberg/TestTrackedFileStruct.java:
##########
@@ -280,12 +268,12 @@ void structLikeSize() {
void serializationRoundTrip(RoundTripSerializer<TrackedFileStruct> serializer) throws Exception {
TrackedFileStruct file =
new TrackedFileStruct(
- TRACKING,
+ null, // TrackingStruct has its own serialization tests
Review Comment:
Technically these serialization test changes are irrelevant for the PR intent: "Use mocks in TrackedFile tests". Should we rephrase the PR title for this to fit more naturally?
--
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: Use mocks in TrackedFile tests [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #17133:
URL: https://github.com/apache/iceberg/pull/17133#discussion_r3573820340
##########
core/src/test/java/org/apache/iceberg/TestTrackedFileStruct.java:
##########
@@ -72,15 +72,7 @@ class TestTrackedFileStruct {
.minSequenceNumber(5L)
.build();
- private static final ContentStats CONTENT_STATS =
- BaseContentStats.builder()
- .withTableSchema(
- new Schema(
- Types.NestedField.optional(1, "id", Types.IntegerType.get()),
- Types.NestedField.optional(2, "data", Types.FloatType.get())))
- .withFieldStats(BaseFieldStats.builder().fieldId(1).build())
- .withFieldStats(BaseFieldStats.builder().fieldId(2).build())
- .build();
+ private static final ContentStats CONTENT_STATS = Mockito.mock(ContentStats.class);
Review Comment:
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.
--
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