Repository navigation
dumpformat: fix corrupted DECIMAL values in dictionary-encoded parquet files (#70436) - #70459
Merged
ti-chi-bot[bot] merged 3 commits intoAug 12, 2026
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>
… 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>
ti-chi-bot
added
release-note
1 of 13 tasks
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe change prevents in-place decimal conversion from mutating dictionary-backed Parquet byte arrays. It adds coverage for repeated dictionary-encoded variable-length and fixed-length decimal values across 256 rows. ChangesParquet decimal decoding
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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/lightning/mydump/parquet_parser_test.go`:
- Around line 1094-1188: Add a focused non-dictionary DECIMAL test alongside
dictionary_encoded_decimal, covering both ByteArray and FixedLenByteArray
columns. Configure the Parquet writer/test column definitions to disable
dictionary encoding for these DECIMAL values, then assert the parser returns the
expected decimal strings for each row and type.
🪄 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: b8542d6e-4588-442c-a483-8307320aa041
📒 Files selected for processing (2)
pkg/lightning/mydump/parquet_parser_test.gopkg/lightning/mydump/parquet_type_converter.go
Comment on lines
+1094
to
+1188
| t.Run("dictionary_encoded_decimal", func(t *testing.T) { | ||
| // Enough repeated rows that the writer keeps the dictionary page instead of | ||
| // falling back to PLAIN, so every row decodes to the same shared buffer. | ||
| const rows = 256 | ||
| repeatedByteArray := func(val []byte) func(int) (any, []int16) { | ||
| return func(numRows int) (any, []int16) { | ||
| values := make([]parquet.ByteArray, numRows) | ||
| levels := make([]int16, numRows) | ||
| for i := range numRows { | ||
| values[i] = val | ||
| levels[i] = 1 | ||
| } | ||
| return values, levels | ||
| } | ||
| } | ||
| repeatedFixedLenByteArray := func(val []byte) func(int) (any, []int16) { | ||
| return func(numRows int) (any, []int16) { | ||
| values := make([]parquet.FixedLenByteArray, numRows) | ||
| levels := make([]int16, numRows) | ||
| for i := range numRows { | ||
| values[i] = val | ||
| levels[i] = 1 | ||
| } | ||
| return values, levels | ||
| } | ||
| } | ||
|
|
||
| pc := []testutils.ParquetColumn{ | ||
| { | ||
| Name: "positive", | ||
| Type: parquet.Types.ByteArray, | ||
| Converted: schema.ConvertedTypes.Decimal, | ||
| Precision: 9, | ||
| Scale: 2, | ||
| Gen: repeatedByteArray([]byte{0x30, 0x39}), // 12345 | ||
| }, | ||
| { | ||
| Name: "negative", | ||
| Type: parquet.Types.ByteArray, | ||
| Converted: schema.ConvertedTypes.Decimal, | ||
| Precision: 9, | ||
| Scale: 2, | ||
| Gen: repeatedByteArray([]byte{0xcf, 0xc7}), // -12345 | ||
| }, | ||
| { | ||
| Name: "fixed_len", | ||
| Type: parquet.Types.FixedLenByteArray, | ||
| Converted: schema.ConvertedTypes.Decimal, | ||
| TypeLen: 4, | ||
| Precision: 9, | ||
| Scale: 2, | ||
| Gen: repeatedFixedLenByteArray([]byte{0x00, 0x00, 0x30, 0x39}), // 12345 | ||
| }, | ||
| { | ||
| // maximumDecimalBytes or longer, so this one takes the string | ||
| // fallback path, which consumes its input in place as well. | ||
| Name: "oversized", | ||
| Type: parquet.Types.ByteArray, | ||
| Converted: schema.ConvertedTypes.Decimal, | ||
| Precision: 75, | ||
| Scale: 2, | ||
| Gen: repeatedByteArray(append([]byte{0x00, 0x01}, make([]byte, 31)...)), // 2^248 | ||
| }, | ||
| } | ||
|
|
||
| dir := t.TempDir() | ||
| fileName := "test.decimal.dictionary.parquet" | ||
| require.NoError(t, testutils.WriteParquetFile(dir, fileName, pc, rows)) | ||
|
|
||
| rdr, err := file.OpenParquetFile(filepath.Join(dir, fileName), false) | ||
| require.NoError(t, err) | ||
| t.Cleanup(func() { | ||
| require.NoError(t, rdr.Close()) | ||
| }) | ||
| for i := range pc { | ||
| cc, err := rdr.MetaData().RowGroup(0).ColumnChunk(i) | ||
| require.NoError(t, err) | ||
| require.True(t, cc.HasDictionaryPage(), "column %s is not dictionary encoded", pc[i].Name) | ||
| } | ||
|
|
||
| reader := newParquetParserForTest(context.Background(), t, dir, fileName, 0, FileMeta{}) | ||
|
|
||
| expectedValues := []string{ | ||
| "123.45", "-123.45", "123.45", | ||
| "4523128485832663883733241601901871400518358776001584532791311875309106626.56", | ||
| } | ||
| for range rows { | ||
| require.NoError(t, reader.ReadRow()) | ||
| require.Len(t, reader.lastRow.Row, len(expectedValues)) | ||
| for i, expectValue := range expectedValues { | ||
| s, err := reader.lastRow.Row[i].ToString() | ||
| require.NoError(t, err) | ||
| require.Equal(t, expectValue, s) | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find byte-array DECIMAL test cases and their writer options.
rg -n -U -C 5 \
'Type:\s*parquet\.Types\.(ByteArray|FixedLenByteArray),\s*\n\s*Converted:\s*schema\.ConvertedTypes\.Decimal' \
pkg/lightning/mydump
# Find dictionary-related writer configuration and assertions.
rg -n -C 4 'Dictionary|dictionary|With.*Encoding|With.*Writer' \
pkg/lightning/mydump/parquet_parser_test.go pkg/lightning/mydumpRepository: pingcap/tidb
Length of output: 13092
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- Decimal-related tests and helpers ---'
rg -n -C 8 'Decimal|decimal|WithDictionaryFor|WriteParquetFile' \
pkg/lightning/mydump/parquet_parser_test.go \
pkg/lightning/mydump/parquet_writer.go \
pkg/lightning/mydump/parquet_type_converter.go
printf '%s\n' '--- Existing non-dictionary test context ---'
sed -n '1580,1725p' pkg/lightning/mydump/parquet_parser_test.go
printf '%s\n' '--- Relevant parser test structure ---'
sed -n '1000,1215p' pkg/lightning/mydump/parquet_parser_test.goRepository: pingcap/tidb
Length of output: 50368
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
path = Path("pkg/lightning/mydump/parquet_parser_test.go")
text = path.read_text()
for match in re.finditer(r'(?s)\{.*?\}', text):
block = match.group(0)
if "Converted: schema.ConvertedTypes.Decimal" in block and (
"parquet.Types.ByteArray" in block
or "parquet.Types.FixedLenByteArray" in block
):
start = text.count("\n", 0, match.start()) + 1
end = text.count("\n", 0, match.end()) + 1
print(f"lines {start}-{end}:")
print(" ".join(line.strip() for line in block.splitlines()))
PY
printf '%s\n' '--- All dictionary-disable uses ---'
rg -n -C 3 'WithDictionaryFor\([^)]*, false\)' pkg/lightning/mydumpRepository: pingcap/tidb
Length of output: 1736
Add non-dictionary DECIMAL coverage.
The repository has no parser test for non-dictionary ByteArray or FixedLenByteArray DECIMAL values. Add a focused case that disables dictionary encoding for both DECIMAL types.
🤖 Prompt for 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.
In `@pkg/lightning/mydump/parquet_parser_test.go` around lines 1094 - 1188, Add a
focused non-dictionary DECIMAL test alongside dictionary_encoded_decimal,
covering both ByteArray and FixedLenByteArray columns. Configure the Parquet
writer/test column definitions to disable dictionary encoding for these DECIMAL
values, then assert the parser returns the expected decimal strings for each row
and type.
1 of 13 tasks
The cherry-pick landed the production change correctly, but the test came from master's pkg/dumpformat/parquetfile, where the parquet test helpers live in a testutils package and the parser meta type is called FileMeta. On this branch both helpers are package-local and the meta type is ParquetFileMeta, so the test did not compile. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## release-nextgen-202603 #70459 +/- ##
===========================================================
Coverage ? 76.2303%
===========================================================
Files ? 1937
Lines ? 541476
Branches ? 0
===========================================================
Hits ? 412769
Misses ? 128707
Partials ? 0
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
D3Hunter
approved these changes
Aug 12, 2026
ti-chi-bot
Bot
added
approved
needs-1-more-lgtm
joechenrh
approved these changes
Aug 12, 2026
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: D3Hunter, joechenrh 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
ti-chi-bot
Bot
merged commit Aug 12, 2026
85aefb4
into
pingcap:release-nextgen-202603
18 checks passed
3 of 13 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is an automated cherry-pick of #70436
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