hudi-agent commented on code in PR #20333:
URL: https://github.com/apache/hudi/pull/20333#discussion_r4271845182
##########
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.getSourceBasePath() != null) {
+ fileToSourceBasePath.put(fileName, stat.getSourceBasePath());
Review Comment:
🤖 `HoodieWriteStat.sourceBasePath` is documented as the base path that
`path` is relative to, but `getFileList` treats the stored value as the file's
parent directory. So a writer that follows the write-stat javadoc
(`sourceBasePath=/data/hive_table`, `path=datestr=2022-06-15/f1.parquet`) would
resolve to `/data/hive_table/f1.parquet`. Could you settle on one meaning
before this gets persisted in the MDT? For example, store the source partition
directory here and name/document it that way on both sides.
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
hudi-common/src/main/java/org/apache/hudi/common/model/HoodieWriteStat.java:
##########
@@ -61,6 +61,14 @@ public class HoodieWriteStat extends HoodieReadStats {
@EqualsAndHashCode.Include
private String path;
+ /**
+ * Base path the file lives under, when the file is not under the table base
path. Set only for
+ * files registered into the table without being rewritten (REGISTER_ONLY
bootstrap); null
+ * otherwise, in which case {@link #path} is relative to the table base path
as usual.
+ */
+ @Nullable
+ private String sourceBasePath;
Review Comment:
🤖 non-blocking: `HoodieWriteStat` in `HoodieCommitMetadata.avsc` has no
`sourceBasePath`, and `convertCommitMetadataToAvro` goes through Jackson with
unknown properties ignored. So on v8+ tables this field is silently dropped
from the completed commit, while v6 JSON keeps it. Anything that re-reads write
stats from the timeline (incremental/CDC reads,
`WriteStatBasedIndexingCatchupTask`) would then look for the file under the
table base path; could you add the field to the commit metadata Avro schema too?
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
hudi-common/src/main/java/org/apache/hudi/metadata/HoodieMetadataPayload.java:
##########
@@ -323,7 +336,7 @@ public static HoodieRecord<HoodieMetadataPayload>
createPartitionFilesRecord(Str
// should not be creating empty files
checkState(fileSize > 0, () -> "File name " + fileName
+ ", is a 0 byte file. It does not have any contents");
- fileInfo.put(fileName, new HoodieMetadataFileInfo(fileSize, false));
+ fileInfo.put(fileName, new HoodieMetadataFileInfo(fileSize, false,
fileToSourceBasePath.get(fileName)));
Review Comment:
🤖 Once this is set, an older reader drops the field during Avro resolution
and looks for the file under the table partition. Would it make sense to gate
tables with registered files behind a table version or table feature in the
follow-up, so old readers fail fast instead of hitting missing files?
@nsivabalan in case you have a view on how this MDT change should be versioned.
<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 +518,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 base path it was
+ // registered with rather than against this table's partition path.
+ StoragePath basePath = e.getValue().getSourceBasePath() == null
Review Comment:
🤖 non-blocking: `BaseHoodieTableFileIndex` passes this output to `new
HoodieTableFileSystemView(metaClient, timeline, allFiles)`. Its
`addFilesToView(List)` derives the partition with
`FSUtils.getRelativePartitionPath(basePath, parent)`, which would throw "does
not belong to base-path" for an entry with a source path, so the Spark
file-index read path will need handling in the follow-up.
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
hudi-common/src/main/java/org/apache/hudi/metadata/HoodieTableMetadataUtil.java:
##########
@@ -2463,7 +2463,8 @@ public static Map<String, HoodieMetadataFileInfo>
combineFileSystemMetadata(Hood
return null;
}
return new HoodieMetadataFileInfo(
- Math.max(newFileInfo.getSize(), oldFileInfo.getSize()),
false);
+ Math.max(newFileInfo.getSize(), oldFileInfo.getSize()),
false,
+ newFileInfo.getSourceBasePath() != null ?
newFileInfo.getSourceBasePath() : oldFileInfo.getSourceBasePath());
Review Comment:
🤖 nit: could you pull the source-base-path fallback into a local variable
(or a small helper)? The inline ternary on a long line makes the merge lambda
harder to scan.
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
hudi-common/src/main/java/org/apache/hudi/metadata/MetadataPartitionType.java:
##########
@@ -303,7 +303,8 @@ private static void
constructFilesMetadataPayload(HoodieMetadataPayload payload,
payload.filesystemMetadata = metadata;
payload.filesystemMetadata.keySet().forEach(k -> {
GenericRecord v = payload.filesystemMetadata.get(k);
- payload.filesystemMetadata.put(k, new HoodieMetadataFileInfo((Long)
v.get("size"), (Boolean) v.get("isDeleted")));
+ payload.filesystemMetadata.put(k, new HoodieMetadataFileInfo((Long)
v.get("size"), (Boolean) v.get("isDeleted"),
+ v.get("sourceBasePath") == null ? null :
v.get("sourceBasePath").toString()));
Review Comment:
🤖 non-blocking: the description mentions a test that reads a record written
without `sourceBasePath`, but I don't see one in
`TestHoodieMetadataPayloadSourceBasePath`. `GenericData.Record.get(String)`
throws on Avro 1.11 when the field is absent, so a test that reads an
old-schema HFile/log record through `HoodieBackedTableMetadata` would lock in
that resolution always fills it in.
<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]
