nsivabalan commented on code in PR #19205:
URL: https://github.com/apache/hudi/pull/19205#discussion_r3782126219
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/BaseHoodieWriteClient.java:
##########
@@ -1543,13 +1544,63 @@ protected boolean loadActiveTimelineOnTableInit() {
return true;
}
+ /**
+ * Pure validation: this method reads both configs and throws, and never
modifies either.
+ *
+ * <p>Nothing reconciles the write config against the table beforehand. That
is deliberate: the mode
+ * is read further down the write path by handles and writer factories, some
of which hold no table
+ * config at all, so the write config has to be correct on its own rather
than corrected on the way
+ * in. This gate is what makes that true, by refusing writes whose
meta-field settings do not already
+ * agree with the table.
+ */
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");
+ // Meta-field population is physical, so a writer must not disagree with
the table about which
+ // meta columns hold values. Compare the full enum rather than the legacy
booleans: those collapse
+ // every selective mode to false, so a writer claiming COMMIT_TIME_ONLY
against a NONE table would
+ // slip through and advertise commit times that were never written.
+ //
+ // hoodie.meta.fields.mode is a table property, changeable only via
hudi-cli or an upgrade -- never
+ // as a side effect of a write.
+ MetaFieldsMode tableMetaFieldsMode = tableConfig.getMetaFieldsMode();
+ // Used only to phrase the error: the deprecated boolean counts as a
statement of intent too, so a
+ // writer passing populate.meta.fields=false is told it "requests NONE"
rather than that it failed
+ // to state anything.
+ boolean writerStatedMetaFields =
+ (writeConfig.contains(HoodieTableConfig.META_FIELDS_MODE)
+ &&
!StringUtils.isNullOrEmpty(writeConfig.getString(HoodieTableConfig.META_FIELDS_MODE)))
+ || writeConfig.contains(HoodieTableConfig.POPULATE_META_FIELDS);
+
+ // The writer's resolved mode must equal the table's. Both directions are
wrong, for different
+ // reasons: widening would leave earlier commits missing a column later
ones have, and readers
+ // cannot tell the two apart; narrowing would leave rows the table still
advertises as populated,
+ // which incremental queries then silently skip. Only hudi-cli or an
upgrade may change the mode.
+ //
+ // This is checked on the resolved values rather than only on what the
writer stated, so it also
+ // catches the writer that stated nothing at all. Such a writer resolves
to the ALL default, which
+ // agrees with an ALL table -- the overwhelmingly common case, and the
reason nearly every existing
+ // caller is unaffected -- but disagrees with every other mode. Against
those, saying nothing is
+ // not a request to inherit; it is a writer that has not been told, and it
would go on to stamp the
+ // wrong set of meta columns.
+ MetaFieldsMode writeMetaFieldsMode = writeConfig.getMetaFieldsMode();
+ if (writeMetaFieldsMode != tableMetaFieldsMode) {
Review Comment:
Done. It is only used to phrase the error, so it now lives inside the `if`.
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/config/HoodieWriteConfig.java:
##########
@@ -3587,11 +3619,64 @@ 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
public Builder withPopulateMetaFields(boolean populateMetaFields) {
writeConfig.setValue(HoodieTableConfig.POPULATE_META_FIELDS,
Boolean.toString(populateMetaFields));
return this;
}
+ public Builder withMetaFieldsMode(MetaFieldsMode metaFieldsMode) {
+ // Leaving the mode unset defers to the deprecated populate.meta.fields
boolean. The legacy
+ // boolean is derived from the mode in build() rather than here, so the
two cannot be made to
+ // disagree by calling the setters in either order.
+ writeConfig.setValue(HoodieTableConfig.META_FIELDS_MODE,
+ metaFieldsMode == null ? "" : metaFieldsMode.name());
+ return this;
+ }
+
+ /**
+ * Rewrite the deprecated {@code populate.meta.fields} boolean from {@code
meta.fields.mode}
+ * whenever a mode is set, so the two can never disagree on the resulting
config.
+ *
+ * <p>Done at build time, not in the setter: {@code
withPopulateMetaFields} does not re-derive
+ * the mode, so deriving in {@link #withMetaFieldsMode} alone would make
the invariant depend on
+ * call order. {@code
withMetaFieldsMode(COMMIT_TIME_ONLY).withPopulateMetaFields(true)} would
+ * leave a selective mode sitting next to {@code
populate.meta.fields=true} — a config that
+ * resolves correctly (the mode wins) but carries the contradiction to
disk on any path that
+ * copies raw write-config props into {@code hoodie.properties},
misleading pre-1.3.0 readers
+ * into treating the table as ALL.
+ */
+ private void deriveLegacyPopulateMetaFieldsFromMode() {
+ String rawMode =
writeConfig.getString(HoodieTableConfig.META_FIELDS_MODE);
+ if (StringUtils.isNullOrEmpty(rawMode)) {
+ return;
+ }
+ boolean derived =
MetaFieldsMode.parse(rawMode).toLegacyPopulateMetaFields();
+ // A caller that explicitly set the boolean to something the mode
contradicts is rejected rather
+ // than silently overridden — otherwise half their request is discarded
without a word. Only a
+ // genuine contradiction fails; restating the derived value (ALL + true,
NONE + false) passes.
+ // An absent boolean is the ordinary case and simply takes the derived
value.
+ checkArgument(
+ !writeConfig.contains(HoodieTableConfig.POPULATE_META_FIELDS)
+ ||
writeConfig.getBoolean(HoodieTableConfig.POPULATE_META_FIELDS) == derived,
+ () -> String.format(
+ "Conflicting meta-field settings on the write config: %s=%s
implies %s=%s, but %s was "
+ + "explicitly set to %s. %s is the source of truth and the
boolean is only its "
+ + "pre-1.3.0 fallback, so the two cannot be set to different
things. Drop %s, or set "
+ + "it to %s.",
+ HoodieTableConfig.META_FIELDS_MODE.key(), rawMode,
+ HoodieTableConfig.POPULATE_META_FIELDS.key(), derived,
+ HoodieTableConfig.POPULATE_META_FIELDS.key(),
+ writeConfig.getBoolean(HoodieTableConfig.POPULATE_META_FIELDS),
+ HoodieTableConfig.META_FIELDS_MODE.key(),
+ HoodieTableConfig.POPULATE_META_FIELDS.key(), derived));
+ writeConfig.setValue(HoodieTableConfig.POPULATE_META_FIELDS,
Boolean.toString(derived));
Review Comment:
Done. The `setValue` is now skipped when the property is already present --
it has passed the check above by then, so it equals `derived` and rewriting it
is a no-op.
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/io/BaseCreateHandle.java:
##########
@@ -171,6 +183,23 @@ protected HoodieRecord<T> updateFileName(HoodieRecord<T>
record, HoodieSchema sc
return record.prependMetaFields(schema, targetSchema, metadataValues,
prop);
}
+ /**
+ * Blank out {@code _hoodie_file_name} on a record being copied forward
under a mode that does not
+ * populate it.
+ *
+ * <p>Clearing rather than leaving the column alone is deliberate. The
record here came from the
+ * previous base file, so it can still carry a file name written while the
table was on
+ * {@code ALL} — and that name points at a file this record no longer lives
in. Passing null
+ * through {@link MetadataValues} would not do it either: {@code
updateMetadataValuesInternal}
+ * skips null entries (HoodieAvroIndexedRecord:383), so the stale value
would survive. Hence the
+ * explicit {@code updateMetaField}.
+ */
+ private HoodieRecord<T> clearFileName(HoodieRecord<T> record, HoodieSchema
schema, HoodieSchema targetSchema, Properties prop) {
Review Comment:
Agreed, and you are right about the scope of `preserveMetadata`. Removed the
clearing on both handles; `updateFileName` now sets the file name only when the
mode populates it, and the create-handle call site is back to a single call.
Checking your premise before changing it, since it decides whether the clear
was dead code or load-bearing: on this branch the mode is written in exactly
three places -- `TableBuilder` at creation, `NineToTenUpgradeHandler` (which
derives from the legacy boolean, so `ALL` or `NONE` only, never a selective
mode), and `TenToNineDowngradeHandler` (removal). So nothing narrows an
existing table to a selective mode, the source file cannot be carrying a stale
`_hoodie_file_name`, and the clear was indeed unnecessary.
One thing to flag rather than leave implicit: that changes with stacked PR
#19206, which adds `set-meta-fields-mode` and permits narrowing an existing
table. At that point a table narrowed from `ALL` to `COMMIT_TIME_ONLY` does
have base files carrying a real file name, and a table-service rewrite would
copy it forward into a table that advertises the column as unpopulated. I would
rather handle it there, where narrowing is introduced, than carry defensive
code here for a state this PR cannot produce -- but I have noted it on that PR
so it does not get lost.
Also applied the same simplification to
`HoodieWriteMergeHandle.writeToFile`, which had the equivalent explicit-null
write.
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/config/HoodieWriteConfig.java:
##########
@@ -3587,11 +3619,64 @@ 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
public Builder withPopulateMetaFields(boolean populateMetaFields) {
writeConfig.setValue(HoodieTableConfig.POPULATE_META_FIELDS,
Boolean.toString(populateMetaFields));
return this;
}
+ public Builder withMetaFieldsMode(MetaFieldsMode metaFieldsMode) {
+ // Leaving the mode unset defers to the deprecated populate.meta.fields
boolean. The legacy
+ // boolean is derived from the mode in build() rather than here, so the
two cannot be made to
+ // disagree by calling the setters in either order.
+ writeConfig.setValue(HoodieTableConfig.META_FIELDS_MODE,
+ metaFieldsMode == null ? "" : metaFieldsMode.name());
+ return this;
+ }
+
+ /**
+ * Rewrite the deprecated {@code populate.meta.fields} boolean from {@code
meta.fields.mode}
+ * whenever a mode is set, so the two can never disagree on the resulting
config.
+ *
+ * <p>Done at build time, not in the setter: {@code
withPopulateMetaFields} does not re-derive
+ * the mode, so deriving in {@link #withMetaFieldsMode} alone would make
the invariant depend on
+ * call order. {@code
withMetaFieldsMode(COMMIT_TIME_ONLY).withPopulateMetaFields(true)} would
+ * leave a selective mode sitting next to {@code
populate.meta.fields=true} — a config that
+ * resolves correctly (the mode wins) but carries the contradiction to
disk on any path that
+ * copies raw write-config props into {@code hoodie.properties},
misleading pre-1.3.0 readers
+ * into treating the table as ALL.
+ */
+ private void deriveLegacyPopulateMetaFieldsFromMode() {
Review Comment:
Good catch, and agreed on the principle -- if the mode is to be the single
truth, it should be present whenever the settings are determinable, not only
when the caller happened to name it.
The method returned early on an absent mode, so a config with only
`populate.meta.fields` kept no mode at all and every downstream reader had to
fall back to the boolean. Now derives `ALL` / `NONE` from a stated boolean in
that case, so `META_FIELDS_MODE` is always populated on a config that says
anything about meta fields.
Deliberately keyed on the *stated* boolean rather than `contains(...)`: an
inherited props blob carrying the boolean should not synthesise a mode the
caller never asked for, which is the same distinction behind the fix for
@voonhous's archival report on this file.
--
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]
