nsivabalan commented on code in PR #19205:
URL: https://github.com/apache/hudi/pull/19205#discussion_r3763342599
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/BaseHoodieWriteClient.java:
##########
@@ -1544,13 +1545,80 @@ protected boolean loadActiveTimelineOnTableInit() {
return true;
}
+ /**
+ * Adopt the table's {@code hoodie.meta.fields.mode} when this writer did
not state one.
+ *
+ * <p>The mode is a table property: it is settable at table creation,
through hudi-cli, or by an
+ * upgrade, and never by an ordinary write. Callers routinely build a write
config without
+ * restating the table's meta-field settings — table services do, and so
does a restarted
+ * StreamSync — and such a writer must write what the table already
advertises rather than silently
+ * narrowing it. Without this, an unstated writer resolves to {@code NONE}
(via the deprecated
+ * {@code hoodie.populate.meta.fields} fallback) and writes null meta
columns into a table whose
+ * earlier files have them populated.
+ *
+ * <p>Inheritance applies only when the writer states <em>neither</em>
property. If it explicitly
+ * set the mode or the deprecated boolean, that value is left alone so the
comparison below can
+ * reject it on a mismatch: a user who deliberately passed {@code
populate.meta.fields=false}
+ * against an {@code ALL} table should be told the setting conflicts, not
have it silently
+ * overridden.
+ *
+ * @return true when the writer stated neither property, i.e. the mode was
inherited and cannot
+ * disagree with the table.
+ */
+ private static boolean inferMetaFieldsModeFromTable(HoodieTableConfig
tableConfig, HoodieWriteConfig writeConfig) {
+ boolean statedMode =
writeConfig.contains(HoodieTableConfig.META_FIELDS_MODE)
+ &&
!StringUtils.isNullOrEmpty(writeConfig.getString(HoodieTableConfig.META_FIELDS_MODE));
+ boolean statedLegacyBoolean =
writeConfig.contains(HoodieTableConfig.POPULATE_META_FIELDS);
+ if (statedMode || statedLegacyBoolean) {
+ return false;
+ }
+ MetaFieldsMode tableMode = tableConfig.getMetaFieldsMode();
+ writeConfig.setValue(HoodieTableConfig.META_FIELDS_MODE, tableMode.name());
+ // Keep the derived boolean in step, so the ~55 call sites still reading
populateMetaFields()
+ // observe an answer consistent with the mode.
+ writeConfig.setValue(HoodieTableConfig.POPULATE_META_FIELDS,
+ Boolean.toString(tableMode.toLegacyPopulateMetaFields()));
+ return true;
+ }
+
public void validateAgainstTableProperties(HoodieTableConfig tableConfig,
HoodieWriteConfig writeConfig) {
// mismatch of table versions.
CommonClientUtils.validateTableVersion(tableConfig, writeConfig);
- // Once meta fields are disabled, it cant be re-enabled for a given table.
- if (!tableConfig.populateMetaFields() && writeConfig.populateMetaFields())
{
- throw new HoodieException(HoodieTableConfig.POPULATE_META_FIELDS.key() +
" already disabled for the table. Can't be re-enabled back");
+ // A writer that stated neither meta-field property inherits the table's
mode here, which makes
+ // the comparison below a no-op for it. A writer that stated either keeps
its value and is
+ // compared.
+ inferMetaFieldsModeFromTable(tableConfig, writeConfig);
Review Comment:
Agreed on both counts — inference should not live here, and this method
should validate rather than quietly rewrite the write config. I have split it
so `validateAgainstTableProperties` is now pure comparison.
Where I would like your call is **how the write path gets the mode**,
because there are three shapes and they trade off differently. Some context
first, since it constrains all three.
### The constraint
The mode is read on the write path in more places than the handles:
| Site | Reads |
|---|---|
| `HoodieRowCreateHandle`, `HoodieWriteMergeHandle`, `BaseCreateHandle` |
the mode, to decide which meta columns to stamp |
| `HoodieAvroFileWriterFactory#newParquetFileWriter` |
`MetaFieldsMode.resolve(config)` — drives both `enableBloomFilter(...)` and the
mode handed to `HoodieAvroParquetWriter` |
| `HoodieSparkFileWriterFactory#newParquetFileWriter` | same |
| `HoodieSparkFileWriterFactory` Lance / Vortex paths |
`config.getBooleanOrDefault(POPULATE_META_FIELDS)` — the raw boolean, bypassing
the mode entirely |
None of the `newXxxFileWriter` methods take the mode or the boolean as a
parameter; each derives it from the `HoodieConfig` it is handed. And there are
8 production `getFileWriter` call sites, of which **3 have no table in scope at
all** — `ParquetUtils` (a static utility), `HoodieNativeLogFormatWriter` and
`HoodieNativeCDCFileWriter` (CDC).
So "just read the table config in the writers" is not uniformly available.
### Option 1 — resolve onto the write config once, in `initTable`
Before any writer is constructed, stamp the table's mode onto the write
config:
```java
resolveMetaFieldsModeForWrite(table.getMetaClient().getTableConfig(),
config);
validateAgainstTableProperties(table.getMetaClient().getTableConfig(),
config);
```
`initTable` is the funnel for every write path and table service, and the
`HoodieWriteConfig` is a single instance threaded down to the handles and
factories — so this fixes all 8 call sites and all 5 formats (parquet, HFile,
ORC, Lance, Vortex) with no signature changes.
- **For:** one line, no API surface touched, works for the three callers
that have no table.
- **Against:** the write config becomes mutable after construction.
Conceptually close to `setDefaults`, except `setDefaults` derives from the
config's own properties whereas this derives from the *table* config.
- **Precedent:** `BaseHoodieClient:123` already does
`config.setValue(APPLICATION_ID, ...)` post-construction, so mutation itself is
not new — deriving from another config is the new part.
### Option 2 — thread the table config (or the mode) through the call chain
Add a parameter to `getFileWriter` → `getFileWriterByFormat` →
`newXxxFileWriter`, across the three factory subclasses (Avro, Spark, Flink
RowData).
- **For:** explicit; no mutation; the writers stop deriving meta-field
policy from writer properties at all.
- **Against:** the widest change of the three. `getFileWriter` is public API
that Flink and Java clients use, so it is a compatibility surface. And the
three tableless callers would have to pass something meaningful — `NONE` for
CDC/log writers, and whatever `ParquetUtils` intends for the bloom-filter index
files it writes.
### Option 3 — resolve immediately before each `getFileWriter` call
Same mutation as option 1, but confined: each call site reads the table
config and updates the `HoodieConfig` it is about to pass.
- **For:** narrower blast radius than option 1; the reconciliation sits next
to the call that needs it, so it is visible at the point of use.
- **Against:** still mutation, now repeated at 5 sites instead of 1 — and
the 3 tableless callers still have nothing to read from, so it does not
actually cover them.
### Where I lean, and why it is weakly held
**Option 1**, because it is the only one that covers the tableless callers
without inventing a value for them, and because `initTable` is a genuine
chokepoint rather than a convenient place to put it. The mutability objection
is real but bounded: it happens once, before any writer exists, and the value
written is the table's own.
**Option 2 is the cleanest end state** if we are willing to pay the API
change — it removes the second source of truth rather than making it agree. If
you prefer that, I would rather do it as its own PR than fold it in here, since
it reaches Flink and the Java client.
One related item either way: the Lance and Vortex paths read the raw
`POPULATE_META_FIELDS` boolean rather than the mode, so a selective table
resolves to `false` there and is treated as `NONE`. That is a bug independent
of which option we pick — currently tracked in #19378.
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/config/HoodieWriteConfig.java:
##########
@@ -3892,6 +3998,8 @@ public HoodieWriteConfig build() {
@VisibleForTesting
public HoodieWriteConfig build(boolean shouldValidate) {
setDefaults();
+ // Before validate(), so the MoR / engine-type checks see the same mode
the built config will.
+ deriveLegacyPopulateMetaFieldsFromMode();
Review Comment:
Happy to move it, with one check first.
`setDefaults` walks the declared `ConfigProperty` fields and applies each
one's default or infer function, deriving purely from the config's own
properties. The derivation here is different in kind: it rewrites
`hoodie.populate.meta.fields` from `hoodie.meta.fields.mode`, i.e. one property
from another, and it has to run after both have been set by the builder rather
than as a per-property default.
If `setDefaults` is the right home anyway, the cleanest fit is probably an
infer function on `POPULATE_META_FIELDS` that reads the mode — that is the
mechanism `PARTITION_EXTRACTOR_CLASS` already uses and it would run in the same
pass. Worth noting the ordering caveat: infer functions fire from
`setDefaultValue`, which is only invoked on the table-config **create** path,
not on load, so this would cover construction but not existing tables.
Let me know if you want the infer-function shape and I will make the change.
##########
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:
Fair question, and the answer is not obvious from the code —
`MetadataValues` **skips null entries**. `updateMetadataValuesInternal` only
calls `avroRecord.put(...)` when the value is non-null, so you cannot clear a
field through it; `setFileName(null)` is a no-op rather than a write of null.
That matters here because the record on this path comes from an existing
file. Under a narrowed mode it can still carry a file name written while the
table was on `ALL`, so leaving the field alone is not the same as clearing it —
the stale value survives into the new file. Hence the two-step:
`prependMetaFields` with empty values to get the schema right, then an explicit
`updateMetaField(..., null)` to clear the column.
Related: the review bot then found that the explicit null NPEs on the Spark
record path (`HoodieInternalRow#update` calls `value.getClass()` on it). Fixed
in `128933f5876a` by routing null through `setNullAt`.
If you would rather avoid the two-step entirely, see my reply on
`HoodieWriteMergeHandle:428` — skipping the update is simpler but leaves the
stale value, so the choice is between a clear-write and accepting carry-over.
##########
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 that `updateFileName` should stay a simple "update the file name"
method. Happy to hoist the check to the caller in `writeRecordToFile`.
One thing to settle first, since it changes what the caller does: see the
thread on `HoodieWriteMergeHandle:428`. If we skip the update when the mode
does not populate the file name, a record copied forward from a file written
under `ALL` keeps its old file name. If we clear it, the caller needs the
null-write rather than a plain skip. I would rather agree that first and then
move the check, so it does not get restructured twice.
##########
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 =
Review Comment:
This is the one place I would push back, and it is worth deciding explicitly
because both behaviors are defensible.
Skipping the update leaves whatever the record already carried. On this path
the record comes from the **previous base file**, so on a table that was
narrowed — created as `ALL`, later moved to `COMMIT_TIME_ONLY` via hudi-cli — a
copied-forward record still holds the `_hoodie_file_name` written under `ALL`.
Skipping preserves that stale value, and it then points at a file that the
merge just replaced.
Writing an explicit null clears it, so the column consistently reflects what
the mode says.
This is not hypothetical: I fixed the merge handle first and assumed it was
complete, and the new upsert test immediately caught the same shape in
`BaseCreateHandle`. Records copied forward really do carry values from the
previous file.
The cost of the null-write was a genuine bug — the review bot found it NPEs
on the Spark record path — but that is now fixed at source (`128933f5876a`,
routing null through `setNullAt`).
So the choice is: **clear** (current, correct on narrowed tables, costs a
null-write) versus **skip** (simpler, leaves stale file names on tables that
were narrowed after data was written). I lean clear. If you prefer skip, I will
make the change and note the carry-over in the mode's documentation, since
users would otherwise see a populated `_hoodie_file_name` on a table whose mode
says it is null.
##########
hudi-client/hudi-spark-client/src/main/java/org/apache/hudi/common/model/HoodieSparkRecord.java:
##########
@@ -296,9 +297,14 @@ public HoodieRecord
wrapIntoHoodieRecordPayloadWithKeyGen(HoodieSchema recordSch
StructType structType =
HoodieInternalRowUtils.getCachedSchema(recordSchema);
String key;
String partition;
- boolean populateMetaFields =
Boolean.parseBoolean(props.getOrDefault(POPULATE_META_FIELDS.key(),
- POPULATE_META_FIELDS.defaultValue().toString()).toString());
- if (!populateMetaFields && keyGen.isPresent()) {
+ // Resolve via hoodie.meta.fields.mode — reading the deprecated boolean
alone would report
+ // "populated" for a selective-mode table (whose _hoodie_record_key column
is null), sending us
+ // down the meta-column branch below and NPE-ing on the null ordinal.
+ boolean recordKeyPopulated = MetaFieldsMode.resolve(
+ props.getProperty(HoodieTableConfig.META_FIELDS_MODE.key()),
Review Comment:
It would be enough **if** the mode were guaranteed present, but it is not on
every path that reaches here.
This method takes a raw `Properties` bag rather than a table config, and
what is in that bag depends on the caller — it is the record-level API shared
across engines, not something that always carries `hoodie.properties`. Reading
`META_FIELDS_MODE` alone would resolve to the empty default for any caller that
passed only the legacy boolean, and then report the record key as populated on
a selective table, which is the NPE this line exists to prevent.
`MetaFieldsMode.resolve(mode, legacyBoolean)` handles both shapes, so it is
correct regardless of which the caller supplied.
That said, your broader point stands: if the mode were materialized into
every table config and every properties bag derived from it, this could be a
single lookup. That is the same question as the `initTable` thread — happy to
fold this in if we land the resolution shape there.
##########
hudi-common/src/main/java/org/apache/hudi/common/table/HoodieTableMetaClient.java:
##########
@@ -1535,7 +1554,37 @@ public Properties build() {
tableConfig.setValue(HoodieTableConfig.CDC_SUPPLEMENTAL_LOGGING_MODE,
cdcSupplementalLoggingMode);
}
}
- if (null != populateMetaFields) {
+ // hoodie.meta.fields.mode is the source of truth, and hoodie.properties
must never contradict
+ // it: a table written selectively that still recorded
populate.meta.fields=true would be read
+ // as ALL by a pre-1.3.0 reader, which ignores the mode property
entirely. For NONE that is
+ // actively unsafe — an older incremental reader would run against
all-null commit times and
+ // silently return no rows.
+ //
+ // A caller that states both and disagrees is rejected rather than
silently overridden. Half
+ // their request would otherwise be discarded without a word, and it
would be inconsistent with
+ // BaseHoodieWriteClient#validateAgainstTableProperties, which already
rejects an explicitly-set
+ // boolean that disagrees with the table. Only a genuine contradiction
fails: ALL + true and
+ // NONE + false are coherent restatements and pass.
+ if (null != metaFieldsMode) {
+ boolean derivedPopulateMetaFields =
metaFieldsMode.toLegacyPopulateMetaFields();
+ if (null != populateMetaFields && populateMetaFields !=
derivedPopulateMetaFields) {
+ throw new HoodieException(String.format(
+ "Conflicting meta-field settings at table creation: %s=%s
implies %s=%s, but %s was "
+ + "explicitly set to %s. %s is the source of truth and the
boolean is only its "
+ + "pre-1.3.0 fallback, so the two cannot be set to different
things. Drop %s, or set "
+ + "it to %s.",
+ HoodieTableConfig.META_FIELDS_MODE.key(), metaFieldsMode,
+ HoodieTableConfig.POPULATE_META_FIELDS.key(),
derivedPopulateMetaFields,
+ HoodieTableConfig.POPULATE_META_FIELDS.key(), populateMetaFields,
+ HoodieTableConfig.META_FIELDS_MODE.key(),
+ HoodieTableConfig.POPULATE_META_FIELDS.key(),
derivedPopulateMetaFields));
+ }
+ tableConfig.setValue(HoodieTableConfig.META_FIELDS_MODE,
metaFieldsMode.name());
+ tableConfig.setValue(HoodieTableConfig.POPULATE_META_FIELDS,
+ Boolean.toString(derivedPopulateMetaFields));
+ } else if (null != populateMetaFields) {
+ // No explicit mode: preserve pre-1.3.0 behavior and record only the
legacy boolean, which
+ // resolves to ALL / NONE on read.
Review Comment:
Good catch — this is the gap that forces inference downstream.
`TableBuilder` currently writes the mode only when one was explicitly
supplied. If a caller passes just `populate.meta.fields`, the table is created
with the boolean alone and no mode, so every later reader has to re-derive it.
Setting the derived mode here would close that for **newly created** tables.
Two things worth noting before we do it:
1. It does not help existing tables. Tables created before this property —
including the v6 tables this feature has to support — will still have only the
boolean, so `getMetaFieldsMode()` must keep deriving on read regardless.
2. It changes what lands in `hoodie.properties` for callers that pass only
the boolean today, so a v6 table created through this path would gain a
`hoodie.meta.fields.mode` entry. Harmless as far as I can tell, since the value
is derived from what is already there, but it is a visible on-disk change worth
being deliberate about.
Happy to make the change if you want it. It pairs naturally with the
`initTable` discussion — if the mode is always materialized at creation *and*
derived on read for older tables, then the write path can rely on it being
present.
--
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]
