nsivabalan commented on code in PR #19205:
URL: https://github.com/apache/hudi/pull/19205#discussion_r3767570756
##########
hudi-common/src/main/java/org/apache/hudi/common/table/HoodieTableMetaClient.java:
##########
@@ -1535,7 +1554,37 @@ public Properties build() {
tableConfig.setValue(HoodieTableConfig.CDC_SUPPLEMENTAL_LOGGING_MODE,
cdcSupplementalLoggingMode);
}
}
- if (null != populateMetaFields) {
+ // hoodie.meta.fields.mode is the source of truth, and hoodie.properties
must never contradict
+ // it: a table written selectively that still recorded
populate.meta.fields=true would be read
+ // as ALL by a pre-1.3.0 reader, which ignores the mode property
entirely. For NONE that is
+ // actively unsafe — an older incremental reader would run against
all-null commit times and
+ // silently return no rows.
+ //
+ // A caller that states both and disagrees is rejected rather than
silently overridden. Half
+ // their request would otherwise be discarded without a word, and it
would be inconsistent with
+ // BaseHoodieWriteClient#validateAgainstTableProperties, which already
rejects an explicitly-set
+ // boolean that disagrees with the table. Only a genuine contradiction
fails: ALL + true and
+ // NONE + false are coherent restatements and pass.
+ if (null != metaFieldsMode) {
+ boolean derivedPopulateMetaFields =
metaFieldsMode.toLegacyPopulateMetaFields();
+ if (null != populateMetaFields && populateMetaFields !=
derivedPopulateMetaFields) {
+ throw new HoodieException(String.format(
+ "Conflicting meta-field settings at table creation: %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(), metaFieldsMode,
+ HoodieTableConfig.POPULATE_META_FIELDS.key(),
derivedPopulateMetaFields,
+ HoodieTableConfig.POPULATE_META_FIELDS.key(), populateMetaFields,
+ HoodieTableConfig.META_FIELDS_MODE.key(),
+ HoodieTableConfig.POPULATE_META_FIELDS.key(),
derivedPopulateMetaFields));
+ }
+ tableConfig.setValue(HoodieTableConfig.META_FIELDS_MODE,
metaFieldsMode.name());
+ tableConfig.setValue(HoodieTableConfig.POPULATE_META_FIELDS,
+ Boolean.toString(derivedPopulateMetaFields));
+ } else if (null != populateMetaFields) {
+ // No explicit mode: preserve pre-1.3.0 behavior and record only the
legacy boolean, which
+ // resolves to ALL / NONE on read.
Review Comment:
It does — `TableBuilder` sets the mode and derives the boolean from it
(`HoodieTableMetaClient:1582`):
```java
tableConfig.setValue(HoodieTableConfig.META_FIELDS_MODE,
metaFieldsMode.name());
if (null == tableVersion || tableVersion.lesserThan(HoodieTableVersion.TEN))
{
tableConfig.setValue(HoodieTableConfig.POPULATE_META_FIELDS,
Boolean.toString(derivedPopulateMetaFields));
}
```
Two things worth calling out, since neither is obvious from the line itself:
**The boolean is version-scoped.** A table created at v10 or later gets the
mode only; below v10 it gets both. The reason for writing both on older tables
is that selective modes have to work on any version 1.x can write, not just the
latest — a fleet adopting this runs patched and unpatched pipelines against the
same v6 tables for a while, and an unpatched reader knows only the boolean.
With the property absent it falls back to its `true` default and would treat a
selective table as `ALL`, over-claiming columns that are physically null.
Writing the derived `false` makes such a reader under-claim instead.
**The boolean is always derived, never taken from the caller.** A caller
that passes a boolean contradicting an explicit mode is rejected here rather
than silently overridden (the check just above, at :1565-1581). That is what
keeps `hoodie.properties` from self-contradicting.
Relatedly, `META_FIELDS_MODE` deliberately carries no `sinceVersion`.
`dropInvalidConfigs` strips properties newer than the table version on load, so
declaring one would silently delete the mode from a v6 table and revert it to
`ALL`. There is a NOTE on the `ConfigProperty` so it does not get added back by
reflex.
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/config/HoodieWriteConfig.java:
##########
@@ -3892,6 +3998,8 @@ public HoodieWriteConfig build() {
@VisibleForTesting
public HoodieWriteConfig build(boolean shouldValidate) {
setDefaults();
+ // Before validate(), so the MoR / engine-type checks see the same mode
the built config will.
+ deriveLegacyPopulateMetaFieldsFromMode();
Review Comment:
I would rather not, and the reason is that this method can fail while
`setDefaults` cannot.
`setDefaults` is a run of `setDefaultValue` calls — fill in what the caller
omitted, never reject anything. `deriveLegacyPopulateMetaFieldsFromMode` throws
when the caller set `populate.meta.fields` to something the mode contradicts:
```java
private void deriveLegacyPopulateMetaFieldsFromMode() {
String rawMode = writeConfig.getString(HoodieTableConfig.META_FIELDS_MODE);
if (StringUtils.isNullOrEmpty(rawMode)) {
return;
}
boolean derived =
MetaFieldsMode.parse(rawMode).toLegacyPopulateMetaFields();
// ... rejects a genuine contradiction rather than silently overriding it
```
Folding a throw into `setDefaults` would make a method every builder path
calls for defaulting into one that can also reject the config, which I think
costs more in surprise than the extra line saves.
There is also an ordering constraint: it has to run *before* `validate()`,
so the MoR and engine-type checks see the same mode the built config will
carry. It currently sits between the two with a comment saying so. Inside
`setDefaults` that ordering would still hold, but it would stop being visible
at the call site.
If the concern is that `build()` is accumulating steps, I would rather
rename this to something that admits it validates —
`reconcileMetaFieldsProperties`, say — than move it under a name that promises
it only fills in defaults. Happy to do that if you prefer.
--
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]
