nsivabalan commented on code in PR #19205:
URL: https://github.com/apache/hudi/pull/19205#discussion_r3782117821
##########
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(
Review Comment:
Confirmed and fixed in 85fe7035. You were right on every link, including the
asymmetry: `derived=false` matches the explicit `false` for selective modes, so
it is `ALL` -- the default -- that broke.
Root cause was as you diagnosed: the check fired on
`writeConfig.contains(...)`, which cannot tell a value the caller stated on
this builder from one that arrived through `withProperties`. Those mean
different things -- stating both and contradicting yourself is an error;
stating one while the other rides in on an inherited blob is an override.
Implemented your suggestion: the builder tracks `statedPopulateMetaFields` /
`statedMetaFieldsMode` and only rejects a caller contradicting itself. An
explicit boolean now narrows an inherited mode; an explicitly-stated mode
overrides an inherited boolean.
Tests: three cases in `TestHoodieWriteConfigMetaFieldsMode` covering both
inherited directions plus the still-rejected self-contradiction, and
`TestHoodieTimelineArchiver#lsmWriterConfigSurvivesAnUpgradedTablesInheritedMode`
reproducing the LSM config shape on an upgraded table. Verified non-vacuous --
neutralising the fix reproduces your reported throw.
On `SevenToEightUpgradeHandler`: it does not build a config, it passes one
to `LSMTimelineWriter`, so the same fix covers it.
##########
hudi-common/src/main/java/org/apache/hudi/common/table/HoodieTableMetaClient.java:
##########
@@ -1535,7 +1570,50 @@ 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) {
Review Comment:
Confirmed and fixed in 633496ac. `TypedProperties.getBoolean(String,
boolean)` returns a primitive, so the argument was never null and any selective
mode was rejected at table creation.
Both sites now use the pattern you quoted, matching `StreamSync:482` and
`HoodieSparkSqlWriter`, which already carried it with a comment explaining
exactly this trap. These two were missed.
Added `unstatedBooleanFromPropsDoesNotContradictAnExplicitMode` to pin the
props-to-builder shape both bootstrap paths use.
##########
hudi-client/hudi-spark-client/src/main/java/org/apache/hudi/common/model/HoodieSparkRecord.java:
##########
@@ -296,9 +307,14 @@ public HoodieRecord
wrapIntoHoodieRecordPayloadWithKeyGen(HoodieSchema recordSch
StructType structType =
HoodieInternalRowUtils.getCachedSchema(recordSchema);
String key;
String partition;
- boolean populateMetaFields =
Boolean.parseBoolean(props.getOrDefault(POPULATE_META_FIELDS.key(),
- POPULATE_META_FIELDS.defaultValue().toString()).toString());
- if (!populateMetaFields && keyGen.isPresent()) {
+ // Resolve via hoodie.meta.fields.mode — reading the deprecated boolean
alone would report
+ // "populated" for a selective-mode table (whose _hoodie_record_key column
is null), sending us
+ // down the meta-column branch below and NPE-ing on the null ordinal.
+ boolean recordKeyPopulated = MetaFieldsMode.resolve(
Review Comment:
Both confirmed and fixed in 668e57b6, resolving through `MetaFieldsMode` as
you suggested.
`AutoRecordKeyGenerationUtils` is the serious one and your reasoning is
exactly right: bypassing that guard lets the write proceed into
`HoodieDatasetBulkInsertHelper`'s non-populate branch, which never runs the key
generator, so the rows land with no identity. I verified the invariant you
cited -- `TestAutoGenerationOfRecordKeys:174` asserts the boolean case throws,
and stating `NONE` walked straight past it.
`@CsvSource` extended with `hoodie.meta.fields.mode,NONE` and
`COMMIT_TIME_ONLY` as you asked.
One follow-on worth flagging: that extension immediately caught a second bug
of mine. The assertion checks the message contains `configKey + " is not
supported with auto generation of record keys"` as one contiguous phrase, and
my new message broke it with a parenthetical -- for the pre-existing boolean
row too. Fixed in 88b3b559; the message now names whichever of the two
properties the caller actually stated.
--
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]
