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 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]
