importer, mydump: preload small parquet files in a single read by joechenrh · Pull Request #68250 · pingcap/tidb

joechenrh · 2026-05-09T09:28:37Z

What problem does this PR solve?

Issue Number: ref #66180

When IMPORT INTO ingests many small parquet files, each file's footer and row-group data are fetched in separate opens — two for footer parsing in NewParquetParser, then one per row group in getBuilder → newInMemoryReaderBase. With 1.5k × ~480 KiB parts, the redundant S3 round-trips dominate wall time.

What changed and how does it work?

Add a whole-file in-memory strategy alongside the existing per-row-group preload and per-column streaming, and unify their gating under one threshold inMemoryThreshold (128 MiB). NewParquetParser reads the file once via prepareReader when it fits, uses the buffer for footer parsing, and lets getBuilder serve every column from the same buffer. Larger files still fall through to per-row-group preload or streaming.

switch {
case pp.preloadBase != nil:
    return inMemoryColumnBuilder(pp.preloadBase, ranges, fileSize), nil
case ranges.end-ranges.start <= int64(inMemoryThreshold):
    base, _ := newInMemoryReaderBase(ctx, pp.store, pp.path, ranges)
    return inMemoryColumnBuilder(base, ranges, fileSize), nil
default:
    return streamingColumnBuilder(ctx, pp.store, pp.path, ranges), nil
}

Benchmark

Staging cluster, 1550 × ~480 KiB parquet files on AWS S3, prod-aligned schema. Baseline and PR back-to-back, 1 node × 1 thread.

Metric	Baseline	PR	Delta
`takeTime` p50	~232 ms	~231 ms	flat
inter-chunk gap p50	~1.2 s	~270 ms	−77%
wall throughput	0.70 chunks/s	1.17 chunks/s	+67%
encode step time	46:34	20:09	+131%

Gain lands in the inter-chunk gap (parser-open path); takeTime and deliver spikes are unaffected, as expected.

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No need to test
- I checked and no code files have been changed.

Side effects

Performance regression: Consumes more CPU
Performance regression: Consumes more Memory
Breaking backward compatibility

Documentation

Release note

None.

Summary by CodeRabbit

Performance Improvements
- Parquet imports for small files can now preload whole files into memory, reducing I/O and speeding up imports.
- Parquet reader memory estimation is more accurate by considering actual file size, improving concurrency and resource decisions.
Tests
- Added tests verifying whole-file in-memory mode activates only when file size is known and under threshold.

Adds a third parquet reader strategy alongside per-column streaming and per-row-group in-memory preload. When the source file is small enough to fit in memory (default threshold 8 MiB), the parser issues one read for the whole file and reuses that buffer for both footer parsing and every column reader, eliminating the second range GET that the per-row-group path would otherwise issue and absorbing arrow-go's footer seek pattern locally. This is meaningful for jobs that import many small parquet parts where per-file open overhead dominates. The three strategies are dispatched in getBuilder, and the footer reader is selected in newFooterReader so each path is contained. Issue Number: ref pingcap#66180 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

ti-chi-bot · 2026-05-09T09:28:40Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

coderabbitai · 2026-05-09T09:28:45Z

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

@coderabbitai resume to resume automatic reviews.
@coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

▶️ Resume reviews
🔍 Trigger review

📝 Walkthrough

Walkthrough

Threads an explicit fileSize through Parquet reader construction, selects whole-file in-memory vs streaming readers via prepareReader, records shared preload buffers in ParquetParser for accurate memory estimation, propagates the fileSize to importer callers, and adds tests for whole-file preload behavior.

Changes

Small-file preloading for Parquet parsing

