Skip to content

Arrow: Fix int96 timestamp offset in arrow dictionary decode - #16435

Merged
sungwy merged 1 commit into
apache:mainfrom
sungwy:int96-timestamp-offset-fix
May 24, 2026
Merged

sungwy merged 1 commit into
apache:mainfrom
sungwy:int96-timestamp-offset-fix

Conversation

@sungwy

@sungwy sungwy commented May 20, 2026

Copy link
Copy Markdown
Contributor

Issue: #13485

This PR fixes the packed dictionary INT96 timestamp decode path in the VectorizedParquetDefinitionLevelReader to write using byte offsets rather than row indexes.

The unit test constructs a dictionary with expected values and verifies that the decoded Arrow buffer contains the expected timestamp values, and verifies that writing multiple rows of values did not corrupt the data as a result of the values being written into the wrong offsets.

The fix is the same as the earlier proposed change discussed in #13486 , and mirrors the offset handling used by other readers in the same class:

.setLong((long) idx * typeWidth, dict.decodeToLong(reader.readInteger()));

Disclosure:

Copilot AI review requested due to automatic review settings May 20, 2026 01:32
@sungwy
sungwy requested a review from nastra May 20, 2026 01:32
@github-actions github-actions Bot added the arrow label May 20, 2026
@sungwy sungwy changed the title Fix int96 timestamp offset in arrow dictionary decode Arrow: Fix int96 timestamp offset in arrow dictionary decode May 20, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes incorrect INT96 timestamp decoding for Parquet dictionary-encoded packed data in Iceberg’s Arrow vectorized reader by writing to Arrow buffers using byte offsets (instead of value indices), preventing buffer corruption and wrong timestamps.

Changes:

  • Correct TimestampInt96Reader.nextDictEncodedVal (PACKED mode) to use (long) idx * typeWidth when calling setLong.
  • Add a unit test that reproduces the corruption scenario and verifies correct decoded timestamps across multiple rows.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
arrow/src/main/java/org/apache/iceberg/arrow/vectorized/parquet/VectorizedParquetDefinitionLevelReader.java Fixes packed dictionary INT96 decode to write to Arrow buffers using byte offsets, matching other readers and preventing data corruption.
arrow/src/test/java/org/apache/iceberg/arrow/vectorized/parquet/TestVectorizedParquetDefinitionLevelReader.java Adds a regression test ensuring packed dictionary INT96 decoding writes correct per-row values without overwriting adjacent entries.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@sungwy
sungwy requested review from Fokko, kevinjqliu and rdblue May 20, 2026 15:11
.order(ByteOrder.LITTLE_ENDIAN);
long timestampInt96 = ParquetUtil.extractTimestampInt96(buffer);
vector.getDataBuffer().setLong(idx, timestampInt96);
vector.getDataBuffer().setLong((long) idx * typeWidth, timestampInt96);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this makes sense to me

@sungwy

sungwy commented May 22, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the review @nastra - I'll leave it open for a few more days for additional review and merge

@sungwy
sungwy merged commit 23c7f23 into apache:main May 24, 2026
58 of 66 checks passed
@sungwy
sungwy deleted the int96-timestamp-offset-fix branch May 24, 2026 21:55
@manuzhang

Copy link
Copy Markdown
Member

@sungwy Please double check the commit message has the module prefix like the PR title when merging.

@kevinjqliu

Copy link
Copy Markdown
Contributor

LGTM! #16459 called out both VectorizedParquetDefinitionLevelReader and VectorizedDictionaryEncodedParquetValuesReader
both now has the same fix

class TimestampInt96DictEncodedReader extends BaseDictEncodedReader {
@Override
protected void nextVal(
FieldVector vector, Dictionary dict, int idx, int currentVal, int typeWidth) {
ByteBuffer buffer =
dict.decodeToBinary(currentVal).toByteBuffer().order(ByteOrder.LITTLE_ENDIAN);
long timestampInt96 = ParquetUtil.extractTimestampInt96(buffer);
vector.getDataBuffer().setLong((long) idx * typeWidth, timestampInt96);
}
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants