rdblue commented on code in PR #17041:
URL: https://github.com/apache/iceberg/pull/17041#discussion_r3539606006
##########
core/src/main/java/org/apache/iceberg/TrackingStruct.java:
##########
@@ -104,6 +105,31 @@ private TrackingStruct(TrackingStruct toCopy) {
this.replacedPositions = replacedPositions;
}
+ @VisibleForTesting
+ TrackingStruct(
Review Comment:
I think @gaborkaszab is right that this needs to test that the
`TrackingStruct.manifestLocation()` method accesses the `manifestLocation`
field of this implementation. If we were to mock it or override it, then we
would be testing the test code and not the implementation.
That said, I'd prefer not to have this constructor. There is a setter for
`manifest_location` and `set` is the only way to set the `_pos` field. I would
be fine using those to set values and verify they are plumbed correctly. So the
accessor test would start with:
```java
TrackingStruct tracking =
new TrackingStruct(
EntryStatus.ADDED,
42L,
10L,
11L,
43L,
1000L,
DELETED_POSITIONS,
REPLACED_POSITIONS);
tracking.setManifestLocation("manifest-location");
tracking.set(ROW_POS_INDEX, 7L);
```
Hard-coding `ROW_POS_INDEX` (or whatever we decide to call it) is okay
because we can't look it up from `Tracking.schema()` and there is no setter.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
