nsivabalan commented on code in PR #19205:
URL: https://github.com/apache/hudi/pull/19205#discussion_r3767550451
##########
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)
+ &&
!StringUtils.isNullOrEmpty(writeConfig.getString(HoodieTableConfig.META_FIELDS_MODE));
+ boolean statedLegacyBoolean =
writeConfig.contains(HoodieTableConfig.POPULATE_META_FIELDS);
+ if (statedMode || statedLegacyBoolean) {
+ return false;
+ }
+ MetaFieldsMode tableMode = tableConfig.getMetaFieldsMode();
+ writeConfig.setValue(HoodieTableConfig.META_FIELDS_MODE, tableMode.name());
+ // Keep the derived boolean in step, so the ~55 call sites still reading
populateMetaFields()
+ // observe an answer consistent with the mode.
+ writeConfig.setValue(HoodieTableConfig.POPULATE_META_FIELDS,
+ Boolean.toString(tableMode.toLegacyPopulateMetaFields()));
+ return true;
+ }
+
public void validateAgainstTableProperties(HoodieTableConfig tableConfig,
HoodieWriteConfig writeConfig) {
// 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");
+ // A writer that stated neither meta-field property inherits the table's
mode here, which makes
+ // the comparison below a no-op for it. A writer that stated either keeps
its value and is
+ // compared.
+ inferMetaFieldsModeFromTable(tableConfig, writeConfig);
Review Comment:
Done, on all four — and points 1 and 2 turned out to be load-bearing in a
way I had not appreciated when you raised them.
**2 (validate, don't mutate) — done.** `resolveMetaFieldsModeForWrite` is
deleted. `validateAgainstTableProperties` now reads both configs and throws,
and never modifies either.
**1 (infer at table-config construction, not here) — done.**
`HoodieTableConfig#getMetaFieldsMode()` resolves the mode from what is
persisted, via `MetaFieldsMode.resolve(mode, legacyBoolean)`. Nothing on the
write path infers.
**3 and 4 (check `populateMetaFields` for v9, `META_FIELDS_MODE` for v10) —
done, and unified.** Rather than branch on table version, the check compares
the *resolved* modes on both sides. That is equivalent to what you asked for,
because resolution is exactly "mode if set, else the legacy boolean, else
`ALL`" — so on a v9 table with only the boolean, comparing resolved modes *is*
comparing `populateMetaFields`. One code path, no version branch:
```java
MetaFieldsMode writeMetaFieldsMode = writeConfig.getMetaFieldsMode();
if (writeMetaFieldsMode != tableMetaFieldsMode) {
throw new HoodieException(...);
}
```
### Why removing the inference forced the rule to get stricter
Once nothing reconciles the write config, a writer that states nothing is no
longer harmless. It resolves to the `ALL` default, and that has to be compared
like any other value — so an unstated writer now **fails** against any table
that is not `ALL`, including `NONE`.
That is a real behavior change and I want it on the record rather than
buried. The reason it is the right one is the write path below this method: the
mode is read again by the handles and by `HoodieAvroFileWriterFactory` /
`HoodieSparkFileWriterFactory`, each deriving it from the `HoodieConfig` it is
handed, and three of the eight production `getFileWriter` call sites
(`ParquetUtils`, `HoodieNativeLogFormatWriter`, `HoodieNativeCDCFileWriter`)
hold no table config at all. Inference at `initTable` would fix this method and
leave all of those reading a write config that disagrees with the table.
Refusing the write is what makes the write config trustworthy everywhere
downstream.
An `ALL` table — the default — still accepts a writer that says nothing,
since such a writer already resolves to `ALL`. So existing callers there are
unaffected, table services included. A `NONE` table already required
`populate.meta.fields=false` on the writer before this PR. In practice the new
requirement lands on the three selective modes.
I got this wrong once in between, and a test caught it: I first scoped the
requirement to selective tables only, which left a `NONE` table accepting an
unstated writer that would then populate all five meta columns on a table that
has none of them.
`validateAgainstTablePropertiesRejectsADefaultWriterAgainstANoneTable` pins it
now.
Pushed in be97fa5. Tests: 12 `validateAgainstTableProperties*` cases, 7
end-to-end streamer cases, and the 8 java-client cases that this rule was
breaking in CI.
--
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]
