hudi-agent commented on code in PR #19518:
URL: https://github.com/apache/hudi/pull/19518#discussion_r3717996733
##########
hudi-flink-datasource/hudi-flink/src/main/java/org/apache/hudi/sink/bulk/BulkInsertWriterHelper.java:
##########
@@ -80,7 +80,8 @@ public class BulkInsertWriterHelper implements AutoCloseable {
private String lastKnownPartitionPath = null;
private final String fileIdPrefix;
private int numFilesWritten = 0;
- protected final Map<String, HoodieRowDataCreateHandle> handles = new
HashMap<>();
+ // String keys are used by existing paths; NBCC bucket writes use
HoodieFileGroupId keys.
+ protected final Map<Object, HoodieRowDataCreateHandle> handles = new
HashMap<>();
Review Comment:
🤖 nit: `Map<Object, HoodieRowDataCreateHandle>` makes it hard to reason
about what keys are valid here — a future reader has to hunt down the comment
to understand the two possible types. Could you consider two separate maps (one
keyed by `String`, one by `HoodieFileGroupId`) or a thin wrapper, so the intent
stays visible in the type system?
<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/bucket/BucketBulkInsertWriterHelper.java:
##########
@@ -125,12 +143,24 @@ public static RowData rowWithFileId(Map<String, String>
bucketIdToFileId, RowDat
indexKeyFields,
numBucketsFunction,
needFixedFileIdSuffix);
- return GenericRowData.of(StringData.fromString(fileId), record);
+ return needFixedFileIdSuffix
Review Comment:
🤖 The sort row here is built with `needFixedFileIdSuffix` (the 3-field NBCC
layout), but `write()` decodes it based on `isNonBlockingConcurrencyControl`
read independently from conf. Today both derive from
`OptionsResolver.isNonBlockingConcurrencyControl(conf)`, so they always agree —
but if a caller ever passed `needFixedFileIdSuffix=true` without NBCC on the
writer (or vice versa), the field offsets would silently misalign and read
fileId as partition path. Would it be worth deriving both from a single source,
or asserting they match, to make that invariant explicit?
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
hudi-flink-datasource/hudi-flink/src/test/java/org/apache/hudi/sink/utils/BulkInsertFunctionWrapper.java:
##########
@@ -245,8 +249,10 @@ private void setupSortOperator() throws Exception {
.setExecutionConfig(new ExecutionConfig().enableObjectReuse())
.build();
SortOperatorGen sortOperatorGen = lsmSortInput
- ?
LsmBucketBulkInsertWriterHelper.getFileIdAndKeySorterGen(sortInputRowType)
- : BucketBulkInsertWriterHelper.getFileIdSorterGen(rowTypeWithFileId);
+ ? LsmBucketBulkInsertWriterHelper.getFileIdAndKeySorterGen(
+ sortInputRowType,
OptionsResolver.isNonBlockingConcurrencyControl(conf))
+ : BucketBulkInsertWriterHelper.getFileIdSorterGen(
Review Comment:
🤖 nit: `OptionsResolver.isNonBlockingConcurrencyControl(conf)` is called
twice in `setupSortOperator` — could you hoist it into a local `boolean isNbcc`
at the top of the method so it's obvious both branches use the same value?
<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]
