nsivabalan commented on code in PR #19205:
URL: https://github.com/apache/hudi/pull/19205#discussion_r3714902831
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/BaseHoodieWriteClient.java:
##########
@@ -1548,9 +1549,40 @@ public void
validateAgainstTableProperties(HoodieTableConfig tableConfig, Hoodie
// 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");
+ // Meta-field population is physical, so a writer must not claim columns
the table does not
+ // have. Compare the full enum rather than the legacy booleans: those
collapse every selective
+ // mode to false, so a writer claiming COMMIT_TIME_ONLY against a NONE
table would slip through
+ // and advertise commit times that were never written.
+ //
+ // Two distinct cases, because writers routinely omit meta-field settings
entirely:
+ //
+ // - Widening is always rejected. Enabling a column now would leave
earlier commits without it,
+ // and readers cannot tell the two apart.
+ // - Any disagreement is rejected when the writer *explicitly* sets
hoodie.meta.fields.mode.
+ // That covers narrowing too, e.g. an explicit NONE against a
COMMIT_TIME_ONLY table, which
+ // would write null commit times while the table still advertises
COMMIT_TIME_ONLY and make
+ // incremental queries silently miss those rows.
+ //
+ // A writer that never mentions the mode is left alone: resolving to NONE
against an ALL table
+ // is long-standing behavior for callers that build a write config without
restating the table's
+ // settings, and writing fewer meta columns cannot make a reader believe
in absent data.
+ MetaFieldsMode tableMetaFieldsMode = tableConfig.getMetaFieldsMode();
+ MetaFieldsMode writeMetaFieldsMode = writeConfig.getMetaFieldsMode();
+ boolean writerStatedMode =
writeConfig.contains(HoodieTableConfig.META_FIELDS_MODE)
+ &&
!StringUtils.isNullOrEmpty(writeConfig.getString(HoodieTableConfig.META_FIELDS_MODE));
Review Comment:
You were right on both counts — it was not fixed, and `c78dc962` fixed
resolution rather than this gate. Fixed now in two commits, because the
diagnosis pointed at a better primary fix than tightening the guard alone.
**[`4a7fa0c`](https://github.com/apache/hudi/pull/19205/commits/4a7fa0c4dd76) —
StreamSync inherits the mode.** Your walk-through showed the danger is
*omission*, not contradiction, which is also why
`HoodieWriterUtils.validateTableConfig` cannot catch it: that loop only visits
keys the writer stated, so `hoodie.meta.fields.mode` is never examined and the
legacy boolean that *is* present matches on-disk. Meta-field population is a
property of the table, not of the run, so when a run does not state a mode it
now adopts the table's — exactly what
`HoodieSparkSqlWriter#mergeParamsAndGetHoodieConfig` already does at
`:1101-1107` ("over-ride only if not explicitly set by the user"). That also
removes the accident you noted: the datasource path was safe only because it
happens to copy table props.
This makes the common restart *succeed correctly* rather than start failing,
which matters — requiring users to restate the mode in their streamer props on
every restart would be a regression for anyone who set it once at creation.
**[`08b12f7`](https://github.com/apache/hudi/pull/19205/commits/08b12f7bed23) —
your guard fix, as suggested.** Applied verbatim:
```java
} else if (writeMetaFieldsMode != tableMetaFieldsMode
&& (writerStatedMode || tableMetaFieldsMode.isSelective())) {
```
Inheritance handles the common case; this catches what remains, including
anything that builds a write config outside StreamSync. The narrowing carve-out
is now scoped to `ALL`/`NONE` tables, which is the `d5026e9a2485` behavior you
identified. The error message also distinguishes an explicitly set mode from
one defaulted off the legacy boolean, since it can no longer assume the former.
Tests: the case you asked for (table `COMMIT_TIME_ONLY`, unstated writer →
must throw) plus `COMMIT_TIME_AND_FILE_NAME`, and the sibling pair from your
`:183` comment — `COMMIT_TIME_ONLY` vs `FILE_NAME_ONLY` rejected both ways,
since neither is a narrowing of the other. Confirmed non-vacuous: reverting the
guard fails exactly the two unstated-narrowing tests and leaves the `ALL`-table
carve-out green.
One thing I could not verify locally: `hudi-utilities` does not compile in
my environment — `KafkaAvroSchemaDeserializer` fails on a Confluent version
mismatch, and I confirmed it fails identically on the base commit with my
changes stashed. So the StreamSync change is type-checked (compiled the module
minus that file, zero errors in the changed lines, both API signatures
confirmed with `javap`) but not test-run. Relying on CI for that one.
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/BaseHoodieWriteClient.java:
##########
@@ -1548,9 +1549,40 @@ public void
validateAgainstTableProperties(HoodieTableConfig tableConfig, Hoodie
// 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");
+ // Meta-field population is physical, so a writer must not claim columns
the table does not
+ // have. Compare the full enum rather than the legacy booleans: those
collapse every selective
+ // mode to false, so a writer claiming COMMIT_TIME_ONLY against a NONE
table would slip through
+ // and advertise commit times that were never written.
+ //
+ // Two distinct cases, because writers routinely omit meta-field settings
entirely:
+ //
+ // - Widening is always rejected. Enabling a column now would leave
earlier commits without it,
+ // and readers cannot tell the two apart.
+ // - Any disagreement is rejected when the writer *explicitly* sets
hoodie.meta.fields.mode.
+ // That covers narrowing too, e.g. an explicit NONE against a
COMMIT_TIME_ONLY table, which
+ // would write null commit times while the table still advertises
COMMIT_TIME_ONLY and make
+ // incremental queries silently miss those rows.
+ //
+ // A writer that never mentions the mode is left alone: resolving to NONE
against an ALL table
+ // is long-standing behavior for callers that build a write config without
restating the table's
+ // settings, and writing fewer meta columns cannot make a reader believe
in absent data.
+ MetaFieldsMode tableMetaFieldsMode = tableConfig.getMetaFieldsMode();
+ MetaFieldsMode writeMetaFieldsMode = writeConfig.getMetaFieldsMode();
+ boolean writerStatedMode =
writeConfig.contains(HoodieTableConfig.META_FIELDS_MODE)
+ &&
!StringUtils.isNullOrEmpty(writeConfig.getString(HoodieTableConfig.META_FIELDS_MODE));
+ if (writeMetaFieldsMode.isWiderThan(tableMetaFieldsMode)) {
+ throw new HoodieException(String.format(
+ "%s cannot be widened for an existing table: table is %s but the
writer requests %s. Meta "
+ + "columns are physical, so enabling one now would leave earlier
commits without it. "
+ + "Set %s=%s on the writer, or recreate the table to change it.",
+ HoodieTableConfig.META_FIELDS_MODE.key(), tableMetaFieldsMode,
writeMetaFieldsMode,
+ HoodieTableConfig.META_FIELDS_MODE.key(), tableMetaFieldsMode));
+ } else if (writerStatedMode && writeMetaFieldsMode != tableMetaFieldsMode)
{
Review Comment:
Resolved by
[`08b12f7`](https://github.com/apache/hudi/pull/19205/commits/08b12f7bed23),
which takes @voonhous's read: `writerStatedMode` is load-bearing but was
mis-scoped.
It stays (the unstated `ALL`-table narrowing path genuinely needs it, per
`d5026e9a2485` / HUDI-2161), but it is no longer the sole gate — the branch now
also fires when the *table* is selective:
```java
} else if (writeMetaFieldsMode != tableMetaFieldsMode
&& (writerStatedMode || tableMetaFieldsMode.isSelective())) {
```
So the narrowing carve-out is limited to `ALL`/`NONE` tables, where nothing
keys off a partially populated state. A selective table advertises exactly what
the read path trusts, so a disagreement there has to be rejected whether or not
the writer named a mode — that was the silent data-loss path.
@danny0405 on eliminating the flag: it cannot go entirely, since
`validateAgainstTablePropertiesAllowsUnstatedWriterToNarrow` (table `ALL`,
writer sets only `withPopulateMetaFields(false)`) must keep passing. The full
matrix @voonhous laid out at `:1573` is now pinned by tests, including the
`COMMIT_TIME_ONLY` / `FILE_NAME_ONLY` pair that is rejected in both directions.
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/BaseHoodieWriteClient.java:
##########
@@ -1548,9 +1549,40 @@ public void
validateAgainstTableProperties(HoodieTableConfig tableConfig, Hoodie
// 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");
+ // Meta-field population is physical, so a writer must not claim columns
the table does not
+ // have. Compare the full enum rather than the legacy booleans: those
collapse every selective
+ // mode to false, so a writer claiming COMMIT_TIME_ONLY against a NONE
table would slip through
+ // and advertise commit times that were never written.
+ //
+ // Two distinct cases, because writers routinely omit meta-field settings
entirely:
+ //
+ // - Widening is always rejected. Enabling a column now would leave
earlier commits without it,
+ // and readers cannot tell the two apart.
+ // - Any disagreement is rejected when the writer *explicitly* sets
hoodie.meta.fields.mode.
+ // That covers narrowing too, e.g. an explicit NONE against a
COMMIT_TIME_ONLY table, which
+ // would write null commit times while the table still advertises
COMMIT_TIME_ONLY and make
+ // incremental queries silently miss those rows.
+ //
+ // A writer that never mentions the mode is left alone: resolving to NONE
against an ALL table
+ // is long-standing behavior for callers that build a write config without
restating the table's
+ // settings, and writing fewer meta columns cannot make a reader believe
in absent data.
+ MetaFieldsMode tableMetaFieldsMode = tableConfig.getMetaFieldsMode();
+ MetaFieldsMode writeMetaFieldsMode = writeConfig.getMetaFieldsMode();
+ boolean writerStatedMode =
writeConfig.contains(HoodieTableConfig.META_FIELDS_MODE)
+ &&
!StringUtils.isNullOrEmpty(writeConfig.getString(HoodieTableConfig.META_FIELDS_MODE));
+ if (writeMetaFieldsMode.isWiderThan(tableMetaFieldsMode)) {
Review Comment:
Resolved by
[`08b12f7`](https://github.com/apache/hudi/pull/19205/commits/08b12f7bed23),
which takes @voonhous's read: `writerStatedMode` is load-bearing but was
mis-scoped.
It stays (the unstated `ALL`-table narrowing path genuinely needs it, per
`d5026e9a2485` / HUDI-2161), but it is no longer the sole gate — the branch now
also fires when the *table* is selective:
```java
} else if (writeMetaFieldsMode != tableMetaFieldsMode
&& (writerStatedMode || tableMetaFieldsMode.isSelective())) {
```
So the narrowing carve-out is limited to `ALL`/`NONE` tables, where nothing
keys off a partially populated state. A selective table advertises exactly what
the read path trusts, so a disagreement there has to be rejected whether or not
the writer named a mode — that was the silent data-loss path.
@danny0405 on eliminating the flag: it cannot go entirely, since
`validateAgainstTablePropertiesAllowsUnstatedWriterToNarrow` (table `ALL`,
writer sets only `withPopulateMetaFields(false)`) must keep passing. The full
matrix @voonhous laid out at `:1573` is now pinned by tests, including the
`COMMIT_TIME_ONLY` / `FILE_NAME_ONLY` pair that is rejected in both directions.
##########
hudi-spark-datasource/hudi-spark-common/src/main/scala/org/apache/hudi/HoodieWriterUtils.scala:
##########
@@ -352,6 +352,23 @@ object HoodieWriterUtils {
diffConfigs.append(s"${HoodieTableConfig.RECORD_MERGE_STRATEGY_ID}:\t$mergeStrategyId\tnull\n")
}
}
+
+ // hoodie.meta.fields.mode is a physical-storage decision baked into
files at write time.
+ // Changing it at runtime would silently produce mixed-mode files
whose incremental / file
+ // pruning behavior differs between old and new commits. The default
loop above only flags
+ // the mismatch when the on-disk value is non-null, so an older table
with the property
+ // absent from hoodie.properties would let a null → non-empty
transition slip through
+ // (silent-drop risk on pre-enablement commits). Guard the null →
selective-mode case
+ // explicitly here. Set it only at table creation, via the hudi-cli,
or during table upgrade;
+ // otherwise the only way to change it is to recreate the table.
+ val paramsMetaFieldsMode =
params.getOrElse(HoodieTableConfig.META_FIELDS_MODE.key(), "")
+ val onDiskMetaFieldsMode =
tableConfig.getString(HoodieTableConfig.META_FIELDS_MODE)
+ if (paramsMetaFieldsMode.nonEmpty && (onDiskMetaFieldsMode == null ||
onDiskMetaFieldsMode.isEmpty)) {
Review Comment:
Fixed in
[`8db4d71`](https://github.com/apache/hudi/pull/19205/commits/8db4d71c37fb) —
took your suggestion, comparing resolved modes.
The second-order problem you raised is the one that convinced me: since the
property is backfilled only by `NineToTenUpgradeHandler` and `current()` is
already `TEN`, a v10 table without it will never be upgraded again, so
rejecting an explicit restatement left it no way to adopt the property at all.
One deviation from the snippet: it uses
`MetaFieldsMode.resolve(tableConfig)` rather than
`tableConfig.getMetaFieldsMode`. The parameter is typed `HoodieConfig`, and
`HoodieCatalogTable:262` passes a plain one built from a map via
`convertMapToHoodieConfig` — so a cast to `HoodieTableConfig` would throw
`ClassCastException` on the Spark SQL path. The `resolve(HoodieConfig)`
overload (added for @danny0405's comment on `MetaFieldsMode:139`) reads the
same two properties and works for both caller shapes.
Tests: the acceptance case you asked for — `mode=ALL` on a table carrying
only `populate.meta.fields=true` — plus the `NONE` mirror, and two rejections
(`COMMIT_TIME_ONLY` on a `NONE` table, `NONE` on an `ALL` table) so the real
guard is pinned too. Each builds a table with *only* the legacy boolean on
disk, so it resolves through the fallback path. Verified non-vacuous: restoring
the presence-based check fails exactly the two acceptance tests.
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/config/HoodieWriteConfig.java:
##########
@@ -3586,11 +3618,31 @@ public Builder withCanIgnorePostCommitFailures(boolean
canIgnorePostCommitFailur
return this;
}
+ /**
+ * @deprecated since 1.3.0, use {@link
#withMetaFieldsMode(MetaFieldsMode)} instead
+ * ({@code true} maps to {@link MetaFieldsMode#ALL}, {@code false} to
{@link MetaFieldsMode#NONE}).
+ */
+ @Deprecated
public Builder withPopulateMetaFields(boolean populateMetaFields) {
writeConfig.setValue(HoodieTableConfig.POPULATE_META_FIELDS,
Boolean.toString(populateMetaFields));
return this;
}
+ public Builder withMetaFieldsMode(MetaFieldsMode metaFieldsMode) {
+ // Leaving the mode unset defers to the deprecated populate.meta.fields
boolean. Setting it
+ // also rewrites that boolean from the mode, so the two can never
disagree — a config carrying
+ // a selective mode alongside populate.meta.fields=true would otherwise
create a table whose
+ // hoodie.properties misleads pre-1.3.0 readers into treating it as ALL.
+ if (metaFieldsMode == null) {
+ writeConfig.setValue(HoodieTableConfig.META_FIELDS_MODE, "");
+ } else {
+ writeConfig.setValue(HoodieTableConfig.META_FIELDS_MODE,
metaFieldsMode.name());
+ writeConfig.setValue(HoodieTableConfig.POPULATE_META_FIELDS,
+ Boolean.toString(metaFieldsMode.toLegacyPopulateMetaFields()));
Review Comment:
Fixed in
[`9193e874`](https://github.com/apache/hudi/pull/19205/commits/9193e8741178) —
took the cheapest fix you suggested and re-derive in `build()`, so order stops
mattering.
It runs before `validate()` so the MoR / engine-type checks see the same
mode the built config will, and only fires when a mode is actually set —
otherwise it would clobber the legacy boolean for the many callers that never
mention the mode.
And you were right that the test skipped the order that did not hold:
`legacyBooleanSetAfterModeStillResolvesFromMode` asserted the resolved mode but
not the raw property, which is precisely where the contradiction lived. It now
asserts the raw property too, plus a loop over all five modes in both call
orders and a case pinning that an unset mode leaves the boolean untouched.
Verified non-vacuous: moving the derivation back into the setter fails exactly
the two order tests.
--
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]
