voonhous commented on code in PR #19205:
URL: https://github.com/apache/hudi/pull/19205#discussion_r3665543517
##########
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:
No, it is load-bearing, and this PR already has the test that proves it:
`TestBaseHoodieWriteClient.java:129-141`
(`validateAgainstTablePropertiesAllowsUnstatedWriterToNarrow`) -- table `ALL`,
writer sets only `withPopulateMetaFields(false)`, so `writeMetaFieldsMode` is
`NONE` and differs from the table's `ALL` while `writerStatedMode` is `false`.
That path has always been legal: the pre-PR check was one-directional
(`!table.populateMetaFields() && write.populateMetaFields()`), added in
`d5026e9a2485` [HUDI-2161]. Dropping the flag would start throwing for every
caller that builds a write config without restating the table's meta-field
settings.
That said, the flag is currently **mis-scoped** rather than redundant -- see
my comment on `:1572`. It should gate only `ALL` tables; for a selective table
the disagreement has to be rejected whether or not the writer stated a mode.
##########
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:
Yes, narrowing is deliberately allowed, and the code does match the comment.
Full matrix, with S = "writer explicitly set `hoodie.meta.fields.mode`":
- **S = true:** passes iff writer mode == table mode; all 20 off-diagonal
cells throw (that is the `:1580` branch).
- **S = false:** the writer mode can only be `ALL` or `NONE`, since it is
derived from the legacy boolean.
- `ALL`: throws for every table mode except `ALL` (caught by `isWiderThan`
via the record-key clause).
- `NONE`: passes against **all five** table modes.
So `:1573` is what allows narrowing, and it is only reached when S is false.
Of those `NONE` cells, `ALL` and `NONE` are correct and long-standing. The
three selective ones are the bug I flagged on `:1572` -- they let a writer
silently write null `_hoodie_commit_time` onto a table that still advertises
`COMMIT_TIME_ONLY`.
Worth noting the relation is not a total order: `COMMIT_TIME_ONLY` and
`FILE_NAME_ONLY` are each wider than the other, so transitions between them are
rejected in both directions. Nothing currently tests that.
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/table/upgrade/NineToTenUpgradeHandler.java:
##########
@@ -34,6 +46,12 @@ public UpgradeDowngrade.TableConfigChangeSet upgrade(
HoodieEngineContext context,
String instantTime,
SupportsUpgradeDowngrade upgradeDowngradeHelper) {
- return new UpgradeDowngrade.TableConfigChangeSet();
+ HoodieTableConfig tableConfig =
+ upgradeDowngradeHelper.getTable(config,
context).getMetaClient().getTableConfig();
+ // Resolves from the legacy boolean for a version 9 table, since the mode
property is absent.
+ MetaFieldsMode metaFieldsMode = tableConfig.getMetaFieldsMode();
+ Map<ConfigProperty, String> propertiesToUpdate = Collections.singletonMap(
+ HoodieTableConfig.META_FIELDS_MODE, metaFieldsMode.name());
+ return new UpgradeDowngrade.TableConfigChangeSet(propertiesToUpdate,
Collections.emptySet());
Review Comment:
I would keep it, and I think this one deserves an explicit comment saying
why, because the symmetry argument points the other way.
`EightToNineUpgradeHandler` does remove the legacy property (`:171-172`,
`:240-241`), so the convention says remove it here too. The difference is the
default: `PAYLOAD_CLASS_NAME` and `PRECOMBINE_FIELD` have no default, but
`POPULATE_META_FIELDS` defaults to `true` (`HoodieTableConfig.java:338-341`).
So if the upgrade removed the boolean and the table were later downgraded, a
v9 reader would find neither property and resolve to `ALL` -- claiming meta
columns on files that do not have them. That is exactly the widening this PR
bans everywhere else.
Suggest keeping it and noting the default-value exception in the class
javadoc, so the deviation reads as deliberate rather than an oversight.
##########
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:
Yes, and I think this is the right call -- it also closes a hole. Details on
`:72`.
The precedent is `NineToEightDowngradeHandler.java:116-117` and `:152-153`:
downgrade restores the legacy property from the new one, then removes the new
one. Here the mode is deleted and nothing is written back.
For `ALL`/`NONE` writing the derived boolean is a no-op, so it costs
nothing. What it fixes is the case where a table carries the mode without the
boolean -- `POPULATE_META_FIELDS` then falls back to its `true` default and the
table downgrades to `ALL`, i.e. Hudi believes `_hoodie_record_key` is populated
on files where it is null.
```java
propertiesToUpdate.put(HoodieTableConfig.POPULATE_META_FIELDS.key(),
String.valueOf(metaFieldsMode.toLegacyPopulateMetaFields()));
```
Separate but related: selective modes are destroyed here with only a
`LOG.warn`, and they cannot be restored afterwards, because a re-upgrade
derives `NONE` and `isWiderThan` then rejects setting the mode back. No other
handler pair in this package is one-way like that -- worth either throwing or
documenting it as intentional.
##########
hudi-client/hudi-client-common/src/test/java/org/apache/hudi/table/upgrade/TestTenToNineDowngradeHandler.java:
##########
@@ -24,18 +24,25 @@
import org.junit.jupiter.api.Test;
import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertTrue;
class TestTenToNineDowngradeHandler {
@Test
- void testDowngradeRemovesStorageLayoutOnly() {
+ void testDowngradeRemovesStorageLayoutAndMetaFieldsMode() {
UpgradeDowngrade.TableConfigChangeSet changeSet =
new TenToNineDowngradeHandler().downgrade(null, null, null, null);
assertTrue(changeSet.propertiesToUpdate().isEmpty());
- assertEquals(1, changeSet.propertiesToDelete().size());
+ assertEquals(2, changeSet.propertiesToDelete().size());
assertTrue(changeSet.propertiesToDelete().contains(HoodieTableConfig.TABLE_STORAGE_LAYOUT));
+ // Version 9 does not understand hoodie.meta.fields.mode, so it is
dropped...
+
assertTrue(changeSet.propertiesToDelete().contains(HoodieTableConfig.META_FIELDS_MODE));
+ // ...while hoodie.populate.meta.fields is deliberately left in place, so
ALL and NONE tables
+ // round-trip unchanged — those are exactly the two states the legacy
boolean can express.
Review Comment:
Keep both and translate on both edges -- that is what the rest of the
package does (`EightToNineUpgradeHandler.java:171-172` on the way up,
`NineToEightDowngradeHandler.java:116-117` on the way down).
The one exception is that the upgrade should **not** delete
`hoodie.populate.meta.fields`, because unlike `PAYLOAD_CLASS_NAME` it has a
`true` default -- deleting it makes a v9 reader see `ALL`. I have written that
up on `NineToTenUpgradeHandler.java:55`.
So: upgrade writes the mode and leaves the boolean; downgrade writes the
derived boolean and deletes the mode. That makes the round trip lossless for
`ALL`/`NONE` and removes the silent-widening case.
This test will also need a selective-mode case -- right now it passes `null`
for the helper, so the handler short-circuits to `ALL` and the whole selective
branch is never executed.
--
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]
