nsivabalan commented on code in PR #19205:
URL: https://github.com/apache/hudi/pull/19205#discussion_r3735257121
##########
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:
Fixed in `664ff2ec23ae`, and the forward-compat consequence you described is
the part that mattered.
Whenever an explicit mode is supplied, the legacy boolean is **derived from
it** (`ALL -> true`, everything else `-> false`) rather than taken from the
caller, at both chokepoints: `HoodieTableMetaClient.TableBuilder` (which every
creation path goes through, including `HoodieSparkSqlWriter` and `StreamSync`)
and `HoodieWriteConfig.Builder`. So `hoodie.properties` can never contradict
the mode, and a pre-1.3.0 reader — which sees only the boolean — reads `false`
for a selective table rather than assuming meta columns that are null.
I went with "always persist the derived value" over "reject the combination"
because rejecting would make `populate.meta.fields=true` + `ALL` an error,
which is a coherent request, and because the derived write also covers the case
where the boolean is simply absent. The resolution table is now spelled out in
the PR description.
On the test: you were right that `populateTrueWithSelectiveModeIsRejected`
was failing, and I should own how that happened — CI flagged it and I rewrote
it to accept the new behavior, reading it as stale rather than as the signal it
was. It is now `selectiveModeWinsOverLegacyPopulateTrue` and additionally
asserts the persisted boolean is `false`, which is the invariant that was
actually broken.
##########
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:
Fixed in `c266b8286b4e`, and strengthened further in the latest push.
`validateAgainstTableProperties` compares the full `MetaFieldsMode` rather
than the collapsed booleans, so your scenario — a direct `SparkRDDWriteClient`
opening a persisted `COMMIT_TIME_ONLY` table with `populate=false` and no mode
— is caught. You were right that this is a *narrowing*, not a widening, so the
`isWiderThan` guard alone would not have caught it: `NONE` is not wider than
`COMMIT_TIME_ONLY`.
What changed since: the writer no longer resolves to `NONE` there at all. A
writer that states neither meta-field property now **inherits** the table's
mode, because the mode is a table property that only hudi-cli or an upgrade may
change. A writer that states either is compared and rejected in both
directions. So your case now either inherits `COMMIT_TIME_ONLY` (if nothing was
stated) or fails loudly (if `populate=false` was stated) — never silently
writes null commit times.
`TestBaseHoodieWriteClient` has 110 tests covering these combinations,
including the exact 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:
Implemented in `69a0906419d8`, and then taken further in the latest push —
worth re-reading, since your closing suggestion is now what the code does.
The write-back is in: downgrade derives `POPULATE_META_FIELDS` from the mode
(`ALL -> true`, everything else `-> false`) and deletes the mode, mirroring
`NineToEightDowngradeHandler:116-117`. For `ALL`/`NONE` that is a no-op
restatement; what it closes is the case @voonhous described, where a table
carrying only the mode would fall back to the `true` default and downgrade to
`ALL`.
On the one-way concern you both raised: **selective modes now throw** rather
than being destroyed with a `LOG.warn`. You were right that no other handler
pair in this package is one-way, and that a warning is not an acknowledgement.
Since `ALL` and `NONE` are exactly the two states v9 can express, a selective
table is now told to rewrite to one of them before downgrading — which makes
the unrecoverable state unreachable rather than documented.
Also fixed the adjacent issue: with no `SupportsUpgradeDowngrade` helper the
handler assumed `ALL` and wrote a derived boolean from that guess. It now
leaves the boolean untouched.
##########
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:
Implemented in `69a0906419d8`, and then taken further in the latest push —
worth re-reading, since your closing suggestion is now what the code does.
The write-back is in: downgrade derives `POPULATE_META_FIELDS` from the mode
(`ALL -> true`, everything else `-> false`) and deletes the mode, mirroring
`NineToEightDowngradeHandler:116-117`. For `ALL`/`NONE` that is a no-op
restatement; what it closes is the case @voonhous described, where a table
carrying only the mode would fall back to the `true` default and downgrade to
`ALL`.
On the one-way concern you both raised: **selective modes now throw** rather
than being destroyed with a `LOG.warn`. You were right that no other handler
pair in this package is one-way, and that a warning is not an acknowledgement.
Since `ALL` and `NONE` are exactly the two states v9 can express, a selective
table is now told to rewrite to one of them before downgrading — which makes
the unrecoverable state unreachable rather than documented.
Also fixed the adjacent issue: with no `SupportsUpgradeDowngrade` helper the
handler assumed `ALL` and wrote a derived boolean from that guess. It now
leaves the boolean untouched.
##########
hudi-client/hudi-spark-client/src/main/java/org/apache/hudi/io/storage/HoodieSparkFileWriterFactory.java:
##########
@@ -56,7 +56,9 @@ public HoodieSparkFileWriterFactory(HoodieStorage storage) {
protected HoodieFileWriter newParquetFileWriter(
String instantTime, StoragePath path, HoodieConfig config, HoodieSchema
schema,
TaskContextSupplier taskContextSupplier) throws IOException {
- boolean populateMetaFields =
config.getBooleanOrDefault(HoodieTableConfig.POPULATE_META_FIELDS);
+ org.apache.hudi.common.model.MetaFieldsMode metaFieldsMode =
Review Comment:
Fixed in `5f397915d1d8` — imported in both `HoodieSparkFileWriterFactory`
and `HoodieAvroFileWriterFactory`, and the two `resolve(config)` call sites use
the simple name.
Note the stacked #19378 adds two more fully-qualified references in this
file (the Lance and Vortex paths); I will fold those into the simple name there
so the file does not end up mixed.
--
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]
