nsivabalan commented on code in PR #19205:
URL: https://github.com/apache/hudi/pull/19205#discussion_r3763338417
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/io/HoodieWriteMergeHandle.java:
##########
@@ -413,7 +418,16 @@ protected void writeToFile(HoodieKey key, HoodieRecord<T>
record, HoodieSchema s
if (shouldPreserveRecordMetadata) {
// NOTE: `FILENAME_METADATA_FIELD` has to be rewritten to correctly
point to the
// file holding this record even in cases when overall metadata is
preserved
- HoodieRecord populatedRecord = record.updateMetaField(schema,
HoodieRecord.FILENAME_META_FIELD_ORD, newFilePath.getName());
+ //
+ // The rewrite is gated on the mode: hoodie.meta.fields.mode is the
single authority on which
+ // meta columns hold values, and this path would otherwise populate
_hoodie_file_name on a
+ // COMMIT_TIME_ONLY / NONE table. The value written when the mode opts
out is an explicit null
+ // rather than a skipped update, because the record being preserved here
came from the previous
+ // base file — under a narrowed mode it can still carry a file name
written while the table was
+ // on ALL, and leaving that in place would carry a stale value forward.
+ String fileNameToWrite =
+ metaFieldsMode.isFileNamePopulated() ? newFilePath.getName() : null;
+ HoodieRecord populatedRecord = record.updateMetaField(schema,
HoodieRecord.FILENAME_META_FIELD_ORD, fileNameToWrite);
Review Comment:
Confirmed and fixed in `128933f5876a` — thank you, this was a real crash and
my tests could not have caught it.
`HoodieInternalRow#update` accepts only `UTF8String` or `String` and reports
anything else via `value.getClass()`, which NPEs on a null before the
`IllegalArgumentException` is built. So clearing the column threw for
`HoodieSparkRecord`, exactly as described.
Fixed in `HoodieSparkRecord#updateMetaField` rather than at the two call
sites, since it is the shared entry point both use: a null now routes through
`setNullAt`, which already handles the meta-field range correctly. Clearing a
meta column is a legitimate request rather than an error — a selective mode
leaves it unpopulated, and a record copied forward from a file written under
`ALL` needs the stale value cleared rather than carried over.
One correction to the diagnosis, which matters for the test: you attributed
the miss to the E2E tests using `SaveMode.Overwrite`. They do use `UPSERT` +
`Append`. The actual reason is the record type — those tests take the **Avro**
path, whose `data.put(ordinal, null)` tolerates a null. Reaching the Spark path
needs
`hoodie.write.record.merge.custom.implementation.classes=org.apache.hudi.DefaultSparkRecordMerger`;
`row.writer.enable` alone does not do it.
That distinction cost me a round: my first version of the regression test
passed **with the fix reverted**. The committed one is verified both ways —
`NullPointerException` without the fix, 29/29 with it.
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/io/BaseCreateHandle.java:
##########
@@ -164,8 +167,17 @@ record = record.prependMetaFields(schema,
writeSchemaWithMetaFields, new Metadat
}
protected HoodieRecord<T> updateFileName(HoodieRecord<T> record,
HoodieSchema schema, HoodieSchema targetSchema, String fileName, Properties
prop) {
- MetadataValues metadataValues = new MetadataValues().setFileName(fileName);
- return record.prependMetaFields(schema, targetSchema, metadataValues,
prop);
+ // hoodie.meta.fields.mode decides whether _hoodie_file_name carries a
value. On the
+ // preserve-metadata path the record comes from an existing file, so
leaving the column alone is
+ // not enough — MetadataValues skips null entries, and a record written
while the table was on
+ // ALL would keep the file name it already had. Overwrite it with an
explicit null instead.
+ if (metaFieldsMode.isFileNamePopulated()) {
+ MetadataValues metadataValues = new
MetadataValues().setFileName(fileName);
+ return record.prependMetaFields(schema, targetSchema, metadataValues,
prop);
+ }
+ HoodieRecord<T> withMetaFields =
+ record.prependMetaFields(schema, targetSchema, new MetadataValues(),
prop);
+ return withMetaFields.updateMetaField(targetSchema,
HoodieRecord.FILENAME_META_FIELD_ORD, null);
Review Comment:
Confirmed and fixed in `128933f5876a` — thank you, this was a real crash and
my tests could not have caught it.
`HoodieInternalRow#update` accepts only `UTF8String` or `String` and reports
anything else via `value.getClass()`, which NPEs on a null before the
`IllegalArgumentException` is built. So clearing the column threw for
`HoodieSparkRecord`, exactly as described.
Fixed in `HoodieSparkRecord#updateMetaField` rather than at the two call
sites, since it is the shared entry point both use: a null now routes through
`setNullAt`, which already handles the meta-field range correctly. Clearing a
meta column is a legitimate request rather than an error — a selective mode
leaves it unpopulated, and a record copied forward from a file written under
`ALL` needs the stale value cleared rather than carried over.
One correction to the diagnosis, which matters for the test: you attributed
the miss to the E2E tests using `SaveMode.Overwrite`. They do use `UPSERT` +
`Append`. The actual reason is the record type — those tests take the **Avro**
path, whose `data.put(ordinal, null)` tolerates a null. Reaching the Spark path
needs
`hoodie.write.record.merge.custom.implementation.classes=org.apache.hudi.DefaultSparkRecordMerger`;
`row.writer.enable` alone does not do it.
That distinction cost me a round: my first version of the regression test
passed **with the fix reverted**. The committed one is verified both ways —
`NullPointerException` without the fix, 29/29 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]
