gaborkaszab commented on code in PR #16964:
URL: https://github.com/apache/iceberg/pull/16964#discussion_r3476994071
##########
core/src/main/java/org/apache/iceberg/TrackedFileBuilder.java:
##########
@@ -23,19 +23,19 @@
import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
class TrackedFileBuilder {
- private final long snapshotId;
private final FileContent contentType;
+ private final TrackingBuilder trackingBuilder;
// Required fields
private Integer formatVersion = null;
private String location = null;
private FileFormat fileFormat = null;
private Long recordCount = null;
private Long fileSizeInBytes = null;
- private PartitionData partitionData = null;
// optional fields
private Integer specId = null;
+ private PartitionData partition = null;
Review Comment:
carry-over comment: "PartitionData should be StructLike. We don't want to
rely on concrete classes here."
`TrackedFileStruct` constructor seem to accept `PartitionData` so we have to
do the conversion at some point. Seemed straightforward to keep the same type
in the builder as expected by the constructed class.
##########
core/src/main/java/org/apache/iceberg/TrackedFileBuilder.java:
##########
@@ -205,15 +196,17 @@ TrackedFileBuilder fileSizeInBytes(long
newFileSizeInBytes) {
return this;
}
- TrackedFileBuilder specId(int newSpecId) {
- Preconditions.checkArgument(newSpecId >= 0, "Invalid spec ID: %s (must be
>= 0)", newSpecId);
- this.specId = newSpecId;
- return this;
- }
-
- TrackedFileBuilder partition(PartitionData newPartitionData) {
- Preconditions.checkArgument(newPartitionData != null, "Invalid partition:
null");
- this.partitionData = newPartitionData;
+ TrackedFileBuilder partition(PartitionSpec spec, StructLike newPartition) {
Review Comment:
carry-over comment: "I think that this should be combined with specId:
partition(int newSpecId, StructLike partitionTuple). If the tuple is set, then
spec ID is required so that we know how to interpret it. I suspect that this
should also be responsible for wrapping the partition tuple with a projection
to the union type."
carry-over answer: "I probably miss something here, but I see that
TrackedFile has a PartitionData member. If we want to produce that, is specId
and partitionTuple enough? Don't we need a PartitionSpec to produce
PartitionData?"
##########
core/src/test/java/org/apache/iceberg/TestTrackedFileAdapters.java:
##########
@@ -60,6 +60,8 @@ class TestTrackedFileAdapters {
private static final PartitionData PARTITION = partition("books");
// Tracking field ordinals, looked up from the schema so the tests do not
hard-code offsets.
+ private static final int STATUS_ORDINAL = ordinalOf(Tracking.schema(),
"status");
Review Comment:
I reverted the part of the test that constructs `Tracking` to the state
before the builder, because that seemed independent of this change. Hence these
show as appeared, but in fact restored
##########
core/src/main/java/org/apache/iceberg/TrackedFileBuilder.java:
##########
@@ -147,18 +139,17 @@ private static TrackedFile terminal(TrackedFile source,
Tracking tracking) {
private TrackedFileBuilder(FileContent contentType, long snapshotId) {
this.contentType = contentType;
- this.snapshotId = snapshotId;
+ this.trackingBuilder = TrackingBuilder.added(snapshotId);
}
private TrackedFileBuilder(TrackedFile source, long snapshotId) {
this.contentType = source.contentType();
- this.snapshotId = snapshotId;
this.formatVersion = source.formatVersion();
this.location = source.location();
this.fileFormat = source.fileFormat();
this.recordCount = source.recordCount();
this.fileSizeInBytes = source.fileSizeInBytes();
- this.partitionData = (PartitionData) source.partition();
+ this.partition = (PartitionData) source.partition();
Review Comment:
carry-over comment: "We should not need unchecked casts like this."
carry-over answer: "
The difficulty here is the TrackedFile.partition() return StructLike while
the TrackedFileStruct constructor accepts PartitionData. Since internally in
TrackedFile we also have PartitionData this seemed safe"
--
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]
