Repository navigation
dumpformat: track Parquet scan progress by source file offset (#70307) - #70345
Conversation
Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
ti-chi-bot
added
do-not-merge/hold
|
@D3Hunter This PR has conflicts, I have hold it. |
|
@ti-chi-bot: ## If you want to know how to resolve it, please read the guide in TiDB Dev Guide. DetailsInstructions 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. |
📝 WalkthroughWalkthroughParquet scan progress no longer sums decoded row sizes. It estimates source-file progress from consumed rows, reports the file size for empty or fully consumed files, and adds coverage for preload, streaming, partial-read, and large-file cases. ChangesParquet scan progress
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 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: 2
🤖 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.go`:
- Around line 529-538: The scan progress logic should handle row-count overshoot
without returning a position beyond the file. In the progress calculation near
totalRows and totalReadRows, change the completion check to totalReadRows >=
totalRows, and clamp the computed progress position to fileSize before returning
it. Add a test covering totalReadRows greater than totalRows.
- Around line 520-539: Resolve the cherry-pick conflicts and remove all conflict
markers. In pkg/lightning/mydump/parquet_parser.go lines 520-539, implement
ScannedPos on *ParquetParser and retain the source-progress logic using its
metadata and row counters, not the Parser interface. In
pkg/lightning/mydump/parquet_parser_test.go lines 248-258, pass the
package-local metadata required by newParquetParserForTest; at lines 290-292,
construct &ParquetParser with fileMeta instead of &Parser.
🪄 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: 0880a3af-a3e2-44f0-b69b-a229df399830
📒 Files selected for processing (4)
pkg/dumpformat/parquetfile/BUILD.bazelpkg/lightning/mydump/parquet_parser.gopkg/lightning/mydump/parquet_parser_test.gopkg/lightning/mydump/parser.go
| if pp.totalRows <= 0 { | ||
| return fileSize, nil | ||
| } | ||
|
|
||
| if pp.totalReadRows == pp.totalRows { | ||
| return fileSize, nil | ||
| } | ||
|
|
||
| progress := float64(pp.totalReadRows) / float64(pp.totalRows) | ||
| return int64(progress * float64(fileSize)), nil |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Clamp scan progress when row counters reach or exceed the total.
totalReadRows == totalRows only handles exact completion. If totalReadRows exceeds totalRows, the ratio can return a position larger than fileSize.
Return fileSize when totalReadRows >= totalRows. Clamp the calculated value to fileSize. Add an overshoot test case.
🤖 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.go` around lines 529 - 538, The scan
progress logic should handle row-count overshoot without returning a position
beyond the file. In the progress calculation near totalRows and totalReadRows,
change the completion check to totalReadRows >= totalRows, and clamp the
computed progress position to fileSize before returning it. Add a test covering
totalReadRows greater than totalRows.
Signed-off-by: D3Hunter <jujj603@gmail.com>
|
Cherry-pick conflicts appear resolved; removing the |
ti-chi-bot
removed
the
do-not-merge/hold
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/lightning/mydump/parquet_parser_test.go (1)
286-325: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd an over-read case for the clamp contract.
The
completecase teststotalReadRows == totalRows. It does not testtotalReadRows > totalRows. Add an over-read case to prove thatScannedPos()remains equal to the source file size.Suggested test case
{name: "complete", fileSize: 101, readRows: 10, totalRows: 10, expected: 101}, + {name: "over-read", fileSize: 101, readRows: 11, totalRows: 10, expected: 101},🤖 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 286 - 325, Extend the table-driven cases in TestParquetScannedPosByReadRows with an over-read scenario where totalReadRows exceeds totalRows, and assert that ScannedPos() returns the source file size. Keep the existing complete and other boundary cases 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.
Nitpick comments:
In `@pkg/lightning/mydump/parquet_parser_test.go`:
- Around line 286-325: Extend the table-driven cases in
TestParquetScannedPosByReadRows with an over-read scenario where totalReadRows
exceeds totalRows, and assert that ScannedPos() returns the source file size.
Keep the existing complete and other boundary cases unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 79d2fcea-7996-40fe-8ea7-e2e0ce9b30a2
📒 Files selected for processing (2)
pkg/lightning/mydump/parquet_parser.gopkg/lightning/mydump/parquet_parser_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/lightning/mydump/parquet_parser.go
ti-chi-bot
Bot
added
approved
needs-1-more-lgtm
|
[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
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## release-nextgen-202603 #70345 +/- ##
===========================================================
Coverage ? 76.2230%
===========================================================
Files ? 1937
Lines ? 541462
Branches ? 0
===========================================================
Hits ? 412719
Misses ? 128743
Partials ? 0
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
293ab37
into
pingcap:release-nextgen-202603
3 participants
This is an automated cherry-pick of #70307
What problem does this PR solve?
Issue Number: close #70306
Problem Summary:
The Parquet parser estimated scanned bytes from decoded datum sizes. Those sizes are independent of the original file's encoding, compression, metadata, and column layout, so byte progress and derived import metrics did not reflect the original file size. A 1,307-byte regression file, for example, reported only 400 bytes after every row had been read.
What changed and how does it work?
Check List
Tests
3 parquet files, total size 5,040,619 bytes
before the
tidb_import_bytes{state="restored"}metric is48,000,024 bytes, after this pr, it's5,040,619The regression assertion failed at the base commit with
expected: 1307, actual: 400in both row-group and whole-file preload modes, and passes with this change.Validation commands:
./tools/check/failpoint-go-test.sh pkg/dumpformat/parquetfile -run '^(TestParquetParser|TestParquetParserMultipleRowGroup)$' -count=1 make lintSide effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.
Summary by CodeRabbit
Bug Fixes
Documentation