hudi-agent commented on code in PR #20333:
URL: https://github.com/apache/hudi/pull/20333#discussion_r4274665294
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/metadata/index/files/FilesIndexer.java:
##########
@@ -189,6 +192,9 @@ private static List<HoodieRecord>
convertMetadataToFilesPartitionRecords(HoodieC
// of the sizes as reported after every write, since
file-sizes are
// monotonically increasing (ie file-size never goes down,
unless deleted)
updatedFilesToSizesMapping.merge(fileName,
stat.getFileSizeInBytes(), Math::max);
+ if (stat.getSourcePartitionPath() != null) {
+ fileToSourcePartitionPath.put(fileName,
stat.getSourcePartitionPath());
Review Comment:
🤖 non-blocking: the files map is keyed by name, so two stats in one
partition with the same file name but different source partition paths would
collapse into one entry here (for example Hive's `000000_0` from
`/src/dt=1/h=01` and `/src/dt=1/h=02`), and one file would silently disappear;
would it make sense to fail fast here, since the write path is the only place
this can still be rejected?
```java
String prev = fileToSourcePartitionPath.put(fileName,
stat.getSourcePartitionPath());
checkState(prev == null || prev.equals(stat.getSourcePartitionPath()), () ->
"File " + fileName + " registered from both " + prev + " and " +
stat.getSourcePartitionPath());
```
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
hudi-common/src/main/java/org/apache/hudi/metadata/HoodieMetadataPayload.java:
##########
@@ -505,7 +526,11 @@ public List<StoragePathInfo> getFileList(HoodieStorage
storage, StoragePath part
.map(e -> {
// NOTE: Since we know that the Metadata Table's Payload is simply a
file-name we're
// creating Hadoop's Path using more performant unsafe variant
- return new StoragePathInfo(new StoragePath(partitionPath,
e.getKey()), e.getValue().getSize(),
+ // A registered file lives outside the table, so it resolves against
the directory it was
+ // registered with rather than against this table's partition path.
+ String sourcePartitionPath = e.getValue().getSourcePartitionPath();
+ StoragePath parent = sourcePartitionPath == null ? partitionPath :
new StoragePath(sourcePartitionPath);
Review Comment:
🤖 Once the follow-up sets this field, I think MDT restore sync would drop
these files: `fetchOutofSyncFilesRecordsFromMetadataTable` marks as deleted
every FILES entry whose name isn't in a listing of `basePath/<partition>`, so a
restore to any instant after registration would silently remove every
registered file from the table. Rebuilding the MDT from a listing would never
find them either. Is the plan to skip entries that have a source partition path
in those paths, or to block restore and re-init on such tables? @nsivabalan
could you weigh in on where this should be handled?
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
hudi-common/src/main/java/org/apache/hudi/metadata/HoodieTableMetadataUtil.java:
##########
@@ -2462,8 +2462,23 @@ public static Map<String, HoodieMetadataFileInfo>
combineFileSystemMetadata(Hood
}
return null;
}
+ // Where a file lives never changes while it exists under the
same name, so the two
+ // records can only ever carry the same source partition path,
and taking whichever
+ // one is set is commutative. Two different paths means the name
was reused for a
+ // file elsewhere, which ordering cannot resolve either: pick
the same one whichever
+ // way the records arrive, so readers merging in different
orders at least agree.
+ String oldSourcePartitionPath =
oldFileInfo.getSourcePartitionPath();
+ String newSourcePartitionPath =
newFileInfo.getSourcePartitionPath();
+ String sourcePartitionPath =
Review Comment:
🤖 nit: would it make sense to move the source-partition-path resolution into
a small helper like `resolveSourcePartitionPath(key, oldFileInfo,
newFileInfo)`, so the merge lambda stays short and the five-line comment can
sit on the helper?
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
--
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]