Layer / File(s)	Summary
Wrapper infrastructure and small-file preload utilities `pkg/lightning/mydump/parquet_wrapper.go`	Introduces `smallFileThreshold`, `newInMemoryReaderBaseFromBytes`, `columnReaderBuilder` implementations, and `prepareReader` to choose whole-file in-memory vs streaming readers based on `fileSize`.
Parser structure and builder selection `pkg/lightning/mydump/parquet_parser.go`	Adds `preloadBase` to `ParquetParser` and refactors `getBuilder` to pick in-memory shared base, per-row-group preload, or streaming column builders.
Parser initialization with fileSize and footer reader `pkg/lightning/mydump/parquet_parser.go`	Changes `NewParquetParser` signature to accept `fileSize int64`, calls `prepareReader(r, fileSize)`, defers closing `r`, and stores returned `preloadBase`; updates `SampleStatisticsFromParquet` to pass `fileSize=0`.
Peak-memory estimator aligned with runtime strategy `pkg/lightning/mydump/parquet_parser.go`	Adds `preloadBufferBytes()` and updates `EstimateParquetReaderMemory` to accept `fileSize int64`, construct the parser with a tracking allocator, and use parser-reported preload bytes for peak-memory estimation.
API wiring across importers and executors `pkg/executor/importer/table_import.go`, `pkg/executor/importer/import.go`, `lightning/pkg/importer/chunk_process.go`, `lightning/pkg/importer/get_pre_info.go`, `pkg/dxf/importinto/task_executor.go`	Propagates `fileSize` to parser/memory-estimation call sites and updates `TableImporter.EstimateParquetReaderMemory` signature to accept `fileSize int64`.
Test helper updates and preload behavior validation `pkg/lightning/mydump/parquet_parser_test.go`	Updates test helper `newParquetParserForTest` to accept `fileSize int64`, changes existing tests to pass `fileSize=0`, and adds `TestParquetParserWholeFileInMemory` validating preload selection.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

pingcap/tidb#66564: Related changes to parquet memory estimation and import concurrency budgeting.
pingcap/tidb#67908: Overlapping edits in NewParquetParser and Parquet parser behavior.
pingcap/tidb#68140: Also modifies pkg/lightning/mydump/parquet_parser.go constructor logic.

Suggested labels

component/import, component/lightning, size/XL, ok-to-test

Suggested reviewers

D3Hunter
GMHDBJD

Poem

🐰 I nibble bytes both big and small,

For tiny Parquets I cache them all.
Streaming bows when files are wide,
But whole-file hops when size's supplied.
A rabbit's sigh — fast reads tonight!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 42.86% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely describes the main change: adding a whole-file preload strategy for small parquet files to reduce redundant reads.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description check	✅ Passed	The PR description follows the template well, includes the issue reference, problem summary, change details, tests, and release note.

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

tiprow · 2026-05-09T09:28:55Z

Hi @joechenrh. Thanks for your PR.

PRs from untrusted users cannot be marked as trusted with /ok-to-test in this repo meaning untrusted PR authors can never trigger tests themselves. Collaborators can still trigger tests on the PR using /test all.

I understand the commands that are listed here.

Details

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 kubernetes-sigs/prow repository.

…hold Trim the verbose comments added in the previous commit and use the shorter name in the constant, parser field comment, and tests. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Match the smallFileThreshold naming, and remove the debug log in NewParquetParser whose information is already conveyed by the field name and trigger condition. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

pantheon-ai · 2026-05-11T10:29:41Z

@joechenrh I've received your pull request and will start the review. I'll conduct a thorough review covering code quality, potential issues, and implementation details.

⏳ This process typically takes 10-30 minutes depending on the complexity of the changes.

_{ℹ️ Learn more details on Pantheon AI.}

codecov · 2026-05-11T10:49:17Z

Codecov Report

❌ Patch coverage is 19.04762% with 68 lines in your changes missing coverage. Please review.
✅ Project coverage is 74.0123%. Comparing base (0b84b15) to head (88af75e).
⚠️ Report is 13 commits behind head on master.

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #68250        +/-   ##
================================================
- Coverage   76.3266%   74.0123%   -2.3144%     
================================================
  Files          2041       2070        +29     
  Lines        559579     589653     +30074     
================================================
+ Hits         427108     436416      +9308     
- Misses       131570     151724     +20154     
- Partials        901       1513       +612

