nsivabalan commented on code in PR #19205:
URL: https://github.com/apache/hudi/pull/19205#discussion_r3767559712
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/io/BaseCreateHandle.java:
##########
@@ -167,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()) {
Review Comment:
Agreed, done in 4445b5e. The check is at the call site now and
`updateFileName` is back to just setting the file name:
```java
HoodieRecord populatedRecord = metaFieldsMode.isFileNamePopulated()
? updateFileName(record, schema, writeSchemaWithMetaFields,
path.getName(), config.getProps())
: clearFileName(record, schema, writeSchemaWithMetaFields,
config.getProps());
```
The other branch is a named `clearFileName` rather than a skip, for the
reason on the merge-handle thread: the record comes from the previous base
file, so it can carry a name written while the table was on `ALL`, and that
name points at a file it no longer lives in. Its javadoc records why the null
cannot go through `MetadataValues`.
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/io/BaseCreateHandle.java:
##########
@@ -167,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:
It does, for the values `MetadataValues` carries — but it cannot express
"clear this field", which is what this branch needs.
`prependMetaFields` delegates to `updateMetadataValuesInternal`, which skips
nulls:
```java
// HoodieAvroIndexedRecord:375-387
static void updateMetadataValuesInternal(GenericRecord avroRecord,
MetadataValues metadataValues) {
if (metadataValues.isEmpty()) {
return; // no-op
}
String[] values = metadataValues.getValues();
for (int pos = 0; pos < values.length; ++pos) {
String value = values[pos];
if (value != null) { // <-- here
avroRecord.put(HoodieMetadataField.values()[pos].getFieldName(),
value);
}
}
}
```
So `new MetadataValues().setFileName(null)` is indistinguishable from not
setting it: the record keeps whatever `_hoodie_file_name` it arrived with. On
the preserve-metadata path it arrived from the previous base file, so on a
table narrowed from `ALL` that is a stale name pointing at a replaced file.
Hence `prependMetaFields` for the schema projection, then an explicit
`updateMetaField(targetSchema, FILENAME_META_FIELD_ORD, null)` to actually
blank the column. I have moved that into a `clearFileName` method (4445b5e)
with this reasoning in its javadoc, since it is not obvious from the call.
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/config/HoodieWriteConfig.java:
##########
@@ -3883,6 +3935,27 @@ private void validate() {
checkArgument(ttlStatsMaxParallelism > 0,
String.format("%s must be positive, but was %d",
HoodieTTLConfig.STATS_MAX_PARALLELISM.key(),
ttlStatsMaxParallelism));
+
+ // hoodie.meta.fields.mode is the source of truth for meta-column
population; the deprecated
+ // populate.meta.fields boolean is consulted only when the mode is
absent. There is therefore
+ // no ambiguous combination to reject here — MetaFieldsMode.resolve
throws on unrecognized
+ // values.
+ MetaFieldsMode metaFieldsMode = writeConfig.getMetaFieldsMode();
+ // Selective meta-field modes are CoW-only in this release. MoR
log-write path does not yet
+ // respect the mode, which would silently produce log records with null
meta columns.
+ boolean isSelective = metaFieldsMode != MetaFieldsMode.ALL &&
metaFieldsMode != MetaFieldsMode.NONE;
Review Comment:
Added — `MetaFieldsMode#isSelective()` exists and is used throughout (the
write-path validation, `TenToNineDowngradeHandler`, and
`HoodieTableMetaClient.TableBuilder`).
It is value-based rather than "is the property present": true for
`COMMIT_TIME_ONLY`, `FILE_NAME_ONLY`, and `COMMIT_TIME_AND_FILE_NAME`, false
for `ALL` and `NONE`. That distinction matters for the downgrade handler, where
`ALL`/`NONE` can be dropped losslessly because the legacy boolean expresses
them, and the other three cannot.
--
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]
