hudi-agent commented on code in PR #19858:
URL: https://github.com/apache/hudi/pull/19858#discussion_r3947776492
##########
hudi-common/src/main/java/org/apache/hudi/common/engine/RecordContext.java:
##########
@@ -440,11 +441,8 @@ private SerializableBiFunction<T, HoodieSchema, String>
metadataKeyExtractor() {
return (record, schema) -> getValue(record, schema,
RECORD_KEY_METADATA_FIELD).toString();
}
- private SerializableBiFunction<T, HoodieSchema, String>
virtualKeyExtractor(String[] recordKeyFields) {
- if (recordKeyFields.length == 1) {
- // there might be consistency for record key encoding when partition
fields are multiple for cow merging,
- // currently the incoming records are using the keys from HoodieRecord
which utilities the write config and by default encodes the field name with the
value
- // while here the field names are ignored, this function would be used
to extract record keys from old base file.
+ private SerializableBiFunction<T, HoodieSchema, String>
virtualKeyExtractor(String[] recordKeyFields, int numPartitionFields) {
+ if (recordKeyFields.length == 1 && numPartitionFields <= 1) {
Review Comment:
🤖 This matches ComplexKeyGenerator / RowDataKeyGen, but
`CustomAvroKeyGenerator` hands a single record key to `SimpleAvroKeyGenerator`
regardless of how many partition fields there are (e.g. `a:simple,b:timestamp`
-> 2 fields after `getPartitionFields()` strips the types), so it writes `k1`
while this extractor now yields `column1:k1`. Before this change the single-key
branch returned the raw value and matched. Spark permits any
BuiltinKeyGenerator with virtual keys, so a NONE/selective-mode table using
CustomKeyGenerator with multiple partition fields would stop matching old-file
keys on the file-group-reader merge path (updates turning into duplicates).
Could you confirm that combination, and whether the extractor should consult
`tableConfig.getKeyGeneratorClassName()` rather than only the partition-field
count?
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/io/HoodieConcatHandle.java:
##########
@@ -93,8 +94,8 @@ public HoodieConcatHandle(HoodieWriteConfig config, String
instantTime, HoodieTa
*/
@Override
public void write(HoodieRecord oldRecord) {
- HoodieSchema oldSchema = config.populateMetaFields() ?
writeSchemaWithMetaFields : writeSchema;
- String key = oldRecord.getRecordKey(oldSchema, keyGeneratorOpt);
+ HoodieSchema oldSchema = config.getMetaFieldsMode() == MetaFieldsMode.NONE
? writeSchema : writeSchemaWithMetaFields;
Review Comment:
🤖 This reads the mode from the write config, while the parent's
`metaFieldsMode` field (made `protected` in this PR) deliberately reads it from
the table config -- the note on BaseCreateHandle explains that a writer
carrying a stale `hoodie.populate.meta.fields` would otherwise resolve to NONE
against a table whose files are populated. Since `writeToFile` right below
consults the table-config value for `isFileNamePopulated`, would it make sense
to use `metaFieldsMode` here too so the schema choice and the file-name rewrite
agree?
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
hudi-client/hudi-flink-client/src/main/java/org/apache/hudi/io/FlinkConcatHandle.java:
##########
@@ -56,8 +56,9 @@ public FlinkConcatHandle(HoodieWriteConfig config, String
instantTime, HoodieTab
*/
@Override
public void write(HoodieRecord oldRecord) {
- HoodieSchema oldSchema = config.populateMetaFields() ?
writeSchemaWithMetaFields : writeSchema;
- String key = oldRecord.getRecordKey(oldSchema, keyGeneratorOpt);
+ // HoodieMergeHelper supplies metadata columns even when NONE leaves their
values null.
Review Comment:
🤖 nit: the comment "HoodieMergeHelper supplies metadata columns even when
NONE leaves their values null" is a bit hard to parse — could you reword to
something like "Always use the schema with meta fields; HoodieMergeHelper
relies on their presence even in NONE mode, where the values are simply null"?
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
hudi-client/hudi-flink-client/src/main/java/org/apache/hudi/io/storage/row/HoodieRowDataLanceWriter.java:
##########
@@ -118,12 +119,20 @@ public void writeRow(String key, RowData row) throws
IOException {
@Override
public void writeRowWithMetaData(HoodieKey key, RowData row) throws
IOException {
- if (populateMetaFields) {
- RowData rowWithMeta = updateRecordMetadata(row, key,
getWrittenRecordCount());
- writeRow(key.getRecordKey(), rowWithMeta);
+ RowData rowWithMeta;
Review Comment:
🤖 nit: this three-way branch on `metaFieldsMode` building `rowWithMeta` is
duplicated almost verbatim in
`HoodieRowDataParquetWriter.writeRowWithMetaData`. Might be worth pulling into
a shared static helper (e.g. on `HoodieRowDataCreation`) to avoid the two
copies drifting apart.
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
hudi-flink-datasource/hudi-flink/src/main/java/org/apache/hudi/sink/bulk/BulkInsertWriterHelper.java:
##########
@@ -115,20 +118,21 @@ public BulkInsertWriterHelper(Configuration conf,
HoodieTable hoodieTable, Hoodi
? schema
: HoodieSchemaUtils.addMetadataFields(schema,
writeConfig.allowOperationMetadataField());
this.preserveHoodieMetadata = preserveHoodieMetadata;
+ this.preserveRecordKey = preserveHoodieMetadata &&
metaFieldsMode.isRecordKeyPopulated();
this.isInputSorted = OptionsResolver.isBulkInsertOperation(conf)
&& (conf.get(FlinkOptions.WRITE_BULK_INSERT_SORT_INPUT)
|| OptionsResolver.isLsmTreeStorageLayout(conf));
this.fileIdPrefix = UUID.randomUUID().toString();
- this.keyGen = preserveHoodieMetadata ? null :
RowDataKeyGens.instance(conf, rowType, taskPartitionId, instantTime);
+ this.keyGen = preserveRecordKey ? null : RowDataKeyGens.instance(conf,
rowType, taskPartitionId, instantTime);
this.writeMetrics = writeMetrics;
}
public void write(RowData record) throws IOException {
try {
- String recordKey = preserveHoodieMetadata
+ String recordKey = preserveRecordKey
? record.getString(HoodieRecord.RECORD_KEY_META_FIELD_ORD).toString()
Review Comment:
🤖 nit: small grammar fix — "only ALL mode populate the partition path"
should read "only ALL mode populates the partition path".
<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]
