hudi-agent commented on code in PR #20282:
URL: https://github.com/apache/hudi/pull/20282#discussion_r4238576469
##########
hudi-common/src/main/java/org/apache/hudi/common/model/MetaFieldsMode.java:
##########
@@ -184,9 +184,22 @@ private static MetaFieldsMode parse(String rawMode) {
/**
* @return the equivalent value of the deprecated {@code
hoodie.populate.meta.fields} boolean, so
* that call sites not yet migrated to this enum keep observing consistent
behavior.
+ *
+ * <p>Only {@link #NONE} maps to {@code false}. The boolean has exactly two
states and {@code
+ * false} carries exactly one meaning -- no meta columns at all -- so every
mode that populates
+ * at least one column must map to {@code true}. Mapping the selective modes
to {@code false}
+ * instead stamps {@code hoodie.populate.meta.fields=false} into {@code
hoodie.properties} for
+ * tables whose files do carry meta columns, and a writer that predates
+ * {@code hoodie.meta.fields.mode} then reads that as "meta fields disabled"
and refuses to
+ * re-enable them.
+ *
+ * <p>This boolean cannot express which columns are populated, only whether
any are. Callers that
+ * need a specific column must ask for it -- {@link #isRecordKeyPopulated()}
for key-dependent
+ * machinery such as bloom filters and the record-level index -- rather than
reading this as
+ * "all five are present".
*/
public boolean toLegacyPopulateMetaFields() {
- return this == ALL;
+ return this != NONE;
Review Comment:
🤖 I think this flips about 50 in-tree callers of `populateMetaFields()` that
read it as "`_hoodie_record_key` is on disk", so a COMMIT_TIME_ONLY MoR table
would start keying log records and merges by that null column. Three examples
are `AbstractHoodieLogRecordScanner` L212,
`HoodieRecordMerger#getMandatoryFieldsForMerging`, and
`HoodieSparkCopyOnWriteTable#getUpdateHandle`, which skips the key generator.
Could you keep the in-tree semantics and change only the value persisted to
`hoodie.properties`, or move those callers to `isRecordKeyPopulated()` first?
@yihua could you weigh in on the compatibility trade-off here?
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
hudi-common/src/main/java/org/apache/hudi/common/model/MetaFieldsMode.java:
##########
@@ -184,9 +184,22 @@ private static MetaFieldsMode parse(String rawMode) {
/**
* @return the equivalent value of the deprecated {@code
hoodie.populate.meta.fields} boolean, so
* that call sites not yet migrated to this enum keep observing consistent
behavior.
+ *
+ * <p>Only {@link #NONE} maps to {@code false}. The boolean has exactly two
states and {@code
+ * false} carries exactly one meaning -- no meta columns at all -- so every
mode that populates
+ * at least one column must map to {@code true}. Mapping the selective modes
to {@code false}
+ * instead stamps {@code hoodie.populate.meta.fields=false} into {@code
hoodie.properties} for
+ * tables whose files do carry meta columns, and a writer that predates
+ * {@code hoodie.meta.fields.mode} then reads that as "meta fields disabled"
and refuses to
+ * re-enable them.
+ *
+ * <p>This boolean cannot express which columns are populated, only whether
any are. Callers that
+ * need a specific column must ask for it -- {@link #isRecordKeyPopulated()}
for key-dependent
+ * machinery such as bloom filters and the record-level index -- rather than
reading this as
+ * "all five are present".
*/
public boolean toLegacyPopulateMetaFields() {
- return this == ALL;
+ return this != NONE;
Review Comment:
🤖 `TenToNineDowngradeHandler` (L83-89) deliberately writes
`hoodie.populate.meta.fields=false` for selective modes so that v9 readers
under-claim, but with this change a downgraded COMMIT_TIME_ONLY table would
advertise `true`. A v9 reader would then key its merges on
`_hoodie_record_key`, which is null in those files. Could you take a look at
whether the downgrade path should keep writing `false` here?
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
hudi-common/src/main/java/org/apache/hudi/common/model/MetaFieldsMode.java:
##########
@@ -184,9 +184,22 @@ private static MetaFieldsMode parse(String rawMode) {
/**
* @return the equivalent value of the deprecated {@code
hoodie.populate.meta.fields} boolean, so
* that call sites not yet migrated to this enum keep observing consistent
behavior.
+ *
+ * <p>Only {@link #NONE} maps to {@code false}. The boolean has exactly two
states and {@code
+ * false} carries exactly one meaning -- no meta columns at all -- so every
mode that populates
+ * at least one column must map to {@code true}. Mapping the selective modes
to {@code false}
+ * instead stamps {@code hoodie.populate.meta.fields=false} into {@code
hoodie.properties} for
+ * tables whose files do carry meta columns, and a writer that predates
+ * {@code hoodie.meta.fields.mode} then reads that as "meta fields disabled"
and refuses to
+ * re-enable them.
+ *
+ * <p>This boolean cannot express which columns are populated, only whether
any are. Callers that
+ * need a specific column must ask for it -- {@link #isRecordKeyPopulated()}
for key-dependent
+ * machinery such as bloom filters and the record-level index -- rather than
reading this as
+ * "all five are present".
Review Comment:
🤖 nit: could you trim this Javadoc to the two key points (only NONE maps to
false; use isRecordKeyPopulated() for key-dependent logic)? The back-compat
rationale could live in the PR description.
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
--
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]
