nsivabalan commented on code in PR #19205:
URL: https://github.com/apache/hudi/pull/19205#discussion_r3767704945
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/BaseHoodieWriteClient.java:
##########
@@ -1544,13 +1550,83 @@ 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.
+ *
+ * <p>Called explicitly from {@link #initTable} rather than from
+ * {@link #validateAgainstTableProperties}: resolving what this writer will
use is a separate
+ * concern from checking it, and a method named "validate" must not quietly
rewrite the config it
+ * is handed. Read-only entry points that only validate (marker deletion,
dry-run partition
+ * listing) therefore do not resolve, which is correct -- they write no
records.
Review Comment:
Good catch, and confirmed — this was a real false positive, now fixed in
a3c753d.
You are right that it is worse than the inheritance question: those two
commands write no records, so failing them on meta-field disagreement is pure
noise. It also reaches further than the two you named —
`SparkMain#upgradeOrDowngradeTable` shares the same `getWriteConfig`, so an
upgrade or downgrade of a selective table would have hit it too.
The fix is to give them a config that reflects the table rather than to
exempt them from the check. Both have the table in hand:
- `SparkMain#getWriteConfig` now reads the mode off the table config (`jsc`
threaded through for the storage conf), so every command built on it — marker
cleanup, upgrade/downgrade, rollback, clean — inherits the table's mode.
- `HoodieDropPartitionsTool` seeds it into `props` in its constructor, where
a `metaClient` already exists, covering both `dryRun` and the delete path.
Neither overrides a mode the operator named explicitly, and both fall back
to the default if the table cannot be read (the base path not being a Hudi
table is the command's own error to report, not this lookup's).
Worth stating the distinction this draws, since it is the reason the
ingestion path does *not* get the same treatment: an ingestion writer must
state the mode, because the write path reads it again in handles and writer
factories — three of the eight `getFileWriter` call sites hold no table config
at all, so nothing there can repeat a lookup. A CLI command runs once against a
table it can already read, so it can afford one. Reading the table config to
build a write config is fine for maintenance tooling and wrong as a general
mechanism.
##########
hudi-client/hudi-client-common/src/test/java/org/apache/hudi/table/upgrade/TestNineToTenUpgradeHandler.java:
##########
@@ -0,0 +1,87 @@
+/*
+ * 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.assertFalse;
+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());
Review Comment:
Done in 59eb54a — static import, matching `TestTenToNineDowngradeHandler`.
--
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]
