voonhous commented on code in PR #19869:
URL: https://github.com/apache/hudi/pull/19869#discussion_r3976033512
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/metadata/SecondaryIndexRecordGenerationUtils.java:
##########
@@ -245,35 +326,32 @@ public static <T> HoodieData<HoodieRecord>
readSecondaryKeysFromFileSlices(Hoodi
return engineContext.emptyHoodieData();
}
final int parallelism = Math.min(fileSlices.size(),
secondaryIndexMaxParallelism);
- final StoragePath basePath = metaClient.getBasePath();
- HoodieSchema tableSchema;
- try {
- tableSchema = new TableSchemaResolver(metaClient).getTableSchema();
- } catch (Exception e) {
- throw new HoodieException("Failed to get latest schema for " +
metaClient.getBasePath(), e);
- }
ReaderContextFactory<T> readerContextFactory =
engineContext.getReaderContextFactory(metaClient);
engineContext.setJobStatus(activeModule, "Secondary Index: reading
secondary keys from " + fileSlices.size() + " file slices");
HoodieFileFormat baseFileFormat =
metaClient.getTableConfig().getBaseFileFormat();
return engineContext.parallelize(fileSlices,
parallelism).flatMap(partitionAndBaseFile -> {
- final String partition = partitionAndBaseFile.getPartitionPath();
final FileSlice fileSlice = partitionAndBaseFile.getFileSlice();
- Option<StoragePath> dataFilePath =
Option.ofNullable(fileSlice.getBaseFile().map(baseFile ->
FSUtils.getAbsoluteFilePath(basePath, partition,
baseFile.getFileName())).orElseGet(null));
- HoodieSchema readerSchema;
- if (dataFilePath.isPresent()) {
- readerSchema = HoodieIOFactory.getIOFactory(metaClient.getStorage())
- .getFileFormatUtils(baseFileFormat)
- .readSchema(metaClient.getStorage(), dataFilePath.get());
- } else {
- readerSchema = tableSchema;
- }
+ // the storage path keeps the directory prefix of a file written outside
Hudi, which its file name alone loses
+ Option<StoragePath> dataFilePath =
fileSlice.getBaseFile().map(HoodieBaseFile::getStoragePath);
+ // a file slice without a base file has only log files, whose schema is
the table schema
+ HoodieSchema readerSchema = dataFilePath.isPresent()
+ ?
HoodieIOFactory.getIOFactory(metaClient.getStorage()).getFileFormatUtils(baseFileFormat).readSchema(metaClient.getStorage(),
dataFilePath.get())
+ : resolveTableSchema(metaClient);
Review Comment:
**minor (perf):** Not blocking. Resolving the schema inside the lambda
answers the laziness ask, but it now runs `new
TableSchemaResolver(metaClient).getTableSchema()` once per log-only file slice
on the executors, where b10d8167 ran it once on the driver; a MOR table with
many base-file-less slices pays that many timeline loads. Could we resolve it
once on the driver, guarded by `fileSlices.stream().anyMatch(s ->
!s.getFileSlice().getBaseFile().isPresent())`, and capture the result in the
closure?
##########
hudi-common/src/main/java/org/apache/hudi/common/util/ExternalFilePathUtil.java:
##########
@@ -143,7 +175,12 @@ private static String getOriginalFileName(String fileName)
{
*/
public static StoragePath getFullPathOfPartition(StoragePath parent, String
fileName) {
return getExternalFileGroupPrefix(fileName)
- .map(prefix -> new StoragePath(parent.toString().substring(0,
parent.toString().length() - prefix.length() - 1)))
+ .map(prefix -> {
+ String parentPath = parent.toString();
+ checkArgument(parentPath.endsWith(StoragePath.SEPARATOR + prefix),
+ "External file " + fileName + " carries the file group prefix "
+ prefix + " but its parent " + parentPath + " does not end with it");
Review Comment:
**minor (perf):** Not blocking. This `checkArgument` builds its message
eagerly on every call, and `getFullPathOfPartition` sits on the per-base-file
view-build path (`HoodieBaseFile:86`, `AbstractTableFileSystemView:180`), so
every prefixed external file pays the concatenation on each view build. Could
we use the `Supplier<String>` overload (`ValidationUtils:49`) so the message is
only built when the check fails?
```suggestion
checkArgument(parentPath.endsWith(StoragePath.SEPARATOR + prefix),
() -> "External file " + fileName + " carries the file group
prefix " + prefix + " but its parent " + parentPath + " does not end with it");
```
--
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]
