nsivabalan commented on code in PR #19205:
URL: https://github.com/apache/hudi/pull/19205#discussion_r3738674429
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/config/HoodieWriteConfig.java:
##########
@@ -3883,6 +3932,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();
Review Comment:
**Reopening: the behavior changed after you read my reply, and it changed on
exactly the surface your P1 was about.**
What you read: a config carrying `populate.meta.fields=true` alongside a
selective mode was *accepted*, with the boolean silently derived to `false`.
Your P1 was that the two properties must not be able to contradict each other
on disk, and derivation achieved that.
What it does now: that combination is **rejected at table creation**.
@nsivabalan pushed back on the derive-silently answer with a question I did not
have a good response to — if `populate=true` can never be persisted alongside a
selective mode, why accept it as input at all? A user who states it believes
they asked for something, and we were discarding half their request without a
word.
So the invariant you asked for is unchanged, but it is now upheld by
*rejection* rather than by *override*:
| | |
|---|---|
| selective mode + unset boolean | derived as `false` (the ordinary case) |
| selective mode + `false` | coherent restatement, accepted |
| selective mode + `true` | **rejected** |
| `ALL` + `true` | coherent restatement, accepted |
| `ALL` + `false` | **rejected** |
Note this is deliberately *narrower* than the check you originally proposed
and that `664ff2e` removed: that one rejected `populate=true` + **any** mode,
including `ALL` + `true`, which is a perfectly coherent request. Only genuine
contradictions fail now.
It also makes creation consistent with the write path, which already rejects
an explicitly-set boolean that disagrees with the table rather than overriding
it.
One implementation note, since it is the part that could have broken things:
`setPopulateMetaFields` took a primitive `boolean`, and all three creation
paths read the property via `getBooleanOrDefault` and always passed it — so
every writer that never mentioned the property looked like it had explicitly
asked for `true`, and a plain `hoodie.meta.fields.mode=COMMIT_TIME_ONLY` would
have failed with a spurious conflict. The setter now takes a nullable
`Boolean`, and `HoodieSparkSqlWriter` (both paths) and `StreamSync` pass `null`
when unstated. The other callers — both bootstrap executors, the metadata
table, Flink — never set a mode, so the check cannot fire for them.
Worth a fresh look given it is your P1.
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/config/HoodieWriteConfig.java:
##########
@@ -1772,8 +1773,40 @@ public int getSmallFileGroupCandidatesLimit() {
return getInt(MERGE_SMALL_FILE_GROUP_CANDIDATES_LIMIT);
}
+ /**
+ * @return true when every meta column is populated.
+ *
+ * <p>Derived from {@link #getMetaFieldsMode()} so that call sites still
written against the
+ * deprecated {@code hoodie.populate.meta.fields} boolean observe the same
answer as the enum:
+ * only {@link MetaFieldsMode#ALL} populates every meta column.
+ */
public boolean populateMetaFields() {
- return getBooleanOrDefault(HoodieTableConfig.POPULATE_META_FIELDS);
+ return getMetaFieldsMode().toLegacyPopulateMetaFields();
+ }
+
+ /**
+ * @return the {@link MetaFieldsMode} resolved from the write config.
+ * {@code hoodie.meta.fields.mode} is the source of truth; configs written
before that property
+ * existed fall back to {@link MetaFieldsMode#ALL} or {@link
MetaFieldsMode#NONE} based on the
+ * deprecated {@code hoodie.populate.meta.fields} boolean.
+ */
+ public MetaFieldsMode getMetaFieldsMode() {
+ return
MetaFieldsMode.resolve(getStringOrDefault(HoodieTableConfig.META_FIELDS_MODE),
Review Comment:
**Reopening: this changed after you read my reply.**
Your P1 was that a direct `SparkRDDWriteClient` could open a persisted
`COMMIT_TIME_ONLY` table with `populate=false` and no mode, resolve to `NONE`,
pass the boolean-only check, and write null commit times while the table still
advertised `COMMIT_TIME_ONLY`. My reply said `validateAgainstTableProperties`
now compares the full enum, which caught it.
That is still true, but the shape has changed underneath it.
`hoodie.meta.fields.mode` is now a **table property** that a write can never
change, and the client applies one rule for every engine:
- writer states **neither** meta-field property → **inherits** the table's
mode; no comparison
- writer states **either** one → compared, rejected on mismatch, in both
directions
So your exact scenario now resolves one of two ways rather than reaching an
enum comparison at all. If the writer stated nothing, it inherits
`COMMIT_TIME_ONLY` and keeps writing commit times. If it stated
`populate=false`, that is an explicit disagreement with the table and it fails
loudly. Either way it can no longer silently write null commit times.
Two consequences worth your attention:
1. This retires the HUDI-2161 carve-out (`d5026e9a2485`) that let an
unstated writer narrow an `ALL` table to `NONE`. Inheritance is what makes that
safe — table services and a restarted `HoodieStreamer` state neither property,
so they inherit rather than being rejected. It also let me delete
`StreamSync`'s own back-fill, which was redundant and keyed only on the mode
key.
2. It is a **breaking change**: passing `hoodie.populate.meta.fields=false`
against an `ALL` table now throws where it used to narrow silently. Called out
in the description for release notes. Only when *explicitly set*.
`TestBaseHoodieWriteClient` covers these combinations at 110 tests,
including the one you described.
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/table/upgrade/TenToNineDowngradeHandler.java:
##########
@@ -18,25 +18,61 @@
package org.apache.hudi.table.upgrade;
+import org.apache.hudi.common.config.ConfigProperty;
import org.apache.hudi.common.engine.HoodieEngineContext;
+import org.apache.hudi.common.model.MetaFieldsMode;
import org.apache.hudi.common.table.HoodieTableConfig;
import org.apache.hudi.config.HoodieWriteConfig;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
import java.util.Collections;
+import java.util.HashSet;
+import java.util.Set;
/**
* Version 10 writes native log files by default. Downgrading to version 9
requires
* full compaction of native data/delete logs before the downgrade completes.
+ *
+ * <p>Version 10 also introduced {@code hoodie.meta.fields.mode}. Version 9
does not understand it,
+ * so the property is dropped here while {@code hoodie.populate.meta.fields}
is left exactly as it
+ * stands — {@code ALL} and {@code NONE} tables round-trip unchanged because
those are precisely the
+ * two states the legacy boolean can express. Selective modes cannot be
expressed in version 9, so
+ * the table degrades to what its legacy boolean says (which is {@code false},
i.e. NONE) and we warn.
*/
public class TenToNineDowngradeHandler implements DowngradeHandler {
+
+ private static final Logger LOG =
LoggerFactory.getLogger(TenToNineDowngradeHandler.class);
+
@Override
public UpgradeDowngrade.TableConfigChangeSet downgrade(
HoodieWriteConfig config,
HoodieEngineContext context,
String instantTime,
SupportsUpgradeDowngrade upgradeDowngradeHelper) {
+ Set<ConfigProperty> propertiesToDelete = new HashSet<>();
+ propertiesToDelete.add(HoodieTableConfig.TABLE_STORAGE_LAYOUT);
+
+ // The warning is best-effort: dropping the property is what matters, and
the helper is not
+ // always available (some callers drive the change set directly).
+ MetaFieldsMode metaFieldsMode = upgradeDowngradeHelper == null
+ ? MetaFieldsMode.ALL
+ : upgradeDowngradeHelper.getTable(config,
context).getMetaClient().getTableConfig().getMetaFieldsMode();
+ if (metaFieldsMode != MetaFieldsMode.ALL && metaFieldsMode !=
MetaFieldsMode.NONE) {
+ LOG.warn("Table is using {}={}, which table version 9 cannot express.
The property is being "
+ + "removed and the table will behave as {}=false (no meta
columns) to version 9 readers. "
+ + "Already-written files keep their populated meta columns, but
incremental queries that "
+ + "relied on {} will stop returning rows. Recreate the table if
you need that behavior back.",
+ HoodieTableConfig.META_FIELDS_MODE.key(), metaFieldsMode,
+ HoodieTableConfig.POPULATE_META_FIELDS.key(), metaFieldsMode);
+ }
+ // hoodie.populate.meta.fields is deliberately left untouched: whatever
the table recorded before
+ // the downgrade stays, so ALL and NONE tables are bit-identical
afterwards.
+ propertiesToDelete.add(HoodieTableConfig.META_FIELDS_MODE);
+
return new UpgradeDowngrade.TableConfigChangeSet(
Collections.emptyMap(),
- Collections.singleton(HoodieTableConfig.TABLE_STORAGE_LAYOUT));
+ propertiesToDelete);
Review Comment:
**Reopening: the downgrade behavior reversed twice since you read this, and
it has landed somewhere different from what my reply described.**
The write-back you both asked for is in and unchanged — downgrade derives
`populate.meta.fields` from the mode and deletes the mode, mirroring
`NineToEightDowngradeHandler:116-117`. That part is settled.
What changed is the selective-mode case:
1. My reply said selective modes degrade to `NONE` with a `LOG.warn`.
@voonhous objected that this was the only one-way, unrecoverable handler pair
in the package.
2. I then made it **throw**, and told you so.
3. @nsivabalan pointed out the throw was unnecessary, and he was right — I
had missed the reason.
**Every selective mode already persists `populate.meta.fields=false`**,
because the boolean is always derived from the mode rather than taken from the
caller. So there is nothing to decide at downgrade time: drop the mode, restate
the `false` that is already on disk, done. No special case at all.
| Table mode | v9 sees | Round trip |
|---|---|---|
| `ALL` | `populate=true` | lossless |
| `NONE` | `populate=false` | lossless |
| any selective mode | `populate=false` | mode is lost |
That is the correct reading for v9, which has no code that reads
`_hoodie_commit_time` selectively — presenting the table as having no meta
columns is the only honest thing it can say. Files keep their populated
columns; only how the table advertises itself changes. Critically the direction
is safe: the table **under-claims**. Over-claiming is the bug, and that is
exactly what the write-back prevents (@voonhous's original catch on the `true`
default).
@voonhous — on your one-way concern specifically: a selective mode still
cannot survive a v10 → v9 → v10 round trip. It returns as `NONE`, and hudi-cli
cannot widen it back, so recreating the table is the only recovery. I think
that is better handled as a documented, warned, and *tested* lossy path than as
a hard failure the operator cannot work around — but it is your call, and
`reUpgradeAfterDowngradingASelectiveModeResolvesToNone` now pins it rather than
leaving it implicit.
`TestTenToNineDowngradeHandler` is 14/14 and the full upgrade/downgrade
suite 84/84.
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/table/upgrade/TenToNineDowngradeHandler.java:
##########
@@ -18,25 +18,61 @@
package org.apache.hudi.table.upgrade;
+import org.apache.hudi.common.config.ConfigProperty;
import org.apache.hudi.common.engine.HoodieEngineContext;
+import org.apache.hudi.common.model.MetaFieldsMode;
import org.apache.hudi.common.table.HoodieTableConfig;
import org.apache.hudi.config.HoodieWriteConfig;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
import java.util.Collections;
+import java.util.HashSet;
+import java.util.Set;
/**
* Version 10 writes native log files by default. Downgrading to version 9
requires
* full compaction of native data/delete logs before the downgrade completes.
+ *
+ * <p>Version 10 also introduced {@code hoodie.meta.fields.mode}. Version 9
does not understand it,
+ * so the property is dropped here while {@code hoodie.populate.meta.fields}
is left exactly as it
+ * stands — {@code ALL} and {@code NONE} tables round-trip unchanged because
those are precisely the
+ * two states the legacy boolean can express. Selective modes cannot be
expressed in version 9, so
+ * the table degrades to what its legacy boolean says (which is {@code false},
i.e. NONE) and we warn.
*/
public class TenToNineDowngradeHandler implements DowngradeHandler {
+
+ private static final Logger LOG =
LoggerFactory.getLogger(TenToNineDowngradeHandler.class);
+
@Override
public UpgradeDowngrade.TableConfigChangeSet downgrade(
HoodieWriteConfig config,
HoodieEngineContext context,
String instantTime,
SupportsUpgradeDowngrade upgradeDowngradeHelper) {
+ Set<ConfigProperty> propertiesToDelete = new HashSet<>();
+ propertiesToDelete.add(HoodieTableConfig.TABLE_STORAGE_LAYOUT);
+
+ // The warning is best-effort: dropping the property is what matters, and
the helper is not
+ // always available (some callers drive the change set directly).
+ MetaFieldsMode metaFieldsMode = upgradeDowngradeHelper == null
+ ? MetaFieldsMode.ALL
+ : upgradeDowngradeHelper.getTable(config,
context).getMetaClient().getTableConfig().getMetaFieldsMode();
+ if (metaFieldsMode != MetaFieldsMode.ALL && metaFieldsMode !=
MetaFieldsMode.NONE) {
+ LOG.warn("Table is using {}={}, which table version 9 cannot express.
The property is being "
+ + "removed and the table will behave as {}=false (no meta
columns) to version 9 readers. "
+ + "Already-written files keep their populated meta columns, but
incremental queries that "
+ + "relied on {} will stop returning rows. Recreate the table if
you need that behavior back.",
+ HoodieTableConfig.META_FIELDS_MODE.key(), metaFieldsMode,
+ HoodieTableConfig.POPULATE_META_FIELDS.key(), metaFieldsMode);
+ }
+ // hoodie.populate.meta.fields is deliberately left untouched: whatever
the table recorded before
+ // the downgrade stays, so ALL and NONE tables are bit-identical
afterwards.
+ propertiesToDelete.add(HoodieTableConfig.META_FIELDS_MODE);
Review Comment:
**Reopening: the downgrade behavior reversed twice since you read this, and
it has landed somewhere different from what my reply described.**
The write-back you both asked for is in and unchanged — downgrade derives
`populate.meta.fields` from the mode and deletes the mode, mirroring
`NineToEightDowngradeHandler:116-117`. That part is settled.
What changed is the selective-mode case:
1. My reply said selective modes degrade to `NONE` with a `LOG.warn`.
@voonhous objected that this was the only one-way, unrecoverable handler pair
in the package.
2. I then made it **throw**, and told you so.
3. @nsivabalan pointed out the throw was unnecessary, and he was right — I
had missed the reason.
**Every selective mode already persists `populate.meta.fields=false`**,
because the boolean is always derived from the mode rather than taken from the
caller. So there is nothing to decide at downgrade time: drop the mode, restate
the `false` that is already on disk, done. No special case at all.
| Table mode | v9 sees | Round trip |
|---|---|---|
| `ALL` | `populate=true` | lossless |
| `NONE` | `populate=false` | lossless |
| any selective mode | `populate=false` | mode is lost |
That is the correct reading for v9, which has no code that reads
`_hoodie_commit_time` selectively — presenting the table as having no meta
columns is the only honest thing it can say. Files keep their populated
columns; only how the table advertises itself changes. Critically the direction
is safe: the table **under-claims**. Over-claiming is the bug, and that is
exactly what the write-back prevents (@voonhous's original catch on the `true`
default).
@voonhous — on your one-way concern specifically: a selective mode still
cannot survive a v10 → v9 → v10 round trip. It returns as `NONE`, and hudi-cli
cannot widen it back, so recreating the table is the only recovery. I think
that is better handled as a documented, warned, and *tested* lossy path than as
a hard failure the operator cannot work around — but it is your call, and
`reUpgradeAfterDowngradingASelectiveModeResolvesToNone` now pins it rather than
leaving it implicit.
`TestTenToNineDowngradeHandler` is 14/14 and the full upgrade/downgrade
suite 84/84.
--
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]
