voonhous commented on code in PR #19205:
URL: https://github.com/apache/hudi/pull/19205#discussion_r3665540965
##########
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:
This guard tests whether the property is *present* on disk, not whether it
*disagrees*. So a write that passes `hoodie.meta.fields.mode=ALL` to an
ordinary default table throws, even though it is asking for exactly what the
table already is. Same for `NONE` against a `populate.meta.fields=false` table.
There is a second-order problem: the property is only ever backfilled by
`NineToTenUpgradeHandler`, and `HoodieTableVersion.current()` is already `TEN`.
A table already at v10 without the property will never be upgraded again, so it
can never adopt the explicit property at all.
Compare resolved modes instead:
```scala
if (paramsMetaFieldsMode.nonEmpty
&& MetaFieldsMode.parse(paramsMetaFieldsMode) !=
tableConfig.getMetaFieldsMode) {
```
That keeps the intended guard -- a legacy table resolves to `ALL`/`NONE`, so
`null -> COMMIT_TIME_ONLY` still throws -- and drops the false positive. Worth
a test that `mode=ALL` on a table with only `populate.meta.fields=true`
succeeds.
##########
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:
`withMetaFieldsMode` re-derives the boolean, but `withPopulateMetaFields`
does not re-derive the mode -- so the invariant this comment claims holds in
only one call order:
```
.withPopulateMetaFields(true).withMetaFieldsMode(COMMIT_TIME_ONLY)
-> populate.meta.fields = false (consistent)
.withMetaFieldsMode(COMMIT_TIME_ONLY).withPopulateMetaFields(true)
-> populate.meta.fields = true (contradictory)
```
The second order produces exactly the config `664ff2ec` was meant to make
impossible: a selective mode sitting next to `populate.meta.fields=true`.
`getMetaFieldsMode()` still resolves correctly because the mode wins, but any
path that copies raw write-config props into `hoodie.properties` carries the
contradiction to disk.
Cheapest fix is to re-derive in `build()` rather than in the setter, so
order stops mattering. Then `TestHoodieWriteConfigMetaFieldsMode:122` can
assert the raw property in both orders, which it currently skips for the order
that does not hold.
##########
hudi-common/src/main/java/org/apache/hudi/common/model/MetaFieldsMode.java:
##########
@@ -0,0 +1,191 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hudi.common.model;
+
+import org.apache.hudi.common.config.HoodieConfig;
+import org.apache.hudi.common.table.HoodieTableConfig;
+import org.apache.hudi.common.util.StringUtils;
+
+import java.util.Locale;
+
+/**
+ * Which of Hudi's meta columns are physically populated on disk.
+ *
+ * <p>Selective modes exist so that tables that opt out of the default {@code
populate.meta.fields=true}
+ * can still keep the two columns that matter for downstream operations
without paying for the other
+ * three:
+ *
+ * <ul>
+ * <li>{@code _hoodie_commit_time} — required for incremental queries.</li>
+ * <li>{@code _hoodie_file_name} — useful for file-level pruning /
investigation lookups.</li>
+ * </ul>
+ *
+ * <p>The remaining three meta columns ({@code _hoodie_commit_seqno}, {@code
_hoodie_record_key},
+ * {@code _hoodie_partition_path}) are all-or-nothing — either populate every
meta column ({@link #ALL})
+ * or none of them beyond the two selectable ones. If you need any of the
remaining columns, set
+ * {@code hoodie.populate.meta.fields=true}.
+ *
+ * <p>This enum is the single source of truth for meta-column population. The
legacy boolean
+ * {@code hoodie.populate.meta.fields} is deprecated and consulted only when
+ * {@code hoodie.meta.fields.mode} is absent, so that tables written before
the mode property
+ * existed keep their behavior:
+ *
+ * <ul>
+ * <li>{@code populate.meta.fields=true} (or absent) → {@link #ALL} —
today's default.</li>
+ * <li>{@code populate.meta.fields=false} → {@link #NONE}.</li>
+ * </ul>
+ *
+ * <p>On-disk representation: the enum {@link #name()} is persisted in {@code
hoodie.properties}
+ * under the property {@code hoodie.meta.fields.mode}.
+ */
+public enum MetaFieldsMode {
+ /**
+ * All five Hudi meta columns are populated — today's default.
+ */
+ ALL(true, true),
+
+ /**
+ * No Hudi meta columns are populated. Incremental queries are unsupported.
File-level pruning
+ * that depends on {@code _hoodie_file_name} is unsupported.
+ */
+ NONE(false, false),
+
+ /**
+ * Only {@code _hoodie_commit_time} is populated. Incremental queries remain
functional; other
+ * meta columns stay null on disk.
+ */
+ COMMIT_TIME_ONLY(true, false),
+
+ /**
+ * Only {@code _hoodie_file_name} is populated. Useful for file-level
lookups and debugging;
+ * incremental queries are unsupported.
+ */
+ FILE_NAME_ONLY(false, true),
+
+ /**
+ * Both {@code _hoodie_commit_time} and {@code _hoodie_file_name} are
populated.
+ */
+ COMMIT_TIME_AND_FILE_NAME(true, true);
+
+ private final boolean commitTimePopulated;
+ private final boolean fileNamePopulated;
+
+ MetaFieldsMode(boolean commitTimePopulated, boolean fileNamePopulated) {
+ this.commitTimePopulated = commitTimePopulated;
+ this.fileNamePopulated = fileNamePopulated;
+ }
+
+ public boolean isCommitTimePopulated() {
+ return commitTimePopulated;
+ }
+
+ public boolean isFileNamePopulated() {
+ return fileNamePopulated;
+ }
+
+ /**
+ * @return true when all five meta columns are populated (i.e. this is
{@link #ALL}). Selective
+ * modes never populate {@code _hoodie_record_key}, {@code
_hoodie_partition_path}, or
+ * {@code _hoodie_commit_seqno}.
+ */
+ public boolean isRecordKeyPopulated() {
+ return this == ALL;
+ }
+
+ /**
+ * Resolve the effective mode. {@code hoodie.meta.fields.mode} is the source
of truth; the
+ * deprecated {@code hoodie.populate.meta.fields} boolean is a fallback for
tables written before
+ * the mode property existed. Precedence:
+ *
+ * <ul>
+ * <li>non-empty mode → the parsed enum value (the legacy boolean is not
consulted).</li>
+ * <li>null/empty mode + {@code populateMetaFields=false} → {@link
#NONE}.</li>
+ * <li>null/empty mode + {@code populateMetaFields=true} → {@link
#ALL}.</li>
+ * </ul>
+ *
+ * @param rawMode raw {@code hoodie.meta.fields.mode} value; may
be null or empty.
+ * @param legacyPopulateMetaFields value of the deprecated {@code
hoodie.populate.meta.fields}.
+ * @throws IllegalArgumentException when the raw mode value does not match
any enum value. This
+ * includes the pre-enum comma-separated format — callers that
upgrade an old table must
+ * migrate the value through the hudi-cli.
+ */
+ /**
+ * Resolve the effective mode from any {@link HoodieConfig} that may carry
the two properties —
+ * a table config, a write config, or a bare config built from write
options. Preferred over the
+ * two-argument overload: it keeps the property keys and the precedence rule
in one place instead
+ * of repeating them at every call site.
+ */
+ public static MetaFieldsMode resolve(HoodieConfig config) {
+ return
resolve(config.getStringOrDefault(HoodieTableConfig.META_FIELDS_MODE),
+ config.getBooleanOrDefault(HoodieTableConfig.POPULATE_META_FIELDS));
+ }
+
+ public static MetaFieldsMode resolve(String rawMode, boolean
legacyPopulateMetaFields) {
+ if (StringUtils.isNullOrEmpty(rawMode)) {
+ return legacyPopulateMetaFields ? ALL : NONE;
+ }
+ return parse(rawMode);
+ }
+
+ /**
+ * Parse a raw {@code hoodie.meta.fields.mode} value into an enum constant,
with a message that
+ * lists the allowed values. Prefer this over {@link #valueOf(String)} for
user-supplied input.
+ */
+ public static MetaFieldsMode parse(String rawMode) {
+ try {
+ // Case-insensitive: users hand-editing hoodie.properties or passing
write options should not
+ // have to match the enum's casing exactly.
+ return MetaFieldsMode.valueOf(rawMode.trim().toUpperCase(Locale.ROOT));
+ } catch (IllegalArgumentException e) {
+ throw new IllegalArgumentException(String.format(
+ "Unsupported value '%s' for hoodie.meta.fields.mode. Allowed values:
%s, %s, %s, %s, %s.",
+ rawMode, ALL, NONE, COMMIT_TIME_ONLY, FILE_NAME_ONLY,
COMMIT_TIME_AND_FILE_NAME), e);
+ }
+ }
+
+ /**
+ * @return the equivalent value of the deprecated {@code
hoodie.populate.meta.fields} boolean, so
+ * that call sites not yet migrated to this enum keep observing consistent
behavior.
+ */
+ public boolean toLegacyPopulateMetaFields() {
+ return this == ALL;
+ }
+
+ /**
+ * @return true when this mode populates at least one meta column that
{@code other} does not.
+ *
+ * <p>Meta-field population is a physical-storage decision baked into files
at write time, so it
+ * can never be widened for an existing table: earlier commits would be
missing columns that later
+ * commits have, and readers cannot tell the two apart. Every transition
that adds a column is
+ * therefore rejected — {@code NONE -> COMMIT_TIME_ONLY} and
+ * {@code FILE_NAME_ONLY -> COMMIT_TIME_AND_FILE_NAME} just as much as
{@code NONE -> ALL}.
+ *
+ * <p>Narrowing is not flagged here: writing fewer meta columns than the
table advertises cannot
+ * make a reader believe in data that is absent, and it is long-standing
behavior for a writer to
+ * resolve to {@link #NONE} against an {@link #ALL} table without restating
its settings.
+ */
+ public boolean isWiderThan(MetaFieldsMode other) {
Review Comment:
`isWiderThan` has exactly one caller (`BaseHoodieWriteClient.java:1573`) and
no direct unit test, and `TestBaseHoodieWriteClient` never mentions
`FILE_NAME_ONLY` or `COMMIT_TIME_AND_FILE_NAME` -- so two of the three clauses
are mutation-survivable. Deleting the `isRecordKeyPopulated()` clause changes
exactly one pair (`ALL` vs `COMMIT_TIME_AND_FILE_NAME`) that nothing tests.
The interesting case is that the relation is not a total order:
`COMMIT_TIME_ONLY` and `FILE_NAME_ONLY` are each wider than the other, so a
transition between them is rejected in **both** directions. That is correct,
and it is exactly what makes the narrowing question subtle -- worth pinning.
Suggest two cases: `COMMIT_TIME_AND_FILE_NAME` writer vs `ALL` table
(allowed) and the reverse (rejected), plus `COMMIT_TIME_ONLY` vs
`FILE_NAME_ONLY` rejected both ways.
##########
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:
nit: `MetaFieldsMode` is referenced fully qualified here (and in
`HoodieAvroFileWriterFactory.java:72-73`) while every other file this PR
touches imports it. Add the import for consistency.
##########
hudi-utilities/src/test/java/org/apache/hudi/utilities/deltastreamer/TestHoodieStreamerMetaFieldsMode.java:
##########
@@ -0,0 +1,139 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.hudi.utilities.deltastreamer;
+
+import org.apache.hudi.common.model.HoodieRecord;
+import org.apache.hudi.common.model.MetaFieldsMode;
+import org.apache.hudi.common.model.WriteOperationType;
+import org.apache.hudi.common.table.HoodieTableConfig;
+import org.apache.hudi.common.table.HoodieTableMetaClient;
+import org.apache.hudi.common.testutils.HoodieTestUtils;
+
+import org.apache.spark.sql.Dataset;
+import org.apache.spark.sql.Row;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.EnumSource;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+/**
+ * End-to-end coverage for {@code hoodie.meta.fields.mode} through the
HoodieStreamer entrypoint.
+ * Each parameterized invocation runs a single ingest cycle in the given
{@link MetaFieldsMode} and
+ * verifies both the persisted table property and the actual on-disk parquet
column population.
+ *
+ * <p>Rejection paths (unknown token, populate=true+mode, MoR+mode) are
exercised in the datasource
+ * test {@code TestMetaFieldsMode}; this fixture focuses on the streamer
control-flow.
+ */
+public class TestHoodieStreamerMetaFieldsMode extends
HoodieDeltaStreamerTestBase {
Review Comment:
This class costs a full streamer fixture but does not test the
streamer-specific thing.
- The only new streamer production code is `StreamSync.java:481-482`. For
`ALL` and `NONE` the mode key is absent from `cfg.configs`, so
`setMetaFieldsModeFromString("")` is a no-op -- 2 of the 5 parameterized runs
exercise zero new code.
- `testStreamerRejectsMorWithSelectiveMode` is the third copy of one
`checkArgument` (also in `TestMetaFieldsMode` and
`TestHoodieWriteConfigMetaFieldsMode`), and this class's own javadoc at
`:46-47` says rejection paths are covered elsewhere.
- The regression this change exists for -- cshuo's "restart with only
`populate=false` downgrades `COMMIT_TIME_ONLY` to `NONE`" -- is **not** tested.
There is only one `ingestOnce()` in the file, so no restart ever happens.
Suggest deleting this class and adding one method to
`TestHoodieDeltaStreamer`: create with `COMMIT_TIME_ONLY`, `ingestOnce()`, then
build a second streamer with only `POPULATE_META_FIELDS=false` and
`ingestOnce()` again, asserting the mode is still `COMMIT_TIME_ONLY`. Given the
`BaseHoodieWriteClient` finding, I expect that test to fail today -- which is
the point.
##########
hudi-spark-datasource/hudi-spark/src/test/scala/org/apache/hudi/TestHoodieSparkSqlWriterWithTestFormat.scala:
##########
@@ -106,7 +106,7 @@ class TestHoodieSparkSqlWriterWithTestFormat extends
HoodieSparkWriterTestBase {
// fetch all records from parquet files generated from write to hudi
val actualDf = sqlContext.read.parquet(fullPartitionPaths(0),
fullPartitionPaths(1), fullPartitionPaths(2))
if (!populateMetaFields) {
- List(0, 1, 2, 3, 4).foreach(i => assertEquals(0,
actualDf.select(HoodieRecord.HOODIE_META_COLUMNS.get(i)).filter(entry =>
!(entry.mkString(",").equals(""))).count()))
+ List(0, 1, 2, 3, 4).foreach(i => assertEquals(0,
actualDf.select(HoodieRecord.HOODIE_META_COLUMNS.get(i)).filter(entry =>
!entry.isNullAt(0) && entry.getString(0).nonEmpty).count()))
Review Comment:
This update is correct -- the old form would fail on NULL, since
`Row.mkString` renders null as the string "null" -- but it relaxed further than
the behaviour change required. `!entry.isNullAt(0) &&
entry.getString(0).nonEmpty` accepts **both** NULL and `""`, so it no longer
pins which representation is written.
That is worth keeping strict, because the whole point of `77f4e9148c4a` is
that these are now NULL, and `TestMetaFieldsMode.java:149` asserts exactly
`assertNull` for the same scenario in this same PR. Right now the two tests
disagree on strictness.
```suggestion
List(0, 1, 2, 3, 4).foreach(i => assertEquals(0,
actualDf.select(HoodieRecord.HOODIE_META_COLUMNS.get(i)).filter(entry =>
!entry.isNullAt(0)).count()))
```
Since this exact edit is being made in two near-identical files, a shared
`assertNoMetaFieldsPopulated(df)` in `HoodieSparkWriterTestBase` (next to
`dropMetaFields`) would be better than duplicating it.
##########
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:
This breaks the symmetry convention this package already established, and
the result is lossy.
The precedent, from `b60d38c40fa1` [HUDI-8401]:
- upgrade adds the new property and **removes** the legacy one --
`EightToNineUpgradeHandler.java:171-172` and `:240-241`
- downgrade **restores** the legacy one from the new one and removes the new
one -- `NineToEightDowngradeHandler.java:116-117` and `:152-153`
Here the downgrade deletes `hoodie.meta.fields.mode` and deliberately writes
nothing back. Two consequences:
1. If a table ever carries the mode without the boolean,
`POPULATE_META_FIELDS` defaults to `true`, so the table downgrades to `ALL` --
Hudi then believes `_hoodie_record_key` is populated on files where it is
physically null.
2. A selective mode is unrecoverable. Downgrade drops it, re-upgrade derives
`NONE` from the boolean, and `isWiderThan` now rejects any writer trying to
restore `COMMIT_TIME_ONLY` as a widening. No other handler pair in this package
is one-way like that.
Suggest adding the derived boolean here, mirroring
`NineToEightDowngradeHandler.java:117`:
```java
propertiesToUpdate.put(HoodieTableConfig.POPULATE_META_FIELDS.key(),
String.valueOf(metaFieldsMode.toLegacyPopulateMetaFields()));
```
For `ALL`/`NONE` that is a no-op, so nothing regresses; it just closes the
hole. (This also answers your open question at `:76` -- yes, infer and add it.)
Separately: a selective mode is silently destroyed here with only a
`LOG.warn`. Consider throwing unless the caller opts in, since the operator
gets no acknowledgement today.
##########
hudi-spark-datasource/hudi-spark/src/test/java/org/apache/hudi/functional/TestMetaFieldsMode.java:
##########
@@ -0,0 +1,469 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.hudi.functional;
+
+import org.apache.hudi.DataSourceWriteOptions;
+import org.apache.hudi.SparkAdapterSupport$;
+import org.apache.hudi.common.config.HoodieMetadataConfig;
+import org.apache.hudi.common.model.HoodieRecord;
+import org.apache.hudi.common.model.MetaFieldsMode;
+import org.apache.hudi.common.table.HoodieTableConfig;
+import org.apache.hudi.common.table.HoodieTableMetaClient;
+import org.apache.hudi.testutils.SparkClientFunctionalTestHarness;
+
+import org.apache.spark.sql.Dataset;
+import org.apache.spark.sql.Row;
+import org.apache.spark.sql.RowFactory;
+import org.apache.spark.sql.SaveMode;
+import org.apache.spark.sql.types.DataTypes;
+import org.apache.spark.sql.types.StructField;
+import org.apache.spark.sql.types.StructType;
+import org.junit.jupiter.api.Test;
+
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+/**
+ * Spark-datasource end-to-end tests for the {@code hoodie.meta.fields.mode}
property on CoW tables.
+ * Every {@link MetaFieldsMode} value is exercised via a write / re-read round
trip; on-disk column
+ * population is verified by reading the parquet files back and inspecting the
meta-column values.
+ */
+class TestMetaFieldsMode extends SparkClientFunctionalTestHarness {
+
+ private static StructType simpleSchema() {
+ return DataTypes.createStructType(new StructField[]{
+ DataTypes.createStructField("column1", DataTypes.StringType, true),
+ DataTypes.createStructField("column2", DataTypes.StringType, true),
+ DataTypes.createStructField("column3", DataTypes.StringType, true)
+ }).asNullable();
+ }
+
+ private Map<String, String> baseOptions() {
+ Map<String, String> opts = new HashMap<>();
+ opts.put(DataSourceWriteOptions.RECORDKEY_FIELD().key(), "column1");
+ opts.put(DataSourceWriteOptions.PARTITIONPATH_FIELD().key(), "column2");
+ opts.put(DataSourceWriteOptions.ORDERING_FIELDS().key(), "column3");
+ opts.put(HoodieTableConfig.NAME.key(), "test_meta_fields_mode");
+ opts.put(DataSourceWriteOptions.TABLE_TYPE().key(), "COPY_ON_WRITE");
+ opts.put(HoodieMetadataConfig.ENABLE.key(), "false");
Review Comment:
The metadata table is disabled in every functional test here, but it is on
by default in production.
I checked and the MDT write config is built fresh
(`HoodieMetadataWriteUtils.createMetadataWriteConfig`) so
`hoodie.meta.fields.mode` does not leak into it and the new MoR gate cannot
false-fire -- but nothing in the PR proves that, and it is one
`withProps(dataWriteConfig.getProps())` refactor away from turning every
MDT-enabled selective write into a hard `checkArgument` failure.
Suggest running at least one selective-mode case with MDT enabled, asserting
the write succeeds and the MDT table config resolves to `NONE`.
##########
hudi-common/src/main/java/org/apache/hudi/common/model/MetaFieldsMode.java:
##########
@@ -0,0 +1,191 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hudi.common.model;
+
+import org.apache.hudi.common.config.HoodieConfig;
+import org.apache.hudi.common.table.HoodieTableConfig;
+import org.apache.hudi.common.util.StringUtils;
+
+import java.util.Locale;
+
+/**
+ * Which of Hudi's meta columns are physically populated on disk.
+ *
+ * <p>Selective modes exist so that tables that opt out of the default {@code
populate.meta.fields=true}
+ * can still keep the two columns that matter for downstream operations
without paying for the other
+ * three:
+ *
+ * <ul>
+ * <li>{@code _hoodie_commit_time} — required for incremental queries.</li>
+ * <li>{@code _hoodie_file_name} — useful for file-level pruning /
investigation lookups.</li>
+ * </ul>
+ *
+ * <p>The remaining three meta columns ({@code _hoodie_commit_seqno}, {@code
_hoodie_record_key},
+ * {@code _hoodie_partition_path}) are all-or-nothing — either populate every
meta column ({@link #ALL})
+ * or none of them beyond the two selectable ones. If you need any of the
remaining columns, set
+ * {@code hoodie.populate.meta.fields=true}.
+ *
+ * <p>This enum is the single source of truth for meta-column population. The
legacy boolean
+ * {@code hoodie.populate.meta.fields} is deprecated and consulted only when
+ * {@code hoodie.meta.fields.mode} is absent, so that tables written before
the mode property
+ * existed keep their behavior:
+ *
+ * <ul>
+ * <li>{@code populate.meta.fields=true} (or absent) → {@link #ALL} —
today's default.</li>
+ * <li>{@code populate.meta.fields=false} → {@link #NONE}.</li>
+ * </ul>
+ *
+ * <p>On-disk representation: the enum {@link #name()} is persisted in {@code
hoodie.properties}
+ * under the property {@code hoodie.meta.fields.mode}.
+ */
+public enum MetaFieldsMode {
+ /**
+ * All five Hudi meta columns are populated — today's default.
+ */
+ ALL(true, true),
+
+ /**
+ * No Hudi meta columns are populated. Incremental queries are unsupported.
File-level pruning
+ * that depends on {@code _hoodie_file_name} is unsupported.
+ */
+ NONE(false, false),
+
+ /**
+ * Only {@code _hoodie_commit_time} is populated. Incremental queries remain
functional; other
+ * meta columns stay null on disk.
+ */
+ COMMIT_TIME_ONLY(true, false),
+
+ /**
+ * Only {@code _hoodie_file_name} is populated. Useful for file-level
lookups and debugging;
+ * incremental queries are unsupported.
+ */
+ FILE_NAME_ONLY(false, true),
+
+ /**
+ * Both {@code _hoodie_commit_time} and {@code _hoodie_file_name} are
populated.
+ */
+ COMMIT_TIME_AND_FILE_NAME(true, true);
+
+ private final boolean commitTimePopulated;
+ private final boolean fileNamePopulated;
+
+ MetaFieldsMode(boolean commitTimePopulated, boolean fileNamePopulated) {
+ this.commitTimePopulated = commitTimePopulated;
+ this.fileNamePopulated = fileNamePopulated;
+ }
+
+ public boolean isCommitTimePopulated() {
+ return commitTimePopulated;
+ }
+
+ public boolean isFileNamePopulated() {
+ return fileNamePopulated;
+ }
+
+ /**
+ * @return true when all five meta columns are populated (i.e. this is
{@link #ALL}). Selective
+ * modes never populate {@code _hoodie_record_key}, {@code
_hoodie_partition_path}, or
+ * {@code _hoodie_commit_seqno}.
+ */
+ public boolean isRecordKeyPopulated() {
+ return this == ALL;
+ }
+
+ /**
+ * Resolve the effective mode. {@code hoodie.meta.fields.mode} is the source
of truth; the
+ * deprecated {@code hoodie.populate.meta.fields} boolean is a fallback for
tables written before
+ * the mode property existed. Precedence:
+ *
+ * <ul>
+ * <li>non-empty mode → the parsed enum value (the legacy boolean is not
consulted).</li>
+ * <li>null/empty mode + {@code populateMetaFields=false} → {@link
#NONE}.</li>
+ * <li>null/empty mode + {@code populateMetaFields=true} → {@link
#ALL}.</li>
+ * </ul>
+ *
+ * @param rawMode raw {@code hoodie.meta.fields.mode} value; may
be null or empty.
+ * @param legacyPopulateMetaFields value of the deprecated {@code
hoodie.populate.meta.fields}.
+ * @throws IllegalArgumentException when the raw mode value does not match
any enum value. This
+ * includes the pre-enum comma-separated format — callers that
upgrade an old table must
+ * migrate the value through the hudi-cli.
+ */
+ /**
Review Comment:
nit: two javadoc blocks are stacked here. The original `@param rawMode` /
`@param legacyPopulateMetaFields` / `@throws` block was left behind when
`9495687a` inserted the `resolve(HoodieConfig)` overload, so Java attaches only
the second one -- the documented precedence rules and the `@throws` contract
are dropped, and the two-arg `resolve` below ends up with no javadoc at all.
Not a build failure (`doclint` is off, `pom.xml:2112`). Just move the first
block down onto the two-arg overload.
##########
hudi-spark-datasource/hudi-spark/src/test/java/org/apache/hudi/functional/TestMetaFieldsMode.java:
##########
@@ -0,0 +1,469 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.hudi.functional;
+
+import org.apache.hudi.DataSourceWriteOptions;
+import org.apache.hudi.SparkAdapterSupport$;
+import org.apache.hudi.common.config.HoodieMetadataConfig;
+import org.apache.hudi.common.model.HoodieRecord;
+import org.apache.hudi.common.model.MetaFieldsMode;
+import org.apache.hudi.common.table.HoodieTableConfig;
+import org.apache.hudi.common.table.HoodieTableMetaClient;
+import org.apache.hudi.testutils.SparkClientFunctionalTestHarness;
+
+import org.apache.spark.sql.Dataset;
+import org.apache.spark.sql.Row;
+import org.apache.spark.sql.RowFactory;
+import org.apache.spark.sql.SaveMode;
+import org.apache.spark.sql.types.DataTypes;
+import org.apache.spark.sql.types.StructField;
+import org.apache.spark.sql.types.StructType;
+import org.junit.jupiter.api.Test;
+
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+/**
+ * Spark-datasource end-to-end tests for the {@code hoodie.meta.fields.mode}
property on CoW tables.
+ * Every {@link MetaFieldsMode} value is exercised via a write / re-read round
trip; on-disk column
+ * population is verified by reading the parquet files back and inspecting the
meta-column values.
+ */
+class TestMetaFieldsMode extends SparkClientFunctionalTestHarness {
Review Comment:
**The feature's headline promise has no test.** `MetaFieldsMode` documents
`COMMIT_TIME_ONLY` as "incremental queries remain functional", and that is the
entire justification for the mode -- but no test in this PR runs an incremental
read. Across all the new test files the only match for "incremental" is a
comment at `:224`.
That matters because the read path here is load-bearing and already bit once
*on this branch*: `13b39db9fb22` fixed `TableSchemaResolver` producing
**zero-row incremental reads** under a selective mode, and it shipped with 1
file changed and 0 tests. `358fbfdd717a` (the clustering file-name fix)
likewise: 1 file, 0 tests.
Suggest one test: write two commits to a `COMMIT_TIME_ONLY` CoW table, run
an incremental read with `START_COMMIT` = the first instant, assert exactly the
second commit's rows come back. Plus the negative -- the same query on
`FILE_NAME_ONLY` / `NONE` throws with the message from
`IncrementalRelationV1.scala:93-97`. This is the single highest-value test the
PR is missing.
##########
hudi-client/hudi-client-common/src/test/java/org/apache/hudi/config/TestHoodieWriteConfigMetaFieldsMode.java:
##########
@@ -0,0 +1,189 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hudi.config;
+
+import org.apache.hudi.common.model.HoodieTableType;
+import org.apache.hudi.common.model.MetaFieldsMode;
+import org.apache.hudi.common.table.HoodieTableConfig;
+
+import org.junit.jupiter.api.Test;
+
+import java.util.Properties;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+/**
+ * Validates the writer-side accessors and validation guards for the
meta-field-population modes
+ * on {@link HoodieWriteConfig}. Companion test for the {@link
HoodieTableConfig} accessors lives
+ * in {@code TestHoodieMetaFieldsMode}; this test covers the writer-builder
surface and the
+ * cross-flag validation that runs at {@code build()} time.
+ */
+class TestHoodieWriteConfigMetaFieldsMode {
Review Comment:
Most of this class re-tests `MetaFieldsMode.resolve` through a second facade
-- `HoodieWriteConfig.getMetaFieldsMode()` is a one-line delegation
(`HoodieWriteConfig.java:1794`), so `defaultsToAllMode`,
`explicitNoneModeBuilds`, `commitTimeOnlyModeBuilds`, `fileNameOnlyModeBuilds`,
`commitTimeAndFileNameCombinationBuilds`,
`explicitAllModeOverridesLegacyFalse`, `noneModeWithExplicitBuildIsStillNone`
and `legacyBooleanIsUsedWhenModeIsAbsent` all duplicate
`TestHoodieMetaFieldsMode` one hop away.
The three that earn their keep are the `validate()` tests. Those belong in
`TestHoodieWriteConfig`, which already hosts exactly this shape ~15 lines above
the new checks (`TestHoodieWriteConfig.java:129`).
Also a real gap: **the engine-type guard has zero coverage.** Every test
here uses the default builder, whose engine is `SPARK`, so deleting the
`engineType != EngineType.SPARK` check leaves the suite green. Worth one case
over `FLINK`/`JAVA`.
Suggest folding the three survivors into `TestHoodieWriteConfig` and
deleting this file.
##########
hudi-hadoop-common/src/test/java/org/apache/hudi/common/table/TestHoodieMetaFieldsMode.java:
##########
@@ -0,0 +1,145 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hudi.common.table;
+
+import org.apache.hudi.common.model.MetaFieldsMode;
+
+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.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+/**
+ * Tests the meta-field-population modes exposed by {@link HoodieTableConfig}
via the
+ * {@code hoodie.meta.fields.mode} property. The mode is materialized as a
{@link MetaFieldsMode}
+ * enum resolved from both the legacy {@code hoodie.populate.meta.fields}
boolean and the on-disk
+ * {@code hoodie.meta.fields.mode} property.
+ */
+class TestHoodieMetaFieldsMode {
Review Comment:
The class name says `MetaFieldsMode`, but every test constructs a
`HoodieTableConfig` and calls its accessors -- this is table-config resolution,
and `TestHoodieTableConfig` is the existing home for it (same package, same
module, and already edited by this PR at `:389`). It already hosts
pure-resolution tests with no storage, e.g. `:342`, `:356`, `:567`.
Suggest moving these nine methods into `TestHoodieTableConfig` and deleting
this file. If you want a genuine enum unit test, a small one covering `parse` /
`isWiderThan` / `toLegacyPopulateMetaFields` under
`hudi-common/.../common/model/` would be the right shape -- but that is a
different class from this one.
##########
hudi-spark-datasource/hudi-spark/src/test/java/org/apache/hudi/functional/TestMetaFieldsMode.java:
##########
@@ -0,0 +1,469 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.hudi.functional;
+
+import org.apache.hudi.DataSourceWriteOptions;
+import org.apache.hudi.SparkAdapterSupport$;
+import org.apache.hudi.common.config.HoodieMetadataConfig;
+import org.apache.hudi.common.model.HoodieRecord;
+import org.apache.hudi.common.model.MetaFieldsMode;
+import org.apache.hudi.common.table.HoodieTableConfig;
+import org.apache.hudi.common.table.HoodieTableMetaClient;
+import org.apache.hudi.testutils.SparkClientFunctionalTestHarness;
+
+import org.apache.spark.sql.Dataset;
+import org.apache.spark.sql.Row;
+import org.apache.spark.sql.RowFactory;
+import org.apache.spark.sql.SaveMode;
+import org.apache.spark.sql.types.DataTypes;
+import org.apache.spark.sql.types.StructField;
+import org.apache.spark.sql.types.StructType;
+import org.junit.jupiter.api.Test;
+
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+/**
+ * Spark-datasource end-to-end tests for the {@code hoodie.meta.fields.mode}
property on CoW tables.
+ * Every {@link MetaFieldsMode} value is exercised via a write / re-read round
trip; on-disk column
+ * population is verified by reading the parquet files back and inspecting the
meta-column values.
+ */
+class TestMetaFieldsMode extends SparkClientFunctionalTestHarness {
+
+ private static StructType simpleSchema() {
+ return DataTypes.createStructType(new StructField[]{
+ DataTypes.createStructField("column1", DataTypes.StringType, true),
+ DataTypes.createStructField("column2", DataTypes.StringType, true),
+ DataTypes.createStructField("column3", DataTypes.StringType, true)
+ }).asNullable();
+ }
+
+ private Map<String, String> baseOptions() {
+ Map<String, String> opts = new HashMap<>();
+ opts.put(DataSourceWriteOptions.RECORDKEY_FIELD().key(), "column1");
+ opts.put(DataSourceWriteOptions.PARTITIONPATH_FIELD().key(), "column2");
+ opts.put(DataSourceWriteOptions.ORDERING_FIELDS().key(), "column3");
+ opts.put(HoodieTableConfig.NAME.key(), "test_meta_fields_mode");
+ opts.put(DataSourceWriteOptions.TABLE_TYPE().key(), "COPY_ON_WRITE");
+ opts.put(HoodieMetadataConfig.ENABLE.key(), "false");
+ return opts;
+ }
+
+ private void writeRows(List<Row> records, StructType schema, Map<String,
String> options, String path, SaveMode mode) {
+ spark().createDataset(records,
+
SparkAdapterSupport$.MODULE$.sparkAdapter().getCatalystExpressionUtils().getEncoder(schema))
+ .write()
+ .format("hudi")
+ .options(options)
+ .mode(mode)
+ .save(path);
+ }
+
+ private HoodieTableConfig writeSampleAndGetTableConfig(Map<String, String>
options, String path) {
+ writeRows(Arrays.asList(
+ RowFactory.create("k1", "p1", "v1"),
+ RowFactory.create("k2", "p1", "v2")),
+ simpleSchema(), options, path, SaveMode.Overwrite);
Review Comment:
Every functional write in this class is `SaveMode.Overwrite`, i.e. a fresh
table each time. That skips the behaviour the feature is actually about:
- `HoodieSparkSqlWriter.scala:1102-1106` folds table props into the write
params **only when `mode != Overwrite`**, so the mode-inheritance path that
keeps an append-write on `COMMIT_TIME_ONLY` is never executed.
- The silent-narrowing bug I flagged on `BaseHoodieWriteClient` is only
reachable on a second write.
- Mixed-file behaviour across commits is never observed.
Suggest converting at least the `COMMIT_TIME_ONLY` case to: commit 1 with
`Overwrite`, commit 2 with `Append` and **without** restating the mode, then
assert the table config is still `COMMIT_TIME_ONLY` and that commit-2 files
have non-null `_hoodie_commit_time`.
##########
hudi-spark-datasource/hudi-spark/src/test/java/org/apache/hudi/functional/TestMetaFieldsMode.java:
##########
@@ -0,0 +1,469 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.hudi.functional;
+
+import org.apache.hudi.DataSourceWriteOptions;
+import org.apache.hudi.SparkAdapterSupport$;
+import org.apache.hudi.common.config.HoodieMetadataConfig;
+import org.apache.hudi.common.model.HoodieRecord;
+import org.apache.hudi.common.model.MetaFieldsMode;
+import org.apache.hudi.common.table.HoodieTableConfig;
+import org.apache.hudi.common.table.HoodieTableMetaClient;
+import org.apache.hudi.testutils.SparkClientFunctionalTestHarness;
+
+import org.apache.spark.sql.Dataset;
+import org.apache.spark.sql.Row;
+import org.apache.spark.sql.RowFactory;
+import org.apache.spark.sql.SaveMode;
+import org.apache.spark.sql.types.DataTypes;
+import org.apache.spark.sql.types.StructField;
+import org.apache.spark.sql.types.StructType;
+import org.junit.jupiter.api.Test;
+
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+/**
+ * Spark-datasource end-to-end tests for the {@code hoodie.meta.fields.mode}
property on CoW tables.
+ * Every {@link MetaFieldsMode} value is exercised via a write / re-read round
trip; on-disk column
+ * population is verified by reading the parquet files back and inspecting the
meta-column values.
+ */
+class TestMetaFieldsMode extends SparkClientFunctionalTestHarness {
+
+ private static StructType simpleSchema() {
+ return DataTypes.createStructType(new StructField[]{
+ DataTypes.createStructField("column1", DataTypes.StringType, true),
+ DataTypes.createStructField("column2", DataTypes.StringType, true),
+ DataTypes.createStructField("column3", DataTypes.StringType, true)
+ }).asNullable();
+ }
+
+ private Map<String, String> baseOptions() {
+ Map<String, String> opts = new HashMap<>();
+ opts.put(DataSourceWriteOptions.RECORDKEY_FIELD().key(), "column1");
+ opts.put(DataSourceWriteOptions.PARTITIONPATH_FIELD().key(), "column2");
+ opts.put(DataSourceWriteOptions.ORDERING_FIELDS().key(), "column3");
+ opts.put(HoodieTableConfig.NAME.key(), "test_meta_fields_mode");
+ opts.put(DataSourceWriteOptions.TABLE_TYPE().key(), "COPY_ON_WRITE");
+ opts.put(HoodieMetadataConfig.ENABLE.key(), "false");
+ return opts;
+ }
+
+ private void writeRows(List<Row> records, StructType schema, Map<String,
String> options, String path, SaveMode mode) {
+ spark().createDataset(records,
+
SparkAdapterSupport$.MODULE$.sparkAdapter().getCatalystExpressionUtils().getEncoder(schema))
+ .write()
+ .format("hudi")
+ .options(options)
+ .mode(mode)
+ .save(path);
+ }
+
+ private HoodieTableConfig writeSampleAndGetTableConfig(Map<String, String>
options, String path) {
+ writeRows(Arrays.asList(
+ RowFactory.create("k1", "p1", "v1"),
+ RowFactory.create("k2", "p1", "v2")),
+ simpleSchema(), options, path, SaveMode.Overwrite);
+ HoodieTableMetaClient metaClient =
+
HoodieTableMetaClient.builder().setBasePath(path).setConf(storageConf()).build();
+ return metaClient.getTableConfig();
+ }
+
+ /**
+ * End-to-end assertion of the on-disk meta columns after a write. Reads the
parquet files back
+ * (bypassing Hudi's own read path so we see the raw column values) and
asserts which meta
+ * columns are non-null.
+ */
+ private void assertMetaColumnPopulation(String path, MetaFieldsMode
expectedMode) {
+ Dataset<Row> raw = spark().read().parquet(path + "/*/*.parquet");
+ Row first = raw.select(
+ HoodieRecord.COMMIT_TIME_METADATA_FIELD,
+ HoodieRecord.COMMIT_SEQNO_METADATA_FIELD,
+ HoodieRecord.RECORD_KEY_METADATA_FIELD,
+ HoodieRecord.PARTITION_PATH_METADATA_FIELD,
+ HoodieRecord.FILENAME_METADATA_FIELD).first();
+
+ if (expectedMode.isCommitTimePopulated()) {
+ assertNotNull(first.get(0), "expected _hoodie_commit_time to be
populated for mode " + expectedMode);
+ } else {
+ assertNull(first.get(0), "expected _hoodie_commit_time to be null for
mode " + expectedMode);
+ }
+ if (expectedMode.isFileNamePopulated()) {
+ assertNotNull(first.get(4), "expected _hoodie_file_name to be populated
for mode " + expectedMode);
+ } else {
+ assertNull(first.get(4), "expected _hoodie_file_name to be null for mode
" + expectedMode);
+ }
+ // Record key, partition path, and commit seq no are ALL-only.
+ if (expectedMode == MetaFieldsMode.ALL) {
+ assertNotNull(first.get(2), "record key must be populated in ALL mode");
+ assertNotNull(first.get(3), "partition path must be populated in ALL
mode");
+ assertNotNull(first.get(1), "commit seq no must be populated in ALL
mode");
+ } else {
+ assertNull(first.get(2), "record key must be null outside ALL mode, got:
" + first.get(2));
+ assertNull(first.get(3), "partition path must be null outside ALL mode,
got: " + first.get(3));
+ assertNull(first.get(1), "commit seq no must be null outside ALL mode,
got: " + first.get(1));
+ }
+ }
+
+ @Test
+ void allModePersistsAndPopulatesAllColumns() {
+ Map<String, String> options = baseOptions();
+ // ALL is the default; no need to set the mode explicitly.
+ options.put(DataSourceWriteOptions.OPERATION().key(),
DataSourceWriteOptions.BULK_INSERT_OPERATION_OPT_VAL());
+
+ HoodieTableConfig tc = writeSampleAndGetTableConfig(options, basePath());
+
+ assertTrue(tc.populateMetaFields());
+ assertEquals(MetaFieldsMode.ALL, tc.getMetaFieldsMode());
+ assertMetaColumnPopulation(basePath(), MetaFieldsMode.ALL);
+ }
+
+ @Test
+ void noneModePersistsAndLeavesAllColumnsNull() {
+ Map<String, String> options = baseOptions();
+ options.put(HoodieTableConfig.POPULATE_META_FIELDS.key(), "false");
+ options.put(DataSourceWriteOptions.OPERATION().key(),
DataSourceWriteOptions.BULK_INSERT_OPERATION_OPT_VAL());
+
+ HoodieTableConfig tc = writeSampleAndGetTableConfig(options, basePath());
+
+ assertFalse(tc.populateMetaFields());
+ assertEquals(MetaFieldsMode.NONE, tc.getMetaFieldsMode());
+ assertMetaColumnPopulation(basePath(), MetaFieldsMode.NONE);
+ }
+
+ @Test
+ void commitTimeOnlyModePopulatesOnlyCommitTime() {
+ Map<String, String> options = baseOptions();
+ options.put(HoodieTableConfig.POPULATE_META_FIELDS.key(), "false");
+ options.put(HoodieTableConfig.META_FIELDS_MODE.key(),
MetaFieldsMode.COMMIT_TIME_ONLY.name());
+ options.put(DataSourceWriteOptions.OPERATION().key(),
DataSourceWriteOptions.BULK_INSERT_OPERATION_OPT_VAL());
+
+ HoodieTableConfig tc = writeSampleAndGetTableConfig(options, basePath());
+
+ assertEquals(MetaFieldsMode.COMMIT_TIME_ONLY.name(),
+ tc.getProps().getProperty(HoodieTableConfig.META_FIELDS_MODE.key()));
+ assertEquals(MetaFieldsMode.COMMIT_TIME_ONLY, tc.getMetaFieldsMode());
+ assertMetaColumnPopulation(basePath(), MetaFieldsMode.COMMIT_TIME_ONLY);
+ }
+
+ @Test
+ void fileNameOnlyModePopulatesOnlyFileName() {
+ Map<String, String> options = baseOptions();
+ options.put(HoodieTableConfig.POPULATE_META_FIELDS.key(), "false");
+ options.put(HoodieTableConfig.META_FIELDS_MODE.key(),
MetaFieldsMode.FILE_NAME_ONLY.name());
+ options.put(DataSourceWriteOptions.OPERATION().key(),
DataSourceWriteOptions.BULK_INSERT_OPERATION_OPT_VAL());
+
+ HoodieTableConfig tc = writeSampleAndGetTableConfig(options, basePath());
+
+ assertEquals(MetaFieldsMode.FILE_NAME_ONLY, tc.getMetaFieldsMode());
+ assertMetaColumnPopulation(basePath(), MetaFieldsMode.FILE_NAME_ONLY);
+ }
+
+ @Test
+ void commitTimeAndFileNameModePopulatesBoth() {
+ Map<String, String> options = baseOptions();
+ options.put(HoodieTableConfig.POPULATE_META_FIELDS.key(), "false");
+ options.put(HoodieTableConfig.META_FIELDS_MODE.key(),
MetaFieldsMode.COMMIT_TIME_AND_FILE_NAME.name());
+ options.put(DataSourceWriteOptions.OPERATION().key(),
DataSourceWriteOptions.BULK_INSERT_OPERATION_OPT_VAL());
+
+ HoodieTableConfig tc = writeSampleAndGetTableConfig(options, basePath());
+
+ assertEquals(MetaFieldsMode.COMMIT_TIME_AND_FILE_NAME,
tc.getMetaFieldsMode());
+ assertMetaColumnPopulation(basePath(),
MetaFieldsMode.COMMIT_TIME_AND_FILE_NAME);
+ }
+
+ @Test
+ void selectiveModeWinsOverLegacyPopulateTrue() {
+ // hoodie.meta.fields.mode is the source of truth: an explicit mode is
honored regardless of
+ // the deprecated boolean, so this combination is no longer ambiguous and
is not rejected.
+ Map<String, String> options = baseOptions();
+ options.put(HoodieTableConfig.POPULATE_META_FIELDS.key(), "true");
+ options.put(HoodieTableConfig.META_FIELDS_MODE.key(),
MetaFieldsMode.COMMIT_TIME_ONLY.name());
+ options.put(DataSourceWriteOptions.OPERATION().key(),
DataSourceWriteOptions.BULK_INSERT_OPERATION_OPT_VAL());
+
+ HoodieTableConfig tc = writeSampleAndGetTableConfig(options, basePath());
+
+ assertEquals(MetaFieldsMode.COMMIT_TIME_ONLY, tc.getMetaFieldsMode());
+ assertMetaColumnPopulation(basePath(), MetaFieldsMode.COMMIT_TIME_ONLY);
+ // ...and hoodie.properties must not contradict the mode. A pre-1.3.0
reader ignores the mode
+ // property entirely, so leaving populate.meta.fields=true here would make
it treat a
+ // selectively-written table as ALL.
+ assertFalse(tc.populateMetaFields(),
+ "legacy populate.meta.fields must be derived from the mode, not
carried through verbatim");
+ }
+
+ @Test
+ void noneModePersistsLegacyBooleanAsFalse() {
+ // The unsafe case: an old incremental reader that sees
populate.meta.fields=true on a NONE
+ // table would run against all-null commit times and silently return zero
rows.
+ Map<String, String> options = baseOptions();
+ options.put(HoodieTableConfig.POPULATE_META_FIELDS.key(), "true");
+ options.put(HoodieTableConfig.META_FIELDS_MODE.key(),
MetaFieldsMode.NONE.name());
+ options.put(DataSourceWriteOptions.OPERATION().key(),
DataSourceWriteOptions.BULK_INSERT_OPERATION_OPT_VAL());
+
+ HoodieTableConfig tc = writeSampleAndGetTableConfig(options, basePath());
+
+ assertEquals(MetaFieldsMode.NONE, tc.getMetaFieldsMode());
+ assertFalse(tc.populateMetaFields(),
+ "NONE must persist populate.meta.fields=false so pre-1.3.0 readers do
not treat it as ALL");
+ }
+
+ @Test
+ void allModePersistsLegacyBooleanAsTrue() {
+ Map<String, String> options = baseOptions();
+ options.put(HoodieTableConfig.POPULATE_META_FIELDS.key(), "false");
+ options.put(HoodieTableConfig.META_FIELDS_MODE.key(),
MetaFieldsMode.ALL.name());
+ options.put(DataSourceWriteOptions.OPERATION().key(),
DataSourceWriteOptions.BULK_INSERT_OPERATION_OPT_VAL());
+
+ HoodieTableConfig tc = writeSampleAndGetTableConfig(options, basePath());
+
+ assertEquals(MetaFieldsMode.ALL, tc.getMetaFieldsMode());
+ assertTrue(tc.populateMetaFields(),
+ "ALL must persist populate.meta.fields=true for pre-1.3.0 readers");
+ }
+
+ @Test
+ void unknownModeValueIsRejected() {
+ Map<String, String> options = baseOptions();
+ options.put(HoodieTableConfig.POPULATE_META_FIELDS.key(), "false");
+ options.put(HoodieTableConfig.META_FIELDS_MODE.key(), "SOMETHING_BOGUS");
+ options.put(DataSourceWriteOptions.OPERATION().key(),
DataSourceWriteOptions.BULK_INSERT_OPERATION_OPT_VAL());
+
+ Throwable thrown = assertThrows(Throwable.class, () ->
+ writeRows(Collections.singletonList(RowFactory.create("k1", "p1",
"v1")),
+ simpleSchema(), options, basePath(), SaveMode.Overwrite));
+
+ String rootMessage = rootMessageOf(thrown);
+ assertTrue(rootMessage.contains("SOMETHING_BOGUS"),
+ "Expected error to name the rejected value, got: " + rootMessage);
+ }
+
+ // -------------------------------------------------------------------------
+ // Non-row-writer path coverage. Bulk insert with row.writer.enable=false
forces the
+ // HoodieAvroParquetWriter path (via HoodieCreateHandle) instead of the
internal-row writer path.
+ // Both paths must respect the mode identically.
+ // -------------------------------------------------------------------------
+
+ @Test
+ void nonRowWriterPathAllMode() {
+ Map<String, String> options = baseOptions();
+ options.put(DataSourceWriteOptions.OPERATION().key(),
DataSourceWriteOptions.INSERT_OPERATION_OPT_VAL());
+ options.put("hoodie.datasource.write.row.writer.enable", "false");
+
+ HoodieTableConfig tc = writeSampleAndGetTableConfig(options, basePath());
+ assertEquals(MetaFieldsMode.ALL, tc.getMetaFieldsMode());
+ assertMetaColumnPopulation(basePath(), MetaFieldsMode.ALL);
+ }
+
+ @Test
+ void nonRowWriterPathNoneMode() {
+ Map<String, String> options = baseOptions();
+ options.put(HoodieTableConfig.POPULATE_META_FIELDS.key(), "false");
+ options.put(DataSourceWriteOptions.OPERATION().key(),
DataSourceWriteOptions.INSERT_OPERATION_OPT_VAL());
+ options.put("hoodie.datasource.write.row.writer.enable", "false");
+
+ HoodieTableConfig tc = writeSampleAndGetTableConfig(options, basePath());
+ assertEquals(MetaFieldsMode.NONE, tc.getMetaFieldsMode());
+ assertMetaColumnPopulation(basePath(), MetaFieldsMode.NONE);
+ }
+
+ @Test
+ void nonRowWriterPathCommitTimeOnly() {
+ Map<String, String> options = baseOptions();
+ options.put(HoodieTableConfig.POPULATE_META_FIELDS.key(), "false");
+ options.put(HoodieTableConfig.META_FIELDS_MODE.key(),
MetaFieldsMode.COMMIT_TIME_ONLY.name());
+ options.put(DataSourceWriteOptions.OPERATION().key(),
DataSourceWriteOptions.INSERT_OPERATION_OPT_VAL());
+ options.put("hoodie.datasource.write.row.writer.enable", "false");
+
+ HoodieTableConfig tc = writeSampleAndGetTableConfig(options, basePath());
+ assertEquals(MetaFieldsMode.COMMIT_TIME_ONLY, tc.getMetaFieldsMode());
+ assertMetaColumnPopulation(basePath(), MetaFieldsMode.COMMIT_TIME_ONLY);
+ }
+
+ @Test
+ void nonRowWriterPathFileNameOnly() {
+ Map<String, String> options = baseOptions();
+ options.put(HoodieTableConfig.POPULATE_META_FIELDS.key(), "false");
+ options.put(HoodieTableConfig.META_FIELDS_MODE.key(),
MetaFieldsMode.FILE_NAME_ONLY.name());
+ options.put(DataSourceWriteOptions.OPERATION().key(),
DataSourceWriteOptions.INSERT_OPERATION_OPT_VAL());
+ options.put("hoodie.datasource.write.row.writer.enable", "false");
+
+ HoodieTableConfig tc = writeSampleAndGetTableConfig(options, basePath());
+ assertEquals(MetaFieldsMode.FILE_NAME_ONLY, tc.getMetaFieldsMode());
+ assertMetaColumnPopulation(basePath(), MetaFieldsMode.FILE_NAME_ONLY);
+ }
+
+ @Test
+ void nonRowWriterPathCommitTimeAndFileName() {
+ Map<String, String> options = baseOptions();
+ options.put(HoodieTableConfig.POPULATE_META_FIELDS.key(), "false");
+ options.put(HoodieTableConfig.META_FIELDS_MODE.key(),
MetaFieldsMode.COMMIT_TIME_AND_FILE_NAME.name());
+ options.put(DataSourceWriteOptions.OPERATION().key(),
DataSourceWriteOptions.INSERT_OPERATION_OPT_VAL());
+ options.put("hoodie.datasource.write.row.writer.enable", "false");
+
+ HoodieTableConfig tc = writeSampleAndGetTableConfig(options, basePath());
+ assertEquals(MetaFieldsMode.COMMIT_TIME_AND_FILE_NAME,
tc.getMetaFieldsMode());
+ assertMetaColumnPopulation(basePath(),
MetaFieldsMode.COMMIT_TIME_AND_FILE_NAME);
+ }
+
+ // -------------------------------------------------------------------------
+ // Clustering coverage. Inline clustering rewrites files through the
create/merge handles which
+ // delegate to the same underlying HoodieAvroParquetWriter /
HoodieRowCreateHandle we exercise
+ // in the write tests. Verifies clustered files preserve the mode's column
population semantics.
+ // -------------------------------------------------------------------------
+
+ @Test
+ void clusteringPreservesCommitTimeOnlyMode() {
Review Comment:
These clustering tests cannot fail on the bug they were written for.
`358fbfdd717a` fixed row-writer clustering copying the *source row's*
`_hoodie_file_name` instead of the new one. The only assertion these reach is
`assertNotNull(first.get(4))` in `assertMetaColumnPopulation` -- and the buggy
value (the pre-clustering file name) is also non-null, so it passes either way.
`git revert -n 358fbfdd717a` and these stay green.
Two more issues in the same helper: it globs `path + "/*/*.parquet"`, which
after inline clustering still includes the *replaced* pre-clustering file (no
cleaning has run), and `.first()` samples one arbitrary row. Nothing asserts
clustering actually ran.
Suggest asserting `_hoodie_file_name` equals the containing file, for every
row:
```java
Dataset<Row> raw = spark().read().parquet(path + "/*/*.parquet")
.withColumn("__f", functions.input_file_name());
assertEquals(0, raw.filter("__f not like concat('%/',
_hoodie_file_name)").count());
```
plus `assertEquals(1,
metaClient.getActiveTimeline().getCompletedReplaceTimeline().countInstants())`.
Also `clusteringPreservesAllMode` and `clusteringPreservesNoneMode` never enter
the selective branch at all (`HoodieRowCreateHandle.java:162-170` routes
ALL/NONE elsewhere) -- those two can go.
##########
hudi-spark-datasource/hudi-spark/src/test/scala/org/apache/hudi/TestHoodieSparkSqlWriter.scala:
##########
@@ -107,7 +107,7 @@ class TestHoodieSparkSqlWriter extends
HoodieSparkWriterTestBase {
// fetch all records from parquet files generated from write to hudi
val actualDf = sqlContext.read.parquet(fullPartitionPaths(0),
fullPartitionPaths(1), fullPartitionPaths(2))
if (!populateMetaFields) {
- List(0, 1, 2, 3, 4).foreach(i => assertEquals(0,
actualDf.select(HoodieRecord.HOODIE_META_COLUMNS.get(i)).filter(entry =>
!(entry.mkString(",").equals(""))).count()))
+ List(0, 1, 2, 3, 4).foreach(i => assertEquals(0,
actualDf.select(HoodieRecord.HOODIE_META_COLUMNS.get(i)).filter(entry =>
!entry.isNullAt(0) && entry.getString(0).nonEmpty).count()))
Review Comment:
This update is correct -- the old form would fail on NULL, since
`Row.mkString` renders null as the string "null" -- but it relaxed further than
the behaviour change required. `!entry.isNullAt(0) &&
entry.getString(0).nonEmpty` accepts **both** NULL and `""`, so it no longer
pins which representation is written.
That is worth keeping strict, because the whole point of `77f4e9148c4a` is
that these are now NULL, and `TestMetaFieldsMode.java:149` asserts exactly
`assertNull` for the same scenario in this same PR. Right now the two tests
disagree on strictness.
```suggestion
List(0, 1, 2, 3, 4).foreach(i => assertEquals(0,
actualDf.select(HoodieRecord.HOODIE_META_COLUMNS.get(i)).filter(entry =>
!entry.isNullAt(0)).count()))
```
Since this exact edit is being made in two near-identical files, a shared
`assertNoMetaFieldsPopulated(df)` in `HoodieSparkWriterTestBase` (next to
`dropMetaFields`) would be better than duplicating it.
##########
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);
Review Comment:
Passing `null` for `upgradeDowngradeHelper` makes the handler short-circuit
to `MetaFieldsMode.ALL`, so the entire selective-mode branch -- the mode lookup
and the data-loss `LOG.warn` -- is never executed by any test.
Suggest one case with a mock helper returning `COMMIT_TIME_ONLY`, copying
the `helperFor(...)` factory from `TestNineToTenUpgradeHandler.java:33-45`. You
will need it anyway to cover the downgrade fix.
##########
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:
**This is cshuo's StreamSync-restart scenario, and I do not think it is
fixed.** It was closed as resolved by `c78dc962`, but that commit fixed
table-config *resolution*, not this gate.
Walk it through with table = `COMMIT_TIME_ONLY` and a writer that sets only
`hoodie.populate.meta.fields=false`:
- `writeMetaFieldsMode` resolves to `NONE`
- `NONE.isWiderThan(COMMIT_TIME_ONLY)` is `false`, so the first branch is
skipped
- `writerStatedMode` is `false`, so the second branch is skipped
- validation passes
The writer then takes the mode from the **write** config, not the table
config (`HoodieSparkFileWriterFactory.java:59-60`), so it writes base files
with null `_hoodie_commit_time`. The table still advertises `COMMIT_TIME_ONLY`,
so `IncrementalRelationV1.scala:92` still admits incremental queries, and the
range filter at `:295-297` silently drops every one of those rows.
StreamSync is the concrete path: it builds its write config from `props`
only (`StreamSync.java:1280-1291`) and never merges the table config -- the
`metaClient != null` block at `:1297-1309` only touches ordering fields. The +2
lines this PR adds live in `initializeEmptyTable`, which runs only when the
base path does not exist, so a restart against an existing table never re-reads
the mode. The Spark datasource is safe only by accident, because
`HoodieSparkSqlWriter.scala:1102-1106` copies table props into the write params.
Same failure shape as `876a891979a3` [HUDI-3544], where the fix was to
detect the config/file disagreement and re-initialize rather than let mixed
files accumulate.
The narrowing carve-out only needs to cover `ALL -> NONE` (the pre-PR
behaviour from `d5026e9a2485`). Suggest tightening to:
```java
} else if (writeMetaFieldsMode != tableMetaFieldsMode
&& (writerStatedMode || tableMetaFieldsMode.isSelective())) {
```
and adding a `TestBaseHoodieWriteClient` case: table `COMMIT_TIME_ONLY`,
unstated writer -> must throw.
##########
hudi-client/hudi-client-common/src/test/java/org/apache/hudi/table/upgrade/TestNineToTenUpgradeHandler.java:
##########
@@ -0,0 +1,85 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hudi.table.upgrade;
+
+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.common.table.HoodieTableMetaClient;
+import org.apache.hudi.config.HoodieWriteConfig;
+import org.apache.hudi.table.HoodieTable;
+
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.CsvSource;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.mockito.Mockito.RETURNS_DEEP_STUBS;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+/**
+ * Version 9 tables predate {@code hoodie.meta.fields.mode}, so the upgrade
records the value
+ * derived from the deprecated {@code hoodie.populate.meta.fields} boolean.
This makes an upgraded
+ * table describe its meta-field layout the same way a freshly created version
10 table does,
+ * instead of relying on the legacy fallback at every read.
+ */
+class TestNineToTenUpgradeHandler {
+
+ private static SupportsUpgradeDowngrade helperFor(MetaFieldsMode
resolvedMode) {
+ HoodieTable table = mock(HoodieTable.class, RETURNS_DEEP_STUBS);
+ HoodieTableMetaClient metaClient = mock(HoodieTableMetaClient.class,
RETURNS_DEEP_STUBS);
+ HoodieTableConfig tableConfig = mock(HoodieTableConfig.class);
+ when(tableConfig.getMetaFieldsMode()).thenReturn(resolvedMode);
+ when(metaClient.getTableConfig()).thenReturn(tableConfig);
+ when(table.getMetaClient()).thenReturn(metaClient);
+
+ SupportsUpgradeDowngrade helper = mock(SupportsUpgradeDowngrade.class);
+
when(helper.getTable(org.mockito.ArgumentMatchers.any(HoodieWriteConfig.class),
+
org.mockito.ArgumentMatchers.any(HoodieEngineContext.class))).thenReturn(table);
+ return helper;
+ }
+
+ @ParameterizedTest
+ @CsvSource({"ALL", "NONE"})
+ void upgradeRecordsTheModeDerivedFromTheLegacyBoolean(String modeName) {
+ MetaFieldsMode expected = MetaFieldsMode.valueOf(modeName);
+ UpgradeDowngrade.TableConfigChangeSet changeSet = new
NineToTenUpgradeHandler().upgrade(
+ mock(HoodieWriteConfig.class), mock(HoodieEngineContext.class), "001",
helperFor(expected));
+
+ assertTrue(changeSet.propertiesToDelete().isEmpty());
+ assertEquals(1, changeSet.propertiesToUpdate().size());
+ assertEquals(expected.name(),
+
changeSet.propertiesToUpdate().get(HoodieTableConfig.META_FIELDS_MODE));
+ }
+
+ @Test
+ void upgradeLeavesTheLegacyBooleanAlone() {
Review Comment:
nit: every assertion here is implied by the parameterized test above with
`modeName=NONE` -- both call `helperFor(NONE)` and assert the same
delete/update sizes, and `containsKey(META_FIELDS_MODE)` is weaker than the
`equals("NONE")` already asserted at `:69`. It cannot fail independently.
Either delete it, or make it discriminate on what its name claims by
asserting `POPULATE_META_FIELDS` appears in neither the update nor the delete
set.
--
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]
