codope commented on code in PR #12105:
URL: https://github.com/apache/hudi/pull/12105#discussion_r1806388549
##########
hudi-common/src/main/java/org/apache/hudi/metadata/HoodieMetadataPayload.java:
##########
@@ -205,38 +205,40 @@ public HoodieMetadataPayload(Option<GenericRecord>
recordOpt) {
}
protected HoodieMetadataPayload(String key, int type, Map<String,
HoodieMetadataFileInfo> filesystemMetadata) {
- this(key, type, filesystemMetadata, null, null, null, null);
+ this(key, type, filesystemMetadata, null, null, null, null,
+ filesystemMetadata.isEmpty());
Review Comment:
I get the intention of setting `isDeletedRecord` based on
`filesystemMetadata.isEmpty()` check. But, is this going to work always? We
could be using this constructor for `ALL_PARTITIONS` key, and in case of
non-partitioned table, or when delete partitions, is the map empty? If so, then
we will be setting `isDeletedRecord` to true inadvertently. `isDeleted` for
filesystemMetadata is at the level of `HoodieMetadataFileInfo`, so do we need
the flag at the Map<String, HoodieMetadataFileInfo> collection? Even if such a
record is going to be returned by the metadata reader, the map is going to be
empty so we don't read any files.
I think `files` partition is very critical and it handles two different
record types (ALL_PARTIITONS and FILES). This change could have unintended side
effects. How about we leave this constructor as is and introduce new
constructor, which instantiates isDeletedRecord, only for all other metadata
payloads?
--
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]