Flag	Coverage Δ
integration	`42.7223% <19.0476%> (+3.0170%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
dumpling	`59.8807% <ø> (ø)`
parser	`∅ <ø> (∅)`
br	`44.9478% <ø> (-17.7736%)`	⬇️

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

ingress-bot · 2026-05-11T10:50:55Z

🔍 Starting code review for this PR...

ingress-bot

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: 4
Summary-only findings (no inline anchor): 0

Findings (highest risk first)

⚠️ [Major] (2)

Parquet reader memory estimator misses the new whole-file preload mode (pkg/lightning/mydump/parquet_wrapper.go:294, pkg/dxf/importinto/task_executor.go:190, pkg/lightning/mydump/parquet_parser.go:897)
Small-file parquet path turns reader close failures into hard parse failures (pkg/lightning/mydump/parquet_wrapper.go:300, pkg/lightning/mydump/parquet_parser.go:619)

🟡 [Minor] (2)

ParquetFileMeta.FileSize comment misstates fallback behavior as always streaming (pkg/lightning/mydump/loader.go:101, pkg/lightning/mydump/parquet_parser.go:427)
Parquet file size contract is duplicated across metadata layers and only partially wired (pkg/lightning/mydump/loader.go:102, pkg/executor/importer/import.go:1747, lightning/pkg/importer/chunk_process.go:111, lightning/pkg/importer/get_pre_info.go:496)

- Make r.Close() non-fatal in newFooterReader's small-file branch so storage close behavior matches the streaming path. - Plumb fileSize through EstimateParquetReaderMemory and account for the whole-file preload buffer via parser.preloadBufferBytes(), so concurrency sizing matches the runtime preload strategy. - Clarify ParquetFileMeta.FileSize comment to only describe the whole-file gate; row-group in-memory preload is unaffected. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

FileSize is a generic source-file property, not a parquet-specific knob, so it does not belong in ParquetFileMeta. Promote it to an explicit parameter of NewParquetParser (and newFooterReader), drop the field from ParquetFileMeta, and update all callers to pass SourceFileMeta.FileSize through directly. This also removes the manual mirror step in newLoadDataParser and lets the lightning openParser / ReadFirstNRowsByFileMeta / sampleDataFromTable paths benefit from the same whole-file preload optimization. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

newFooterReader returned the in-memory wrapper and its base separately, even though the wrapper already owned the base. Drop the extra return value, replace ParquetParser.smallFileBase with smallFileWrapper, and let NewParquetParser type-assert the wrapper to detect small-file mode. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

The parser only reads .base off the small-file wrapper, so storing the wrapper itself carries phantom pos/fileSize state. Extract base via type-assert in NewParquetParser and keep the field as the bare base pointer. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

prepareReader is what the function actually does: pick the read strategy for one parquet file. Restore the base return so callers do not need a type-assert dance to extract it from the wrapper. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Both branches eventually close r anyway, and r is only needed while NewParquetParser parses the footer. Move the close to a single defer in NewParquetParser so prepareReader stops shipping a thin closer function alongside the reader. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Signed-off-by: Ruihao Chen <joechenrh@gmail.com>

joechenrh · 2026-07-24T10:22:57Z

 	StartAfter string
 }

-// ReadSeekCloser is the interface that groups the basic Read, Seek and Close methods.


Redundant with io.ReadSeekCloser

joechenrh · 2026-07-24T10:23:03Z

 }

-// ReadSeekCloser = Reader + Seeker + Closer
-type ReadSeekCloser interface {


Redundant with io.ReadSeekCloser

Signed-off-by: Ruihao Chen <joechenrh@gmail.com>

joechenrh · 2026-07-27T02:23:36Z

/hold

ti-chi-bot · 2026-07-27T02:37:46Z

@D3Hunter: Your lgtm message is repeated, so it is ignored.

Details

In 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 kubernetes-sigs/prow repository.

D3Hunter · 2026-07-27T03:08:52Z

/retest

D3Hunter · 2026-07-27T04:10:36Z

/cherry-pick release-nextgen-202603

ti-chi-bot · 2026-07-27T04:10:38Z

@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.

Details

In response to this:

/cherry-pick release-nextgen-202603

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.

ti-chi-bot · 2026-07-29T04:11:46Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: D3Hunter, GMHDBJD

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~lightning/OWNERS~~ [D3Hunter,GMHDBJD]
~~pkg/dumpformat/OWNERS~~ [D3Hunter,GMHDBJD]
~~pkg/dxf/OWNERS~~ [D3Hunter,GMHDBJD]
~~pkg/executor/importer/OWNERS~~ [D3Hunter,GMHDBJD]
~~pkg/lightning/OWNERS~~ [D3Hunter,GMHDBJD]
~~pkg/objstore/OWNERS~~ [D3Hunter,GMHDBJD]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ti-chi-bot · 2026-07-29T04:11:51Z

[LGTM Timeline notifier]

Timeline:

2026-05-14 06:45:51.710985677 +0000 UTC m=+334520.243764996: ☑️ agreed by D3Hunter.
2026-07-29 04:11:50.463498217 +0000 UTC m=+1982896.499593273: ☑️ agreed by GMHDBJD.

joechenrh · 2026-07-29T05:34:40Z

Forget to unhold.
/unhold

ti-chi-bot · 2026-07-29T06:58:24Z

@D3Hunter: new pull request created to branch release-nextgen-202603: #70193.
But this PR has conflicts, please resolve them!

Details

In response to this:

/cherry-pick release-nextgen-202603

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.

… (#70193) ref #66180

ti-chi-bot Bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels May 9, 2026

ti-chi-bot Bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 9, 2026

joechenrh and others added 2 commits May 11, 2026 14:57

importer, mydump: rename wholeFileInMemoryThreshold to smallFileThres…

59c8623

…hold Trim the verbose comments added in the previous commit and use the shorter name in the constant, parser field comment, and tests. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

joechenrh marked this pull request as ready for review May 11, 2026 10:29

ti-chi-bot Bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 11, 2026

ingress-bot reviewed May 11, 2026

View reviewed changes

Comment thread pkg/lightning/mydump/parquet_wrapper.go Outdated

Comment thread pkg/lightning/mydump/parquet_wrapper.go Outdated

Comment thread pkg/lightning/mydump/loader.go Outdated

Comment thread pkg/lightning/mydump/loader.go Outdated

ti-chi-bot Bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 12, 2026

joechenrh force-pushed the parquet-whole-file-cache branch from e2f81ba to aff6152 Compare May 12, 2026 10:23

ti-chi-bot Bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels May 12, 2026

joechenrh and others added 8 commits May 12, 2026 18:31

mydump: trim prepareReader doc comment

fc5f470

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

mydump: trim smallFileBase and getBuilder doc comments

79e4e83

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

mydump: rename smallFileBase to preloadBase

baf4a7f

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Update

660b64d

Signed-off-by: Ruihao Chen <joechenrh@gmail.com>

joechenrh added 4 commits July 24, 2026 17:52

parquet: verify actual object storage request counts

4c1b034

parquet: focus request count test on reads

0e4c4c8

parquet: inline reader opener type

78002b7

*: use io.ReadSeekCloser directly

7989bdc

joechenrh commented Jul 24, 2026

View reviewed changes

joechenrh added 3 commits July 26, 2026 17:15

parquet: cap whole-file preload at 32 MiB

3b531c3

parquet: cover whole-file preload across row groups

0919a2b

update

88af75e

Signed-off-by: Ruihao Chen <joechenrh@gmail.com>

ti-chi-bot Bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 27, 2026

D3Hunter approved these changes Jul 27, 2026

View reviewed changes

GMHDBJD approved these changes Jul 29, 2026

View reviewed changes

ti-chi-bot Bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Jul 29, 2026

ti-chi-bot Bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 29, 2026

ti-chi-bot Bot merged commit ab79433 into pingcap:master Jul 29, 2026
35 checks passed

ti-chi-bot mentioned this pull request Jul 29, 2026

importer, mydump: preload small parquet files in a single read (#68250) #70193

Merged

1 of 13 tasks

ti-chi-bot Bot pushed a commit that referenced this pull request Jul 30, 2026

importer, mydump: preload small parquet files in a single read (#68250)…

b79ac99

… (#70193) ref #66180

joechenrh deleted the parquet-whole-file-cache branch July 31, 2026 05:08

This was referenced Aug 4, 2026

importinto: log chunk count and size #70271

Merged

dumpformat: track Parquet scan progress by source file offset #70307

Merged

Conversation

joechenrh commented May 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What problem does this PR solve?

What changed and how does it work?

Benchmark

Check List

Release note

Summary by CodeRabbit

Uh oh!

ti-chi-bot Bot commented May 9, 2026

Uh oh!

coderabbitai Bot commented May 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews paused

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Suggested labels

Suggested reviewers

Poem

❌ Failed checks (1 warning)

Uh oh!

tiprow Bot commented May 9, 2026

Uh oh!

pantheon-ai Bot commented May 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented May 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ingress-bot commented May 11, 2026

Uh oh!

ingress-bot left a comment

Choose a reason for hiding this comment

Summary

⚠️ [Major] (2)

🟡 [Minor] (2)

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

joechenrh Jul 24, 2026

Choose a reason for hiding this comment

Uh oh!

joechenrh Jul 24, 2026

Choose a reason for hiding this comment

Uh oh!

joechenrh commented Jul 27, 2026

Uh oh!

ti-chi-bot Bot commented Jul 27, 2026

Uh oh!

D3Hunter commented Jul 27, 2026

Uh oh!

D3Hunter commented Jul 27, 2026

Uh oh!

ti-chi-bot commented Jul 27, 2026

Uh oh!

ti-chi-bot Bot commented Jul 29, 2026

Uh oh!

ti-chi-bot Bot commented Jul 29, 2026

[LGTM Timeline notifier]

Uh oh!

joechenrh commented Jul 29, 2026

Uh oh!

Uh oh!

ti-chi-bot commented Jul 29, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

joechenrh commented May 9, 2026 •

edited

Loading

coderabbitai Bot commented May 9, 2026 •

edited

Loading

pantheon-ai Bot commented May 11, 2026 •

edited

Loading

codecov Bot commented May 11, 2026 •

edited

Loading