nsivabalan commented on code in PR #19205:
URL: https://github.com/apache/hudi/pull/19205#discussion_r3735255383
##########
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:
Agreed, and fixed — you were right that the relaxation went further than the
behavior change required, and that it left the two tests disagreeing on
strictness.
Extracted `assertNoMetaFieldsPopulated(df)` into `HoodieSparkWriterTestBase`
next to `dropMetaFields`, as you suggested, rather than duplicating the edit:
```scala
def assertNoMetaFieldsPopulated(df: Dataset[Row]): Unit = {
(0 until HoodieRecord.HOODIE_META_COLUMNS.size()).foreach { i =>
val column = HoodieRecord.HOODIE_META_COLUMNS.get(i)
assertEquals(0, df.select(column).filter(entry =>
!entry.isNullAt(0)).count(), ...)
}
}
```
Strictly `NULL`, so it now agrees with the `assertNull` assertions in the
functional test for the same scenario. Applied in both this file and
`TestHoodieSparkSqlWriterWithTestFormat`.
##########
hudi-hadoop-common/src/main/java/org/apache/hudi/io/storage/hadoop/HoodieAvroParquetWriter.java:
##########
@@ -48,39 +51,72 @@ public class HoodieAvroParquetWriter
private final String fileName;
private final String instantTime;
private final TaskContextSupplier taskContextSupplier;
- private final boolean populateMetaFields;
+ private final MetaFieldsMode metaFieldsMode;
private final HoodieAvroWriteSupport writeSupport;
+ /**
+ * @deprecated since 1.3.0, use the {@link MetaFieldsMode} overload.
Retained for existing callers
+ * that only distinguish all-or-nothing meta fields ({@code true} maps to
{@link MetaFieldsMode#ALL},
+ * {@code false} to {@link MetaFieldsMode#NONE}); it cannot express the
selective modes.
+ */
+ @Deprecated
@SuppressWarnings({"unchecked", "rawtypes"})
public HoodieAvroParquetWriter(StoragePath file,
HoodieParquetConfig<HoodieAvroWriteSupport>
parquetConfig,
String instantTime,
TaskContextSupplier taskContextSupplier,
boolean populateMetaFields) throws
IOException {
+ this(file, parquetConfig, instantTime, taskContextSupplier,
+ populateMetaFields ? MetaFieldsMode.ALL : MetaFieldsMode.NONE);
+ }
+
+ @SuppressWarnings({"unchecked", "rawtypes"})
+ public HoodieAvroParquetWriter(StoragePath file,
+ HoodieParquetConfig<HoodieAvroWriteSupport>
parquetConfig,
+ String instantTime,
+ TaskContextSupplier taskContextSupplier,
+ MetaFieldsMode metaFieldsMode) throws
IOException {
super(file, (HoodieParquetConfig) parquetConfig);
this.fileName = file.getName();
this.writeSupport = parquetConfig.getWriteSupport();
this.instantTime = instantTime;
this.taskContextSupplier = taskContextSupplier;
- this.populateMetaFields = populateMetaFields;
+ this.metaFieldsMode = metaFieldsMode == null ? MetaFieldsMode.NONE :
metaFieldsMode;
}
@Override
public void writeAvroWithMetadata(HoodieKey key, IndexedRecord avroRecord)
throws IOException {
- if (populateMetaFields) {
- prepRecordWithMetadata(key, avroRecord, instantTime,
- taskContextSupplier.getPartitionIdSupplier().get(),
getWrittenRecordCount(), fileName);
- super.write(avroRecord);
- writeSupport.add(key.getRecordKey());
- } else {
- super.write(avroRecord);
+ switch (metaFieldsMode) {
+ case ALL:
+ prepRecordWithMetadata(key, avroRecord, instantTime,
+ taskContextSupplier.getPartitionIdSupplier().get(),
getWrittenRecordCount(), fileName);
+ super.write(avroRecord);
+ writeSupport.add(key.getRecordKey());
+ break;
+ case NONE:
+ super.write(avroRecord);
+ break;
+ default:
+ // Selective mode — populate only the opted-in columns. The other meta
columns stay null,
+ // which Parquet stores as definition-level flags (zero data bytes).
Bloom filter /
+ // record-key index population is intentionally skipped — that
requires the record-key
+ // column, which is never populated in selective modes.
+ GenericRecord genericRecord = (GenericRecord) avroRecord;
+ if (metaFieldsMode.isCommitTimePopulated()) {
+ genericRecord.put(HoodieRecord.COMMIT_TIME_METADATA_FIELD,
instantTime);
+ }
+ if (metaFieldsMode.isFileNamePopulated()) {
Review Comment:
Good catch, and it was a real bug — thanks. `_hoodie_file_name` was indeed
being stamped on records copied forward, so a `COMMIT_TIME_ONLY` table
accumulated file names on every upsert.
Fixed in two places. `HoodieWriteMergeHandle.writeToFile` was the site you
identified. A new upsert test then caught a **second** one immediately:
`BaseCreateHandle.writeRecordToFile` has the identical `preserveMetadata` fork
via `updateFileName`, which nothing had flagged.
One subtlety worth recording: the two sites need different fixes.
`MetadataValues` skips null entries (`updateMetadataValuesInternal` only puts
non-null values), so passing null through `setFileName` would leave whatever
the record already carried — and a record written while the table was on `ALL`
would keep its old file name. `BaseCreateHandle` therefore prepends empty
metadata and then overwrites the ordinal with an explicit null. Both handles
resolve the mode once into a field rather than per record.
On the contract question: the mode is authoritative, so a column it does not
opt into is null, including on copied records. The complement is tested too —
under `COMMIT_TIME_AND_FILE_NAME` the file name is still rewritten to the
containing file, which is why the unconditional rewrite existed in the first
place.
##########
hudi-utilities/src/main/java/org/apache/hudi/utilities/streamer/StreamSync.java:
##########
@@ -1294,6 +1296,21 @@ private Pair<HoodieWriteConfig, HoodieSchema>
getHoodieClientConfigAndWriterSche
if (metaClient != null) {
HoodieTableConfig tableConfig = metaClient.getTableConfig();
+ // Inherit the table's meta-fields mode when this run does not state
one, mirroring how the Spark
+ // datasource folds table props into the write params
(HoodieSparkSqlWriter#mergeParamsAndGetHoodieConfig).
+ //
+ // Meta-field population is physical, so it belongs to the table, not to
the run. StreamSync builds
+ // its write config from `props` alone, and the mode is persisted only
in `initializeEmptyTable` --
+ // which runs solely when the base path does not exist. So a restart
against an existing table that
+ // passes only the legacy boolean (or nothing at all) resolved to NONE
and wrote base files with a
+ // null _hoodie_commit_time, while hoodie.properties still advertised
COMMIT_TIME_ONLY. Incremental
+ // queries were then admitted and silently dropped every one of those
rows.
+ //
+ // Only fills the gap: an explicitly stated mode is left alone so a
genuine conflict is still caught
+ // downstream by BaseHoodieWriteClient#validateAgainstTableProperties.
+ if (!props.containsKey(HoodieTableConfig.META_FIELDS_MODE.key())) {
Review Comment:
Valid concern, and the answer is that the code you flagged is now
**removed** rather than patched.
You were right that the inheritance keyed on `META_FIELDS_MODE` alone, so an
explicitly-passed `hoodie.populate.meta.fields` was silently overridden instead
of being treated as a stated intent. That inheritance has moved to
`BaseHoodieWriteClient#validateAgainstTableProperties`, where it applies to
every engine and keys on **both** properties:
- writer states neither → inherits the table's mode
- writer states either → compared, rejected on mismatch
So `populate.meta.fields=false` against an `ALL` table now throws rather
than being overridden. That does retire the HUDI-2161 narrowing you referenced
(`d5026e9a2485`) — deliberately: the mode is a table property, changeable only
via hudi-cli or upgrade, so a write cannot narrow it. It is called out as a
breaking change in the PR description, since a job carrying that flag against a
default table will now fail until it drops it.
##########
hudi-hadoop-common/src/main/java/org/apache/hudi/io/storage/hadoop/HoodieAvroFileWriterFactory.java:
##########
@@ -89,7 +89,7 @@ protected HoodieFileWriter newParquetFileWriter(
hoodieConfig.getLongOrDefault(HoodieStorageConfig.PARQUET_MAX_FILE_SIZE),
storageConfiguration,
hoodieConfig.getDoubleOrDefault(HoodieStorageConfig.PARQUET_COMPRESSION_RATIO_FRACTION),
hoodieConfig.getBooleanOrDefault(HoodieStorageConfig.PARQUET_DICTIONARY_ENABLED));
- return new HoodieAvroParquetWriter(path, parquetConfig, instantTime,
taskContextSupplier, populateMetaFields);
+ return new HoodieAvroParquetWriter(path, parquetConfig, instantTime,
taskContextSupplier, populateMetaFields, metaFieldsMode);
Review Comment:
Resolved by `cfd1fea706ed` — `populateMetaFields` and `metaFieldsMode` are
folded into a single `MetaFieldsMode` argument through every writer factory and
constructor, with `isCommitTimePopulated()` / `isFileNamePopulated()` used in
the write branches.
Since then the enum has become the sole authority rather than a parallel
signal: `populateMetaFields()` on both configs is *derived* from it, and the
two write handles gated in the latest push (`HoodieWriteMergeHandle`,
`BaseCreateHandle`) resolve the enum too. So there is no longer any path that
takes both.
##########
hudi-spark-datasource/hudi-spark-common/src/main/scala/org/apache/hudi/IncrementalRelationV1.scala:
##########
@@ -89,8 +88,9 @@ class IncrementalRelationV1(val sqlContext: SQLContext,
s"option ${DataSourceReadOptions.START_COMMIT.key}")
}
- if (!metaClient.getTableConfig.populateMetaFields()) {
- throw new HoodieException("Incremental queries are not supported when meta
fields are disabled")
+ if (!metaClient.getTableConfig.isCommitTimePopulated()) {
+ throw new HoodieException("Incremental queries are not supported when
_hoodie_commit_time is not populated. "
+ + "Either keep hoodie.populate.meta.fields=true or include
_hoodie_commit_time in hoodie.meta.fields.mode.")
Review Comment:
Done in `cfd1fea706ed`, and the guard is now actually reachable — which it
was not when I wrote that reply.
The message states that `hoodie.meta.fields.mode` is a physical-storage
decision baked into files at write time, cannot be flipped by write options,
and that the table must be recreated (or changed through hudi-cli, #19206).
The correction: that guard lived only in `IncrementalRelationV1/V2`, which
back the *streaming* source. A Spark datasource incremental read goes through
`HoodieCopyOnWriteIncrementalHadoopFsRelationFactory`, so a `NONE` or
`FILE_NAME_ONLY` table returned **zero rows silently** instead of hitting this
message. The check is now on that factory as well, and there are tests
asserting the throw for both modes.
##########
hudi-common/src/main/java/org/apache/hudi/common/model/MetaFieldsMode.java:
##########
@@ -0,0 +1,152 @@
+/*
+ * 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.util.StringUtils;
+
+/**
+ * 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.
+ */
+ public static MetaFieldsMode resolve(String rawMode, boolean
legacyPopulateMetaFields) {
Review Comment:
Done in `9495687a7813` — `MetaFieldsMode.resolve(HoodieConfig)`. Both
`HoodieTableConfig` and `HoodieWriteConfig` extend `HoodieConfig`, so one
overload covers the write config and both writer factories, and the property
keys plus precedence rule live in a single place.
It has since picked up more callers than it had when you asked: the two
write handles gated in this push (`HoodieWriteMergeHandle`, `BaseCreateHandle`)
both use it, and `HoodieWriterUtils.scala` uses it rather than casting — which
matters because `HoodieCatalogTable` passes a plain `HoodieConfig` built from a
map, so a cast would throw on the Spark SQL path.
One deliberate exception remains: `HoodieTableConfig#getMetaFieldsMode`
keeps the two-argument form, because its fallback must read the *raw*
`populate.meta.fields` property while `populateMetaFields()` on that class is
itself derived from the mode. Routing it through the overload would be circular
in intent even though it happens to work; commented in place.
##########
hudi-common/src/main/java/org/apache/hudi/common/table/TableSchemaResolver.java:
##########
@@ -124,7 +125,11 @@ private Option<HoodieSchema>
getTableSchemaFromDataFileInternal() {
* @throws Exception
*/
public HoodieSchema getTableSchema() throws Exception {
- return getTableSchema(metaClient.getTableConfig().populateMetaFields());
+ // Include meta fields whenever the table's meta-fields mode populates any
of them. Under
+ // selective modes (COMMIT_TIME_ONLY / FILE_NAME_ONLY /
COMMIT_TIME_AND_FILE_NAME) the meta
+ // columns exist as physical nullable Parquet columns even though
populateMetaFields() is false,
+ // and read paths (e.g. incremental relations) must see them in the
projected schema.
+ return getTableSchema(metaClient.getTableConfig().getMetaFieldsMode() !=
MetaFieldsMode.NONE);
Review Comment:
Reconfirming this now that there are end-to-end tests, since the answer is
load-bearing for the read path.
Not compatibility — it reflects what is physically on disk. Under a
selective mode all five meta columns are still written to the Parquet file; the
ones the mode does not populate hold `null`. `HoodieDatasetBulkInsertHelper`
prepends nullable stubs for every meta field and the writers fill only the
opted-in ones, so Parquet stores the rest as definition-level flags rather than
data. That is what makes a selective mode cheaper than `ALL` without needing a
different column layout per mode.
Given the columns exist, the schema has to describe them: a reader
projecting `_hoodie_commit_time` on a `COMMIT_TIME_ONLY` table would otherwise
fail against a file that really has the column, and incremental queries depend
on exactly that projection. The predicate is `mode != NONE` rather than
`populateMetaFields()` because the latter is false for every selective mode,
which would give the wrong answer here.
This is now covered rather than asserted: the incremental tests added in
this push read `_hoodie_commit_time` back through the datasource on a
`COMMIT_TIME_ONLY` table. `NONE` remains the genuinely different case — no meta
columns written, none in the schema, same as today's
`populate.meta.fields=false`.
##########
hudi-common/src/main/java/org/apache/hudi/common/table/HoodieTableConfig.java:
##########
@@ -327,12 +328,31 @@ public static final String getDefaultPayloadClassName() {
.noDefaultValue()
.withDocumentation("Base path of the dataset that needs to be
bootstrapped as a Hudi table");
+ /**
+ * @deprecated since 1.3.0, use {@link #META_FIELDS_MODE} instead. {@code
true} maps to
+ * {@link MetaFieldsMode#ALL} and {@code false} maps to {@link
MetaFieldsMode#NONE}. This property
+ * is still honored for tables written before {@code
hoodie.meta.fields.mode} existed, but it is
+ * consulted only when the mode property is absent.
+ */
+ @Deprecated
Review Comment:
Handled, and it got stricter than my earlier reply described — worth
re-reading before resolving.
**Upgrade (9 → 10)**: `NineToTenUpgradeHandler` records the mode derived
from the legacy boolean (`true -> ALL`, `false -> NONE`), making the on-disk
state explicit. It deliberately does not delete `POPULATE_META_FIELDS` — unlike
`EightToNineUpgradeHandler`'s removals, that property has a `true` default, so
deleting it is exactly what would create a silent widening.
**Downgrade (10 → 9)**: writes the legacy boolean back from the mode and
deletes the mode, mirroring `NineToEightDowngradeHandler:116-117`. `ALL` and
`NONE` round-trip losslessly.
**Selective modes now fail the downgrade** rather than degrading to `NONE`
with a warning, which is what my earlier reply described. @voonhous pointed out
the degrade path was one-way and unrecoverable — re-upgrading derives `NONE`
from the boolean and widening back is rejected — and no other handler pair in
the package behaves that way. Throwing makes that state unreachable instead of
documented: the operator is told to rewrite the table to `ALL` or `NONE` first.
`TestTenToNineDowngradeHandler` covers all five modes plus the no-helper
case; removing the throw fails exactly 3 tests, and reverting the boolean
write-back fails 7 of 10.
--
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]
