nsivabalan commented on code in PR #19205:
URL: https://github.com/apache/hudi/pull/19205#discussion_r3767578471
##########
hudi-client/hudi-spark-client/src/main/java/org/apache/hudi/common/model/HoodieSparkRecord.java:
##########
@@ -296,9 +297,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(
+ props.getProperty(HoodieTableConfig.META_FIELDS_MODE.key()),
Review Comment:
Nearly — the catch is that this method does not receive a
`HoodieTableConfig`. It is
`HoodieRecordCompatibilityInterface#wrapIntoHoodieRecordPayloadWithKeyGen(HoodieSchema,
Properties, Option<BaseKeyGenerator>)`, so what arrives is a raw `Properties`
bag, and which bag depends on the caller — a write config in most paths, and
nothing guarantees it carries the mode rather than only the legacy boolean.
That is why it goes through `MetaFieldsMode.resolve(mode, boolean)`: it is
the same resolution rule used on the table config, so it gets the right answer
whether the props carry the mode, the boolean, or both. Reading
`META_FIELDS_MODE` alone would resolve a props bag that has only
`populate.meta.fields=false` to `ALL`, and then take the meta-column branch
below and NPE on the null `_hoodie_record_key` ordinal — which is the exact bug
this call site was fixed for.
So the check is arguably in the wrong *place* rather than the wrong *shape*.
If the interface took the table config, or the write config were guaranteed to
carry the mode, one property would be enough. Making that guarantee real is the
"thread the table config through" option — it also covers the three
`getFileWriter` call sites that have no table config, and I would rather do it
as its own PR than fold it in here, since the interface is implemented by
Flink, Hive, and the Trino plugin.
Worth noting the new validation gate makes the *write* path safe by a
different route: a writer whose resolved mode disagrees with the table is now
refused outright, so by the time records are being wrapped, the write config
agrees with the table by construction. The `resolve` call here is then
belt-and-braces for the paths that reach this method without going through
`initTable` at all.
--
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]
