nsivabalan commented on code in PR #19205: URL: https://github.com/apache/hudi/pull/19205#discussion_r3735258035
########## 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: This one is already covered, and I think it predates the file that covers it — flagging rather than resolving so you can confirm. `hudi-common/src/test/java/org/apache/hudi/common/model/TestMetaFieldsMode.java` has `isWiderThanCoversEveryOrderedPair`, a `@CsvSource` over the full 5×5 matrix, plus `commitTimeOnlyAndFileNameOnlyAreMutuallyWider`, `noModeIsWiderThanItself`, and the null case. That includes both pairs you asked for — `COMMIT_TIME_AND_FILE_NAME` vs `ALL` in both directions, and the mutually-wider siblings. `TestBaseHoodieWriteClient` also now names `FILE_NAME_ONLY` and `COMMIT_TIME_AND_FILE_NAME` explicitly, in `validateAgainstTablePropertiesRejectsSiblingSelectiveModesBothWays` and `...RejectsStatedSelectiveToSelectiveNarrowing`. One thing that did change since your comment: `isWiderThan`'s role narrowed. It is no longer the sole gate — the write client rejects *any* stated mismatch, in both directions, because the mode is a table property. `isWiderThan` now only distinguishes the two error messages there, and marks the direction that hudi-cli / upgrade may take (documented in its javadoc). So the "narrowing is allowed" subtlety you were pinning is gone at the writer level; it survives only for the sanctioned mutation paths. ########## 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: Mostly taken, with one deviation. Taken: the class name was wrong about what it tested. Renamed to `TestHoodieTableConfigMetaFieldsMode`, and its javadoc now names the two sibling classes and what each covers, so the split is intentional rather than accidental. Also fixed a collision you may not have seen — there were two classes named `TestMetaFieldsMode`; the functional one is now `TestMetaFieldsModeE2E`. Also taken: `TestHoodieTableConfig` gains `testMetaFieldsModeSurvivesAPropertiesRoundTrip`, which is the assertion the in-memory cases cannot make — that the mode survives a real `hoodie.properties` write/read and still resolves the same way, written the way `TableBuilder` writes it (derived boolean, never the caller's). Deviation: I did not fold the nine methods in. Their `configOf(Boolean, String)` helper is a lighter in-memory `HoodieTableConfig` than `TestHoodieTableConfig`'s storage-backed fixture, and they run in 0.6s against that class's 30s. Keeping the pure-resolution cases separate from the storage round-trip seemed better than making nine fast tests pay for a fixture they do not use. Happy to merge them if you disagree — it is mechanical. And your suggestion of a genuine enum unit test under `common/model/` already exists as `TestMetaFieldsMode` (the `parse` / `isWiderThan` / `toLegacyPopulateMetaFields` cases), so all three layers now have a distinct home. ########## 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: Added the restart test, and trimmed as you suggested — but I kept the class rather than deleting it, so flagging the deviation. `testRestartWithoutRestatingTheModeKeepsWritingCommitTimes`: create with `COMMIT_TIME_ONLY`, `ingestOnce()`, then a **second** streamer stating neither meta-field property, `ingestOnce()` again, asserting the mode survived and that no row across either commit has a null `_hoodie_commit_time`. That is cshuo's scenario, and you were right that nothing tested it — the file had a single `ingestOnce()`. Trimmed per your other two points: the parameterized cases are now only the three selective modes (`ALL`/`NONE` added no mode key, so they exercised none of the new plumbing), and the third copy of the MoR `checkArgument` is gone. **Why I kept the class:** it also holds the on-disk per-mode assertions, and `HoodieDeltaStreamerTestBase` is already its fixture, so folding those into `TestHoodieDeltaStreamer` would either duplicate the helper or lose the coverage. Happy to move it if you would rather — but note the streamer-specific production code you pointed at (`StreamSync.java:481-482`) is now *gone*: the inheritance it did is redundant with the write-client rule, and worse, it keyed only on the mode property, so an explicitly-passed legacy boolean would have silently inherited instead of conflicting. The remaining streamer-side value of this class is the end-to-end restart assertion. ########## hudi-client/hudi-client-common/src/main/java/org/apache/hudi/config/HoodieWriteConfig.java: ########## @@ -3611,15 +3619,20 @@ 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 Review Comment: Revisiting this, since the design changed materially in the latest push and your original objection is now largely addressed. You said a table config option should not be settable through the write config API. That is now true in the way that matters: `hoodie.meta.fields.mode` is a **table property**, and a write cannot change it. `BaseHoodieWriteClient#validateAgainstTableProperties` enforces one rule for every engine — a writer that states neither meta-field property inherits the table's mode; a writer that states either is compared and rejected on mismatch. The only sanctioned mutation paths are table creation, hudi-cli (#19206), and upgrade. So `withMetaFieldsMode` on the write-config builder is now purely an *input to table creation* plus a way to assert an expectation against an existing table — never a mutation channel. The builder can express an intent, and a disagreement is rejected rather than applied. The broader cleanup you were pointing at — write-config builders should not carry table-config setters at all — still stands as a separate concern, and it covers the pre-existing `POPULATE_META_FIELDS` setter too. Happy to file a JIRA for that sweep if you think it is worth tracking. ########## hudi-common/src/main/java/org/apache/hudi/common/table/HoodieTableConfig.java: ########## @@ -1230,11 +1250,58 @@ public String getTimelinePath() { /** * @returns true is meta fields need to be populated. else returns false. + * + * <p>Derived from {@link #getMetaFieldsMode()} so that call sites still written against the + * deprecated boolean observe the same answer as the enum: only {@link MetaFieldsMode#ALL} + * populates every meta column. Selective modes report {@code false} here, which keeps + * key-dependent machinery (bloom filters, record-level index) correctly disabled. */ public boolean populateMetaFields() { + return getMetaFieldsMode().toLegacyPopulateMetaFields(); + } + + /** + * @return the raw, deprecated {@code hoodie.populate.meta.fields} value, used only as the + * fallback when {@link #META_FIELDS_MODE} is absent. Callers should use + * {@link #getMetaFieldsMode()} instead. + */ + private boolean legacyPopulateMetaFields() { return Boolean.parseBoolean(getStringOrDefault(POPULATE_META_FIELDS)); } + /** + * @return the {@link MetaFieldsMode} resolved from the on-disk properties. {@link #META_FIELDS_MODE} + * is the source of truth; tables written before that property existed fall back to + * {@link MetaFieldsMode#ALL} or {@link MetaFieldsMode#NONE} based on the deprecated + * {@link #POPULATE_META_FIELDS} boolean. + */ + public MetaFieldsMode getMetaFieldsMode() { + return MetaFieldsMode.resolve(getStringOrDefault(META_FIELDS_MODE), legacyPopulateMetaFields()); Review Comment: Worth revisiting: the answer changed in the latest push, and it moved toward what you were asking for. Inference now happens **once per write**, in `BaseHoodieWriteClient#validateAgainstTableProperties`, rather than being re-derived at every read of the property. A writer that states neither meta-field property adopts the table's mode there, and the derived legacy boolean is set alongside it so the ~55 unmigrated `populateMetaFields()` call sites observe a consistent answer. Downstream code then reads a config that already agrees with the table. What remains per-read is `HoodieTableConfig#getMetaFieldsMode` consulting the legacy boolean when the mode property is absent, which only happens for tables written before the property existed. I would still rather not cache that in a field: `HoodieTableConfig` is mutable after load (`setValue` / `setAll` / `clearValue`, used by the upgrade paths and tests), so a cached enum would go stale on any property update — a correctness hazard in exchange for a map lookup plus a `valueOf`. And on the one-time migration reading: `NineToTenUpgradeHandler` does exactly that — a v9 table upgraded to v10 gets `hoodie.meta.fields.mode` written explicitly, derived from the boolean, so the fallback stops being consulted for it. ########## hudi-hadoop-common/src/main/java/org/apache/hudi/io/storage/hadoop/HoodieAvroParquetWriter.java: ########## @@ -57,30 +60,57 @@ public HoodieAvroParquetWriter(StoragePath file, 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, Review Comment: Still your call — leaving open. The boolean overload on `HoodieAvroParquetWriter` is `@Deprecated` with a javadoc pointing at the `MetaFieldsMode` one and noting it cannot express the selective modes. I kept it because it has a live production caller (`SparkHelpers.scala:74`) plus four test call sites, all of which only ever distinguish all-or-nothing meta fields, so converting them would be churn without behavior change. The Spark equivalent had zero callers and was removed in `7dcd9d1`. If you would rather the boolean be gone entirely from this class, say so and I will convert the five call sites in this PR. -- 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]
