Repository navigation
dumpformat: fix corrupted DECIMAL values in dictionary-encoded parquet files - #70436
Conversation
setDatumFromDecimalByte consumes its input in place (radix conversion and two's-complement negation both write into the byte slice). Values decoded from a dictionary-encoded page alias the shared dictionary entry, so converting the first row rewrote the dictionary bytes and every later row referencing the same entry decoded from the already-consumed buffer, silently producing a wrong value (0.00 in the reported witness). Copy each value into a buffer owned by the setter before converting. The column iterator drives setters sequentially, so one reused buffer per column keeps the copy allocation-free. close pingcap#70365 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ti-chi-bot
Bot
added
do-not-merge/needs-tests-checked
release-note
📝 WalkthroughWalkthroughThe decimal converter now copies Parquet byte-array inputs before conversion. A parser integration test covers repeated dictionary-encoded decimals in variable-length and fixed-length representations. ChangesDecimal dictionary safety
Estimated code review effort: 3 (Moderate) | ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/dumpformat/parquetfile/parser_test.go`:
- Line 1156: Update the cleanup around the Parquet reader in the relevant test
to capture and assert the error returned by rdr.Close instead of discarding it.
Use the test’s existing assertion mechanism and ensure the cleanup runs after
the test body.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1a715999-f383-436b-a191-08b5e3292a75
📒 Files selected for processing (2)
pkg/dumpformat/parquetfile/parser_test.gopkg/dumpformat/parquetfile/type_converter.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #70436 +/- ##
================================================
- Coverage 76.3277% 73.4692% -2.8586%
================================================
Files 2041 2081 +40
Lines 558363 589824 +31461
================================================
+ Hits 426186 433339 +7153
- Misses 131277 155412 +24135
- Partials 900 1073 +173
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
🔍 Starting code review for this PR... |
ingress-bot
left a comment
ingress-bot
left a comment
There was a problem hiding this comment.
This review was generated by AI and should be verified by a human reviewer.
Manual follow-up is recommended before merge.
Summary
- Total findings: 4
- Inline comments: 3
- Summary-only findings (no inline anchor): 1
Findings (highest risk first)
🟡 [Minor] (3)
- repeatedByteArray and repeatedFixedLenByteArray duplicate the same generator body (pkg/dumpformat/parquetfile/parser_test.go:1099, pkg/dumpformat/parquetfile/parser_test.go:1110)
- Regression test misses the large-decimal string-fallback path that has the same shared-buffer bug (pkg/dumpformat/parquetfile/parser_test.go:1122, pkg/dumpformat/parquetfile/type_converter.go:72)
- Regression test misses the in-place-mutating fallback decimal path (pkg/dumpformat/parquetfile/parser_test.go:1165, pkg/dumpformat/parquetfile/type_converter.go:72)
🧹 [Nit] (1)
- newDecimalByteSetter breaks the file's get*Setter naming convention (pkg/dumpformat/parquetfile/type_converter.go:390)
Unanchored findings
🟡 [Minor] (1)
- repeatedByteArray and repeatedFixedLenByteArray duplicate the same generator body
- Request: Collapse the two closures into one generic helper parameterized on the slice element type, or on
anywith a type-switch, and use it for all threeGenfields in this subtest.
- Request: Collapse the two closures into one generic helper parameterized on the slice element type, or on
… test Values of maximumDecimalBytes or longer take getStringFromParquetByte, which consumes its input in place just like FromParquetArray, so add a dictionary encoded column that reaches it. Verified it decodes 0.00 from row 1 onwards when the copy is removed. Also rename newDecimalByteSetter to getDecimalByteSetter to match the other setter factories in the file, and assert the error from closing the reader. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
/cherry-pick release-nextgen-202603 |
|
@D3Hunter: once the present PR merges, I will cherry-pick it on top of release-nextgen-202603 in the new PR and assign it to you. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository. |
|
/retest |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: D3Hunter, OliverS929 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
ti-chi-bot
Bot
added
lgtm
and removed
needs-1-more-lgtm
[LGTM Timeline notifier]Timeline:
|
|
@D3Hunter: new pull request created to branch DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository. |
|
/cherry-pick release-8.5 |
|
@joechenrh: new pull request created to branch DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository. |
… files (pingcap#70436) The automated cherry-pick of pingcap#70436 dropped two files into pkg/dumpformat/parquetfile, a package that does not exist on this branch, and left the actual 8.5 code path untouched. This replaces it with the equivalent fix for release-8.5. Here the byte-array DECIMAL setters live in pkg/lightning/mydump and convert through binaryToDecimalStr, which rewrites its input in place when the value is negative. arrow-go's dictionary decoder assigns dict[idx] to every row referencing an entry, so all those rows share one backing array: converting the first occurrence negated the dictionary entry, and later rows decoded the already-negated bytes as a positive value. Positive values are unaffected on this branch because that path only calls big.Int.SetBytes, which does not touch the input. Copy each value into a buffer owned by the setter before converting. The column iterator drives setters sequentially, so one reused buffer per column keeps the copy allocation-free. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
5 participants
What problem does this PR solve?
Issue Number: close #70365
Problem Summary:
IMPORT INTOsilently writes wrong values for repeated dictionary-encoded Parquet DECIMAL columns. Only the first occurrence of a dictionary entry is correct; every later row referencing the same entry gets a different value (0.00in the reported witness). The job reports success andADMIN CHECK TABLEpasses, because both validate the KVs produced after the value was already corrupted.Root cause is buffer ownership:
types.MyDecimal.FromParquetArraydocuments its input as disposable and does the radix conversion by overwriting the byte slice in place (the two's-complement negation and the long division both write intobuf). The oversized-value fallbackgetStringFromParquetBytedoes the same.dictConverter.Fill/Copyfill decoded values by assigningdict[idx], i.e. only the slice header. Every row referencing the same dictionary entry shares one backing array.What changed and how does it work?
Copy each value into a buffer owned by the setter before converting. The column iterator drives setters sequentially (
columnIteratoris documented as not usable in parallel), so one reused buffer per column keeps the copy allocation-free — no per-row allocation is added.Both byte-array DECIMAL setters (
BYTE_ARRAYandFIXED_LEN_BYTE_ARRAY) go through the shared helper, so the fix covers both physical types and both the directMyDecimalpath and the string fallback path for oversized values.AI has also checked other code paths, this is the only path need to be fixed.
Check List
Tests
Side effects
Documentation
Release note
Summary by CodeRabbit
Bug Fixes
Tests