Repository navigation
dumpformat: track Parquet scan progress by source file offset - #70307
Conversation
ti-chi-bot
Bot
added
release-note
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe Parquet parser now estimates ChangesParquet scan progress
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Tools execution failed with the following error: Failed to run tools: 14 UNAVAILABLE: read ECONNRESET 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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #70307 +/- ##
================================================
- Coverage 76.3227% 73.8300% -2.4928%
================================================
Files 2041 2088 +47
Lines 559123 588915 +29792
================================================
+ Hits 426738 434796 +8058
- Misses 131485 152873 +21388
- Partials 900 1246 +346
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: 3
- Inline comments: 1
- Summary-only findings (no inline anchor): 2
Findings (highest risk first)
🟡 [Minor] (3)
- ScannedPos comment contradicts the shared Parser interface contract (pkg/dumpformat/parquetfile/parser.go:555, pkg/lightning/mydump/parser.go:144)
- scanProgress doc comment describes only the column-reader path, but preload paths update it too (pkg/dumpformat/parquetfile/parser.go:314, pkg/dumpformat/parquetfile/reader_wrapper.go:175)
- ScannedPos scale redefinition breaks parquet restore-progress metrics across an upgrade/downgrade resume boundary (pkg/dumpformat/parquetfile/parser.go:555, lightning/pkg/importer/chunk_process.go:724, lightning/pkg/importer/chunk_process.go:617, lightning/pkg/importer/chunk_process.go:124)
Unanchored findings
🟡 [Minor] (2)
- ScannedPos comment contradicts the shared Parser interface contract
- Request: Update the shared
mydump.Parser.ScannedPosinterface doc to note that per-format implementations (like parquet, with disjoint/parallel column readers) may return an approximation rather than a literal reader pointer, mirroring howPosalready documents per-format differences just above it.
- Request: Update the shared
- scanProgress doc comment describes only the column-reader path, but preload paths update it too
- Request: Reword the comment to describe
scanProgressas tracking the furthest byte offset reached by any reader that loads row-group data — streaming per-column readers or bulk preload — not just "column readers".
- Request: Reword the comment to describe
| // track the footer. | ||
| return fileSize, nil | ||
| } | ||
| return min(max(pp.progress.current(), int64(0)), fileSize), nil |
There was a problem hiding this comment.
🟡 [Minor] ScannedPos scale redefinition breaks parquet restore-progress metrics across an upgrade/downgrade resume boundary
Impact
The persisted parquet checkpoint field RealOffset is taken from ScannedPos, which this PR redefines from an estimated decoded-datum byte count to an on-disk file offset. A parquet chunk checkpointed by one binary and resumed by another (rolling upgrade or downgrade) mixes the two incompatible scales for the same chunk, so the StateRestored progress math diverges from a same-version run.
Data correctness is unaffected because resume seeking uses the row-based Chunk.Offset via SetPos, not RealOffset.
Scope
pkg/dumpformat/parquetfile/parser.go:555—Parser.ScannedPoslightning/pkg/importer/chunk_process.go:724—deliverLooplightning/pkg/importer/chunk_process.go:617lightning/pkg/importer/chunk_process.go:124
Evidence
New ScannedPos returns min(max(pp.progress.current(),0), fileSize) (a file offset), while the old path returned totalReadBytes (a decoded-datum estimate, typically larger than the on-disk size). That value is persisted as cr.chunk.Chunk.RealOffset and reused as startRealOffset on resume; the parquet branch only adds to BytesCounter when currRealOffset > startRealOffset, so an old large datum-estimate startRealOffset makes the new small file-offset base fail the guard and the restored-bytes metric stalls for that chunk. Resume positioning is unaffected: SetPos(chunk.Chunk.Offset, ...) at line 124 seeks by row count.
Change request
Since RealOffset is progress-metrics-only for parquet and resume is row-based, either rebase the parquet restore-progress from the post-SetPos ScannedPos on resume (rather than trusting the persisted RealOffset scale), or gate/version the field and note in release docs that parquet restore-progress metrics may be inaccurate for chunks resumed across this upgrade/downgrade boundary. Add a regression test that resumes a parquet chunk whose persisted RealOffset uses the old datum-estimate scale and asserts progress does not stall.
| // Parquet may read disjoint column ranges, so this returns the furthest data | ||
| // offset reached by any reader rather than a single underlying reader position. |
There was a problem hiding this comment.
The "furthest data offset" must be produced by the last column in the current row group, so the watermark can only be advanced by the last column reader. How about using this as an rough estimation
ScannedPos = fileSize × totalReadRows / totalRows
ti-chi-bot
Bot
added
approved
needs-1-more-lgtm
|
/hold |
ti-chi-bot
Bot
added
the
do-not-merge/hold
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: GMHDBJD, 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
|
/unhold |
ti-chi-bot
Bot
removed
the
do-not-merge/hold
|
/retest |
|
/unhold |
|
/retest |
|
/cherry-pick release-nextgen-202603 |
|
@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. |
5 participants
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
Reliability
Documentation
Tests