Repository navigation
lightning: fix corrupted DECIMAL values in dictionary-encoded parquet files (#70436) - #70461
Conversation
ti-chi-bot
added
release-note
ti-chi-bot
Bot
added
size/XXL
|
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 (3)
📝 WalkthroughWalkthroughThe change prevents decimal conversion from mutating Parquet-backed byte arrays. It adds fixed-length byte-array writing support and regression coverage for repeated dictionary-encoded decimal values. ChangesParquet decimal safety
Estimated code review effort: 2 (Simple) | ~15 minutes Possibly related PRs
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: 4
🧹 Nitpick comments (1)
pkg/dumpformat/parquetfile/type_converter.go (1)
53-57: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument that this function can mutate
valin place.
setDatumFromDecimalBytepassesvaltogetStringFromParquetByteon the fallback path. That function inverts and decrements the bytes in place, and then destroys them during base-256 division. The only note about this contract is ongetDecimalByteSetterat Line 387. A future caller that reads this signature alone can pass a shared or dictionary-backed slice and reintroduce the exact corruption this PR fixes.Add the constraint to the function comment.
♻️ Proposed documentation
+// setDatumFromDecimalByte converts a parquet two's complement decimal value +// into d. +// +// The caller must own val: on the string fallback path the conversion +// consumes val in place, so a shared or dictionary-backed slice is corrupted. func setDatumFromDecimalByte(d *types.Datum, val []byte, scale int) error {🤖 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/dumpformat/parquetfile/type_converter.go` around lines 53 - 57, Add a function comment for setDatumFromDecimalByte explicitly stating that it may mutate the supplied val byte slice in place, so callers must not pass shared or dictionary-backed data. Keep the implementation unchanged.
🤖 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`:
- Around line 151-153: Update every testutils.WriteParquetFile call in
pkg/dumpformat/parquetfile/parser_test.go at lines 151-153, 421-423, 1072-1074,
1204-1206, and 1383-1389 to pass its returned error to require.NoError using the
test instance. Ensure all five fixture writes fail immediately with actionable
context instead of discarding errors; the surrounding parser tests require no
other changes.
- Around line 1860-1864: Correct the argument order in the ErrorIs assertion for
dec.FromParquetArray: pass its returned error as the actual error and
types.ErrOverflow as the target sentinel, preserving the overflow test’s intent
and allowing wrapped errors.
- Around line 1289-1294: Replace assert.NoError with require.NoError for each
ReadRow call in the affected test loops, including the corresponding patterns
near the other referenced locations, so execution stops before parser.LastRow()
is accessed when reading fails.
- Line 116: Add the missing parquet fixtures aurora_snapshot.parquet and
hive_dump.parquet under the testfiles directory referenced by TestParquetParser,
then run make bazel_prepare to generate and commit the BUILD.bazel metadata for
pkg/dumpformat/parquetfile.
---
Nitpick comments:
In `@pkg/dumpformat/parquetfile/type_converter.go`:
- Around line 53-57: Add a function comment for setDatumFromDecimalByte
explicitly stating that it may mutate the supplied val byte slice in place, so
callers must not pass shared or dictionary-backed data. Keep the implementation
unchanged.
🪄 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: 1459e43f-c9d8-4c32-96fb-63621f9ab199
📒 Files selected for processing (2)
pkg/dumpformat/parquetfile/parser_test.gopkg/dumpformat/parquetfile/type_converter.go
| return parquet.Int96(b) | ||
| } | ||
|
|
||
| func TestParquetParser(t *testing.T) { |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify the package BUILD.bazel lists the new sources and test fixtures.
set -euo pipefail
fd -t f 'BUILD.bazel' pkg/dumpformat/parquetfile --exec cat {}
# Confirm the checked-in fixtures referenced by the tests exist.
fd -t f -e parquet . pkg/dumpformat/parquetfileRepository: pingcap/tidb
Length of output: 150
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- package files ---'
git ls-files pkg/dumpformat/parquetfile
printf '%s\n' '--- BUILD files under pkg/dumpformat ---'
fd -t f 'BUILD.bazel' pkg/dumpformat -x sh -c 'echo "--- $1"; cat -n "$1"' sh {}
printf '%s\n' '--- parquet fixtures under repository ---'
fd -t f -e parquet . | sed -n '1,120p'
printf '%s\n' '--- relevant repository changes ---'
git diff --stat
git status --shortRepository: pingcap/tidb
Length of output: 1867
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- dumpformat tree ---'
find pkg/dumpformat -maxdepth 3 -type f -print | sort
printf '%s\n' '--- references to pkg/dumpformat/parquetfile ---'
rg -n --hidden --glob '!vendor/**' --glob '!node_modules/**' \
'pkg/dumpformat/parquetfile|dumpformat/parquetfile|type_converter.go|parser_test.go' \
. | sed -n '1,240p'
printf '%s\n' '--- nearby Bazel metadata ---'
fd -t f -g 'BUILD.bazel' -g 'BUILD' pkg | rg 'dumpformat|parquet|lightning' | sed -n '1,160p'
printf '%s\n' '--- applicable instructions ---'
fd -t f 'AGENTS.md' -x sh -c 'echo "--- $1"; cat "$1"' sh {}Repository: pingcap/tidb
Length of output: 1889
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- Bazel preparation rules ---'
rg -n -C 4 'bazel_prepare|BUILD\.bazel' Makefile build .github 2>/dev/null | sed -n '1,240p'
printf '%s\n' '--- all Bazel files that mention dumpformat or broad Go globs ---'
fd -t f 'BUILD.bazel' . -x sh -c '
if rg -q "dumpformat|glob\\(.*\\.go|go_library|go_test" "$1"; then
echo "--- $1"
rg -n -C 2 "dumpformat|glob\\(.*\\.go|go_library|go_test" "$1" | sed -n "1,180p"
fi
' sh {}
printf '%s\n' '--- parser test file structure and fixture references ---'
ast-grep outline pkg/dumpformat/parquetfile/parser_test.go
rg -n -C 3 'testfiles|testdata|\.parquet|Test[A-Za-z0-9_]+\s*\(|Benchmark[A-Za-z0-9_]+' \
pkg/dumpformat/parquetfile/parser_test.go | sed -n '1,280p'
printf '%s\n' '--- Go file headers and imports ---'
sed -n '1,70p' pkg/dumpformat/parquetfile/type_converter.go
sed -n '1,70p' pkg/dumpformat/parquetfile/parser_test.goRepository: pingcap/tidb
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- bazel_prepare implementation ---'
sed -n '581,600p' Makefile
printf '%s\n' '--- top-level tests and benchmark declarations ---'
python3 - <<'PY'
import re
from pathlib import Path
for path in sorted(Path("pkg/dumpformat/parquetfile").glob("*.go")):
text = path.read_text()
print(f"--- {path}")
for match in re.finditer(r"^func\s+(Test\w+|Benchmark\w+)\s*\(", text, re.M):
print(f"{match.group(1)}:{text.count(chr(10), 0, match.start()) + 1}")
PY
printf '%s\n' '--- fixture and generated-file references in parser_test.go ---'
rg -n -C 2 'testfiles|testdata|\.parquet|os\.|embed|ReadFile|OpenFile' \
pkg/dumpformat/parquetfile/parser_test.go | sed -n '1,240p'
printf '%s\n' '--- package metadata existence ---'
for path in pkg/dumpformat/BUILD.bazel pkg/dumpformat/parquetfile/BUILD.bazel \
pkg/dumpformat/parquetfile/testfiles; do
if [ -e "$path" ]; then
echo "exists: $path"
else
echo "missing: $path"
fi
doneRepository: pingcap/tidb
Length of output: 7682
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- fixture call sites and helper implementation ---'
sed -n '70,125p' pkg/dumpformat/parquetfile/parser_test.go
sed -n '1248,1335p' pkg/dumpformat/parquetfile/parser_test.go
printf '%s\n' '--- referenced fixture names in the repository ---'
rg -n --hidden --glob '!vendor/**' \
'aurora_snapshot\.parquet|hive_dump\.parquet|newParquetParserForTest\(.*testfiles' . \
| sed -n '1,160p'
printf '%s\n' '--- exact filesystem checks ---'
python3 - <<'PY'
from pathlib import Path
root = Path("pkg/dumpformat/parquetfile")
for path in (
root / "testfiles",
root / "testfiles" / "aurora_snapshot.parquet",
root / "testfiles" / "hive_dump.parquet",
):
print(f"{path}: {'exists' if path.exists() else 'missing'}")
PYRepository: pingcap/tidb
Length of output: 5112
Regenerate Bazel metadata and add the missing test fixtures.
pkg/dumpformat/parquetfile has no BUILD.bazel. Run make bazel_prepare and commit the generated metadata. Add testfiles/aurora_snapshot.parquet and testfiles/hive_dump.parquet; both tests open these missing files.
🤖 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/dumpformat/parquetfile/parser_test.go` at line 116, Add the missing
parquet fixtures aurora_snapshot.parquet and hive_dump.parquet under the
testfiles directory referenced by TestParquetParser, then run make bazel_prepare
to generate and commit the BUILD.bazel metadata for pkg/dumpformat/parquetfile.
Source: Coding guidelines
| testutils.WriteParquetFile(dir, name, pc, 100) | ||
|
|
||
| reader := newParquetParserForTest(context.Background(), t, dir, name, 0, FileMeta{}) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Five call sites discard the error from testutils.WriteParquetFile. The shared root cause is inconsistent error checking on the same helper. Other call sites in this file check it, for example Line 234, Line 509, and Line 1162. When a discarded write fails, the test fails later inside the parser with an unrelated open or parse error, which hides the real cause.
Wrap each call in require.NoError.
pkg/dumpformat/parquetfile/parser_test.go#L151-L153: wrap theTestParquetParserfixture write inrequire.NoError.pkg/dumpformat/parquetfile/parser_test.go#L421-L423: wrap thetimestamp_and_decimalfixture write inrequire.NoError.pkg/dumpformat/parquetfile/parser_test.go#L1072-L1074: wrap thedecimal_with_nullsfixture write inrequire.NoError.pkg/dumpformat/parquetfile/parser_test.go#L1204-L1206: wrap thebooleanfixture write inrequire.NoError.pkg/dumpformat/parquetfile/parser_test.go#L1383-L1389: wrap theTestBasicReadFilemulti-page fixture write inrequire.NoError.
The coding guidelines state: "Keep error handling actionable and contextual; do not silently swallow errors."
📍 Affects 1 file
pkg/dumpformat/parquetfile/parser_test.go#L151-L153(this comment)pkg/dumpformat/parquetfile/parser_test.go#L421-L423pkg/dumpformat/parquetfile/parser_test.go#L1072-L1074pkg/dumpformat/parquetfile/parser_test.go#L1204-L1206pkg/dumpformat/parquetfile/parser_test.go#L1383-L1389
🤖 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/dumpformat/parquetfile/parser_test.go` around lines 151 - 153, Update
every testutils.WriteParquetFile call in
pkg/dumpformat/parquetfile/parser_test.go at lines 151-153, 421-423, 1072-1074,
1204-1206, and 1383-1389 to pass its returned error to require.NoError using the
test instance. Ensure all five fixture writes fail immediately with actionable
context instead of discarding errors; the surrounding parser tests require no
other changes.
Source: Coding guidelines
| for i := range expectedRes { | ||
| err := parser.ReadRow() | ||
| assert.NoError(t, err) | ||
| expectedValues := expectedRes[i] | ||
| row := parser.LastRow().Row | ||
| assert.Len(t, expectedValues, len(row)) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use require.NoError before you read LastRow.
assert.NoError records the failure and continues. If ReadRow fails, the loop still calls parser.LastRow() and indexes row, so the test reports a confusing secondary failure, or panics on a short row. The same pattern appears at Line 1079 and Line 1212.
🐛 Proposed fix
for i := range expectedRes {
err := parser.ReadRow()
- assert.NoError(t, err)
+ require.NoError(t, err)
expectedValues := expectedRes[i]
row := parser.LastRow().Row
- assert.Len(t, expectedValues, len(row))
+ require.Len(t, expectedValues, len(row))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for i := range expectedRes { | |
| err := parser.ReadRow() | |
| assert.NoError(t, err) | |
| expectedValues := expectedRes[i] | |
| row := parser.LastRow().Row | |
| assert.Len(t, expectedValues, len(row)) | |
| for i := range expectedRes { | |
| err := parser.ReadRow() | |
| require.NoError(t, err) | |
| expectedValues := expectedRes[i] | |
| row := parser.LastRow().Row | |
| require.Len(t, expectedValues, len(row)) |
🤖 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/dumpformat/parquetfile/parser_test.go` around lines 1289 - 1294, Replace
assert.NoError with require.NoError for each ReadRow call in the affected test
loops, including the corresponding patterns near the other referenced locations,
so execution stops before parser.LastRow() is accessed when reading fails.
| // Test overflow | ||
| var dec types.MyDecimal | ||
| raw := bytes.Repeat([]byte{0x7f}, 40) | ||
| require.ErrorIs(t, types.ErrOverflow, dec.FromParquetArray(raw, 0)) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
The require.ErrorIs arguments are reversed.
The signature is require.ErrorIs(t, err, target). This call passes types.ErrOverflow as err and the value returned by FromParquetArray as target. The assertion therefore evaluates errors.Is(types.ErrOverflow, returnedErr), which is the opposite of the intent.
The call passes only when the returned error is identical to types.ErrOverflow. If FromParquetArray wraps or annotates the error, the assertion fails even though the behavior is correct. It also does not verify the returned error against the sentinel in the general case.
🐛 Proposed fix
// Test overflow
var dec types.MyDecimal
raw := bytes.Repeat([]byte{0x7f}, 40)
- require.ErrorIs(t, types.ErrOverflow, dec.FromParquetArray(raw, 0))
+ require.ErrorIs(t, dec.FromParquetArray(raw, 0), types.ErrOverflow)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Test overflow | |
| var dec types.MyDecimal | |
| raw := bytes.Repeat([]byte{0x7f}, 40) | |
| require.ErrorIs(t, types.ErrOverflow, dec.FromParquetArray(raw, 0)) | |
| } | |
| // Test overflow | |
| var dec types.MyDecimal | |
| raw := bytes.Repeat([]byte{0x7f}, 40) | |
| require.ErrorIs(t, dec.FromParquetArray(raw, 0), types.ErrOverflow) |
🤖 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/dumpformat/parquetfile/parser_test.go` around lines 1860 - 1864, Correct
the argument order in the ErrorIs assertion for dec.FromParquetArray: pass its
returned error as the actual error and types.ErrOverflow as the target sentinel,
preserving the overflow test’s intent and allowing wrapped errors.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## release-8.5 #70461 +/- ##
================================================
Coverage ? 55.1391%
================================================
Files ? 1852
Lines ? 666989
Branches ? 0
================================================
Hits ? 367772
Misses ? 271833
Partials ? 27384
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
… 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>
369e9c1 to
f3290ed
Compare
ti-chi-bot
Bot
added
size/L
joechenrh
changed the title
|
/retest |
|
[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
approved
lgtm
and removed
needs-1-more-lgtm
ti-chi-bot
Bot
added
cherry-pick-approved
3 participants
This started as an automated cherry-pick of #70436, but it is not a clean cherry-pick: on master the code lives in
pkg/dumpformat/parquetfileand converts throughMyDecimal.FromParquetArray, neither of which exists onrelease-8.5. The bot's version added two orphan files under a package that does not exist on this branch and left the actual 8.5 code path untouched, so it fixed nothing. This PR replaces it with the equivalent fix for the code that is actually onrelease-8.5.What problem does this PR solve?
Issue Number: close #70365
Problem Summary:
IMPORT INTOsilently writes wrong values for repeated dictionary-encoded Parquet DECIMAL columns stored asBYTE_ARRAY/FIXED_LEN_BYTE_ARRAY. Only the first occurrence of a dictionary entry is correct; every later row referencing the same entry decodes from bytes that were already consumed. The job reports success andADMIN CHECK TABLEpasses, because both validate the KVs produced after the value was corrupted.Root cause is buffer ownership:
binaryToDecimalStrderives the absolute value from the two's-complement representation by negating the byte slice in place (rawBytes[i] = ^rawBytes[i], then the carry loop).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.On this branch the corruption is narrower than on master: the negation only runs for negative values, and the positive path just calls
big.Int.SetBytes, which does not touch its input. So the witness is a repeated negative decimal, and it comes out with the wrong sign —-123.45becomes123.45from the second row on.Master is worse because #65624 replaced
big.IntwithMyDecimal.FromParquetArray, whose long division writes into the input for positive values too; that is why #70436 describes a different root cause and a different call path.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, 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.The regression test appends a dictionary-encoded case to
TestParquetVariousTypes: 256 rows repeating one value per column, assertingHasDictionaryPage()so the case cannot go vacuous if the writer ever falls back to PLAIN. Each of the two negative columns fails independently without the fix.WriteParquetFilegained aFixedLenByteArrayColumnChunkWritercase, which it needs to emit the fixed-length column.Check List
Tests
Side effects
Documentation
Release note
Summary by CodeRabbit