hudi-agent commented on code in PR #19206: URL: https://github.com/apache/hudi/pull/19206#discussion_r3737565291
########## hudi-common/src/main/java/org/apache/hudi/common/model/MetaFieldsMode.java: ########## @@ -0,0 +1,213 @@ +/* + * 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; + } + + /** + * @return true for the modes that populate some but not all meta columns, i.e. everything except + * {@link #ALL} and {@link #NONE}. + * + * <p>These are the modes the deprecated {@code hoodie.populate.meta.fields} boolean cannot + * express, so they are what callers gate on when a code path only understands all-or-nothing meta + * fields — writer engines not yet wired for selective population, table versions that predate the + * mode property, and validation that must not let a two-state writer speak for a five-state table. + */ + public boolean isSelective() { + return this != ALL && this != NONE; + } + + /** + * 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)); + } + + /** + * 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) { + 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); Review Comment: 🤖 nit: the allowed-values list in the error message is hardcoded, so adding a sixth enum constant would silently leave the message stale. Could you replace it with `Arrays.stream(values()).map(Enum::name).collect(Collectors.joining(", "))` so it stays in sync automatically? <sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag quality.</i></sub> ########## hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/BaseHoodieWriteClient.java: ########## @@ -1544,13 +1545,80 @@ protected boolean loadActiveTimelineOnTableInit() { return true; } + /** + * Adopt the table's {@code hoodie.meta.fields.mode} when this writer did not state one. + * + * <p>The mode is a table property: it is settable at table creation, through hudi-cli, or by an + * upgrade, and never by an ordinary write. Callers routinely build a write config without + * restating the table's meta-field settings — table services do, and so does a restarted + * StreamSync — and such a writer must write what the table already advertises rather than silently + * narrowing it. Without this, an unstated writer resolves to {@code NONE} (via the deprecated + * {@code hoodie.populate.meta.fields} fallback) and writes null meta columns into a table whose + * earlier files have them populated. + * + * <p>Inheritance applies only when the writer states <em>neither</em> property. If it explicitly + * set the mode or the deprecated boolean, that value is left alone so the comparison below can + * reject it on a mismatch: a user who deliberately passed {@code populate.meta.fields=false} + * against an {@code ALL} table should be told the setting conflicts, not have it silently + * overridden. + * + * @return true when the writer stated neither property, i.e. the mode was inherited and cannot + * disagree with the table. + */ + private static boolean inferMetaFieldsModeFromTable(HoodieTableConfig tableConfig, HoodieWriteConfig writeConfig) { Review Comment: 🤖 `inferMetaFieldsModeFromTable` mutates `writeConfig` as a side effect, and that mutation is what makes selective/NONE writers emit the right meta columns — the create/merge handles resolve the mode from this same config later during the write. It works today because `initTable` validates the shared `config` in place before any handle is built, but it's a fairly hidden dependency for something that silently drives on-disk layout: a caller that validates a defensive copy, or reads the mode before `validateAgainstTableProperties` runs, would quietly write the wrong meta columns with no error. Would it be worth lifting the inheritance into an explicit config-normalization step (or renaming to signal it mutates), so the data-correctness dependency isn't buried inside a `validate` method? @yihua <sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag quality.</i></sub> ########## hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/BaseHoodieWriteClient.java: ########## @@ -1544,13 +1545,80 @@ protected boolean loadActiveTimelineOnTableInit() { return true; } + /** + * Adopt the table's {@code hoodie.meta.fields.mode} when this writer did not state one. + * + * <p>The mode is a table property: it is settable at table creation, through hudi-cli, or by an + * upgrade, and never by an ordinary write. Callers routinely build a write config without + * restating the table's meta-field settings — table services do, and so does a restarted + * StreamSync — and such a writer must write what the table already advertises rather than silently + * narrowing it. Without this, an unstated writer resolves to {@code NONE} (via the deprecated + * {@code hoodie.populate.meta.fields} fallback) and writes null meta columns into a table whose + * earlier files have them populated. + * + * <p>Inheritance applies only when the writer states <em>neither</em> property. If it explicitly + * set the mode or the deprecated boolean, that value is left alone so the comparison below can + * reject it on a mismatch: a user who deliberately passed {@code populate.meta.fields=false} + * against an {@code ALL} table should be told the setting conflicts, not have it silently + * overridden. + * + * @return true when the writer stated neither property, i.e. the mode was inherited and cannot + * disagree with the table. + */ + private static boolean inferMetaFieldsModeFromTable(HoodieTableConfig tableConfig, HoodieWriteConfig writeConfig) { + boolean statedMode = writeConfig.contains(HoodieTableConfig.META_FIELDS_MODE) Review Comment: 🤖 nit: `inferMetaFieldsModeFromTable` is declared to return `boolean`, but its only call site discards the value — have you considered changing the return type to `void`? The Javadoc for the return value (`@return true when the writer stated neither property`) is meaningful, but since no caller acts on it, a reader will search for where it matters and find nothing. <sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag quality.</i></sub> -- 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]
