Core: Make partition field in TrackedFile optional#17000
Conversation
| FILE_SIZE_IN_BYTES, | ||
| SPEC_ID, | ||
| Types.NestedField.required(PARTITION_ID, PARTITION_NAME, partitionType, PARTITION_DOC), | ||
| Types.NestedField.optional(PARTITION_ID, PARTITION_NAME, partitionType, PARTITION_DOC), |
There was a problem hiding this comment.
Now that this field is optional, partition() can return null (for example a projection without the partition column, per TestTrackedFileStruct.projectionWithoutPartition). The partition() javadoc at line 145 still says only "Returns partition for this file as a StructLike", while the other optional getters in this interface all document "or null". Suggest noting the null case there, e.g. "or null if the partition is not present".
There was a problem hiding this comment.
the pattern seems to be simply writing ", or null.". I added that to the existing comment
gaborkaszab
force-pushed
the
main_trackedfile_partition_optional
branch
from
d6cf350 to
ebe1087
Compare
| /** Adapts {@link TrackedFile} entries to the {@link DataFile} and {@link DeleteFile} APIs. */ | ||
| class TrackedFileAdapters { | ||
|
|
||
| static final Types.StructType EMPTY_STRUCT_TYPE = Types.StructType.of(); |
There was a problem hiding this comment.
we can probably use the package private constants from the BaseFile class.
There was a problem hiding this comment.
You're right. I removed introducing it here and reuse the on in BaseFile
| @Override | ||
| public StructLike partition() { | ||
| return file().partition(); | ||
| return file().partition() != null ? file().partition() : EMPTY_PARTITION_DATA; |
There was a problem hiding this comment.
nit: We could use the MoreObjects.firstNonNull method. Feel free to disregard this comment since this method isn't widely used in the codebase.
There was a problem hiding this comment.
I just did a grep on the Java codebase, apparently, there is a single usage of this. I'd prefer to keep the ternary as is to follow the pattern around these files.
gaborkaszab
force-pushed
the
main_trackedfile_partition_optional
branch
from
ebe1087 to
21f8c80
Compare
gaborkaszab
left a comment
gaborkaszab
left a comment
There was a problem hiding this comment.
Thanks for the reviews, @stevenzwu , @ebyhr !
| // partition is rebuilt with the supplied struct types inside schemaWithContentStats, so its | ||
| // ordinal is looked up by field ID. | ||
| private static final int PARTITION_ORDINAL = ordinalOf(TrackedFile.PARTITION_ID); | ||
| private static final int CONTENT_STATS_ORDINAL = ordinalOf(TrackedFile.CONTENT_STATS_ID); |
There was a problem hiding this comment.
Note, this was unused, removed as part of this PR
| /** Adapts {@link TrackedFile} entries to the {@link DataFile} and {@link DeleteFile} APIs. */ | ||
| class TrackedFileAdapters { | ||
|
|
||
| static final Types.StructType EMPTY_STRUCT_TYPE = Types.StructType.of(); |
There was a problem hiding this comment.
You're right. I removed introducing it here and reuse the on in BaseFile
| // should return EMPTY_PARTITION_DATA | ||
| assertThat(file.partition()).isNotNull(); | ||
| assertThat(file.partition().size()).isEqualTo(0); | ||
| assertThat(file.partition()).isNull(); |
There was a problem hiding this comment.
curious and non-blocking, if the intention is projection without partition. Do we need a coverage for projection where we have a valid/nonnull partition but not included in the projection as well?
5 participants
No description provided.