Re: [PR] [MINOR] Add optimisation for merged read handle with logical timestamp [hudi]
hudi-bot commented on PR #18478: URL: https://github.com/apache/hudi/pull/18478#issuecomment-4200949452 ## CI report: * 551a7a2df29199284f0663f80d82c9895486ee69 Azure: [SUCCESS](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=13126) Bot commands @hudi-bot supports the following commands: - `@hudi-bot run azure` re-run the last Azure build -- 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]
Re: [PR] [MINOR] Add optimisation for merged read handle with logical timestamp [hudi]
yihua commented on code in PR #18478:
URL: https://github.com/apache/hudi/pull/18478#discussion_r3046519819
##
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/io/HoodieMergedReadHandle.java:
##
@@ -56,26 +56,24 @@ public class HoodieMergedReadHandle extends
HoodieReadHandle fileSliceOpt;
- public HoodieMergedReadHandle(HoodieWriteConfig config,
-Option instantTime,
-HoodieTable hoodieTable,
-Pair partitionPathFileIDPair) {
-this(config, instantTime, hoodieTable, partitionPathFileIDPair,
Option.empty());
+ public HoodieMergedReadHandle(HoodieWriteConfig config, Option
instantTime,
+HoodieTable hoodieTable,
Pair partitionPathFileIDPair,
+Schema baseFileReaderSchema, boolean
hasTimestampFields) {
+this(config, instantTime, hoodieTable, partitionPathFileIDPair,
baseFileReaderSchema, hasTimestampFields, Option.empty());
}
- public HoodieMergedReadHandle(HoodieWriteConfig config,
-Option instantTime,
-HoodieTable hoodieTable,
-Pair partitionPathFileIDPair,
+ public HoodieMergedReadHandle(HoodieWriteConfig config, Option
instantTime,
+HoodieTable hoodieTable,
Pair partitionPathFileIDPair,
+Schema baseFileReaderSchema, boolean
hasTimestampFields,
Option fileSliceOption) {
super(config, instantTime, hoodieTable, partitionPathFileIDPair);
Schema orignalReaderSchema = HoodieAvroUtils.addMetadataFields(new
Schema.Parser().parse(config.getSchema()),
config.allowOperationMetadataField());
// config.getSchema is not canonicalized, while config.getWriteSchema is
canonicalized. So, we have to use the canonicalized schema to read the existing
data.
-baseFileReaderSchema = HoodieAvroUtils.addMetadataFields(new
Schema.Parser().parse(config.getWriteSchema()),
config.allowOperationMetadataField());
+this.baseFileReaderSchema = baseFileReaderSchema;
fileSliceOpt = fileSliceOption.isPresent() ? fileSliceOption :
getLatestFileSlice();
// Repair reader schema.
// Assume writer schema should be correct. If not, no repair happens.
-readerSchema = AvroSchemaUtils.getRepairedSchema(orignalReaderSchema,
baseFileReaderSchema);
+readerSchema = hasTimestampFields ?
AvroSchemaUtils.getRepairedSchema(orignalReaderSchema,
this.baseFileReaderSchema) : orignalReaderSchema;
Review Comment:
🤖 nit: the comment above this line should be updated to clarify that the
repair is now conditional — it only happens when `hasTimestampFields` is true,
rather than always repairing.
##
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/index/HoodieIndexUtils.java:
##
@@ -244,8 +245,10 @@ private static HoodieData>
getExistingRecords(
.filterCompletedInstants()
.lastInstant()
.map(HoodieInstant::getTimestamp);
+Schema baseFileReaderSchema = HoodieAvroUtils.addMetadataFields(new
Schema.Parser().parse(config.getWriteSchema()),
config.allowOperationMetadataField());
Review Comment:
🤖 nit: the schema setup pattern on these two lines is duplicated identically
in HoodieBackedTableMetadataWriter.java (lines 590–591) and
TestHoodieMergedReadHandle.java (lines 204–205) — could you extract this into a
static helper method to reduce duplication?
--
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]
Re: [PR] [MINOR] Add optimisation for merged read handle with logical timestamp [hudi]
yihua commented on code in PR #18478:
URL: https://github.com/apache/hudi/pull/18478#discussion_r3046544058
##
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/io/HoodieMergedReadHandle.java:
##
@@ -56,26 +56,24 @@ public class HoodieMergedReadHandle extends
HoodieReadHandle fileSliceOpt;
- public HoodieMergedReadHandle(HoodieWriteConfig config,
-Option instantTime,
-HoodieTable hoodieTable,
-Pair partitionPathFileIDPair) {
-this(config, instantTime, hoodieTable, partitionPathFileIDPair,
Option.empty());
+ public HoodieMergedReadHandle(HoodieWriteConfig config, Option
instantTime,
+HoodieTable hoodieTable,
Pair partitionPathFileIDPair,
+Schema baseFileReaderSchema, boolean
hasTimestampFields) {
+this(config, instantTime, hoodieTable, partitionPathFileIDPair,
baseFileReaderSchema, hasTimestampFields, Option.empty());
}
- public HoodieMergedReadHandle(HoodieWriteConfig config,
-Option instantTime,
-HoodieTable hoodieTable,
-Pair partitionPathFileIDPair,
+ public HoodieMergedReadHandle(HoodieWriteConfig config, Option
instantTime,
+HoodieTable hoodieTable,
Pair partitionPathFileIDPair,
+Schema baseFileReaderSchema, boolean
hasTimestampFields,
Option fileSliceOption) {
super(config, instantTime, hoodieTable, partitionPathFileIDPair);
Schema orignalReaderSchema = HoodieAvroUtils.addMetadataFields(new
Schema.Parser().parse(config.getSchema()),
config.allowOperationMetadataField());
// config.getSchema is not canonicalized, while config.getWriteSchema is
canonicalized. So, we have to use the canonicalized schema to read the existing
data.
-baseFileReaderSchema = HoodieAvroUtils.addMetadataFields(new
Schema.Parser().parse(config.getWriteSchema()),
config.allowOperationMetadataField());
+this.baseFileReaderSchema = baseFileReaderSchema;
fileSliceOpt = fileSliceOption.isPresent() ? fileSliceOption :
getLatestFileSlice();
// Repair reader schema.
// Assume writer schema should be correct. If not, no repair happens.
-readerSchema = AvroSchemaUtils.getRepairedSchema(orignalReaderSchema,
baseFileReaderSchema);
+readerSchema = hasTimestampFields ?
AvroSchemaUtils.getRepairedSchema(orignalReaderSchema,
this.baseFileReaderSchema) : orignalReaderSchema;
Review Comment:
🤖 The guard `hasTimestampFields` is checked via `hasTimestampMillisField`,
which returns true only for `TimestampMillis` and `LocalTimestampMillis`.
However, `needsLogicalTypeRepair` in `AvroSchemaRepair` also repairs the case
where the file schema is a plain LONG and the table schema has
`LocalTimestampMicros`. In that scenario, `hasTimestampMillisField` returns
false but the repair would still be needed. Could you verify this gap doesn't
affect any realistic schema canonicalization path, or broaden the check to
cover all timestamp logical types that `needsLogicalTypeRepair` handles?
##
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/metadata/HoodieBackedTableMetadataWriter.java:
##
@@ -584,8 +587,10 @@ private static HoodieData
readRecordKeysFromFileSliceSnapshot(Hood
final String partition = partitionAndFileSlice.getKey();
final FileSlice fileSlice = partitionAndFileSlice.getValue();
final String fileId = fileSlice.getFileId();
- return new HoodieMergedReadHandle(dataWriteConfig, instantTime,
hoodieTable, Pair.of(partition, fileSlice.getFileId()),
- Option.of(fileSlice)).getMergedRecords().stream().map(record -> {
+ Schema baseFileReaderSchema = HoodieAvroUtils.addMetadataFields(new
Schema.Parser().parse(dataWriteConfig.getWriteSchema()),
dataWriteConfig.allowOperationMetadataField());
Review Comment:
🤖 The schema parsing and `hasTimestampMillisField` computation are inside
the `flatMap` lambda, so they're re-evaluated for every partition/file-slice
pair. The PR description says this should be computed on the driver side. Could
you hoist `baseFileReaderSchema` and `hasTimestampFields` above the `flatMap`,
similar to how it's done in `HoodieIndexUtils.getExistingRecords`?
--
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]
