hudi-agent commented on code in PR #19560:
URL: https://github.com/apache/hudi/pull/19560#discussion_r3737940329
##########
hudi-common/src/main/java/org/apache/hudi/metadata/HoodieMetadataPayload.java:
##########
@@ -426,8 +432,11 @@ public Option<IndexedRecord> getInsertValue(Schema schema,
Properties properties
// TODO: feature(schema): Swap this over to HOODIE_METADATA_SCHEMA after
HoodieRecordPayload implementations are using HoodieSchema
// Uses cached Avro schema reference for O(1) equality check.
- if (schema == null || schema == HOODIE_METADATA_AVRO_SCHEMA) {
- // If the schema is same or none is provided, we can return the record
directly
+ if (schema == null || schema == HOODIE_METADATA_AVRO_SCHEMA ||
isHoodieMetadataRecordSchema(schema)) {
Review Comment:
🤖 `isHoodieMetadataRecordSchema` only checks the record name/namespace, so
it matches *any* schema named `HoodieMetadataRecord` — including one with the 5
Hudi meta columns prepended (`addMetadataFields` preserves the name) or a
projected subset. Those cases previously fell into the else-branch. Now they
take the fast path and get back the full 7-field record regardless of the
requested layout. Is that safe for every caller here (e.g. a
`populateMetaFields=true` MDT or a projected read), or could it hand back a
record that doesn't match the schema the caller asked for?
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
hudi-common/src/main/java/org/apache/hudi/metadata/HoodieMetadataPayload.java:
##########
@@ -390,6 +390,12 @@ public HoodieMetadataPayload
preCombine(HoodieMetadataPayload previousRecord) {
return
MetadataPartitionType.get(type).combineMetadataPayloads(previousRecord, this);
}
+ private static boolean isHoodieMetadataRecordSchema(Schema schema) {
Review Comment:
🤖 nit: `isHoodieMetadataRecordSchema` describes the structural check but not
the intent — could you rename it to something like
`isCompatibleMetadataRecordSchema` or `isOlderVersionMetadataRecordSchema` to
signal that this exists specifically to handle older schemas missing newer
fields?
<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]
