hudi-agent commented on code in PR #19200:
URL: https://github.com/apache/hudi/pull/19200#discussion_r3528913231
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/io/cdc/HoodieNativeLogFormatWriter.java:
##########
@@ -237,23 +234,12 @@ private Map<HeaderMetadataType, String>
addRecordPositionsIfRequired(
return header;
}
- Set<Long> positionSet = new HashSet<>(recordPositions.size());
- long previousPosition = Long.MIN_VALUE;
- for (Long position : recordPositions) {
- // RECORD_POSITIONS is encoded as a bitmap and decoded in ascending
order. Native log records
- // are read back in file write order, so only publish positions when
both orders match.
- if (!HoodieRecordLocation.isPositionValid(position) || position <=
previousPosition) {
- Map<HeaderMetadataType, String> updatedHeader = new HashMap<>(header);
-
updatedHeader.remove(HeaderMetadataType.BASE_FILE_INSTANT_TIME_OF_RECORD_POSITIONS);
- return updatedHeader;
- }
- previousPosition = position;
- positionSet.add(position);
- }
-
Map<HeaderMetadataType, String> updatedHeader = new HashMap<>(header);
- if (positionSet.size() == recordPositions.size()) {
- updatedHeader.put(HeaderMetadataType.RECORD_POSITIONS,
LogReaderUtils.encodePositions(positionSet));
+ Option<String> encodedRecordPositions =
LogReaderUtils.encodePositionsAsLongList(recordPositions);
Review Comment:
🤖 The previous logic dropped position metadata when it saw duplicate
positions; now duplicates are preserved and used for position-based merge. In
`PositionBasedFileGroupRecordBuffer`, records are keyed by position
(`records.put(recordPosition, ...)` / `records.remove(nextRecordPosition)`), so
two records sharing a position within one block would collide and the earlier
update would be silently lost. Can native data/delete blocks ever emit
duplicate positions? If not it might be worth keeping a guard (or asserting
uniqueness) so we fail loud rather than drop an update.
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
hudi-common/src/main/java/org/apache/hudi/common/table/log/block/HoodieNativeLogDataBlock.java:
##########
@@ -69,6 +71,15 @@ public HoodieLogBlockType getBlockType() {
return HoodieLogBlockType.PARQUET_DATA_BLOCK;
}
+ @Override
+ public List<Long> getRecordPositionList() throws IOException {
Review Comment:
🤖 nit: `HoodieNativeLogDataBlock` and `HoodieNativeLogDeleteBlock` now have
byte-for-byte identical `getRecordPositionList()` overrides — could you pull
this into a protected static helper in `HoodieLogBlock` (or a thin shared
method in `LogReaderUtils`) so there's only one place to update if the encoding
changes?
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
hudi-common/src/main/java/org/apache/hudi/common/table/read/buffer/PositionBasedFileGroupRecordBuffer.java:
##########
@@ -306,22 +302,12 @@ protected static List<Long>
extractRecordPositions(HoodieLogBlock logBlock,
blockBaseFileInstantTime, baseFileInstantTime);
return null;
}
- Roaring64NavigableMap positions = logBlock.getRecordPositions();
- if (positions == null || positions.isEmpty()) {
+ List<Long> blockPositions = logBlock.getRecordPositionList();
+ if (blockPositions == null || blockPositions.isEmpty()) {
Review Comment:
🤖 nit: `getRecordPositionList()` never returns null — the base-class
implementation returns an `ArrayList` and both native-log overrides return
either `Collections.emptyList()` or a decoded list. The `blockPositions ==
null` branch is dead code and may make a future reader wonder what code path
produces null here. Could you drop the null guard and just keep
`blockPositions.isEmpty()`?
<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]
