Repository navigation
importer, mydump: preload small parquet files in a single read (#68250) - #70193
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. |
ti-chi-bot
Bot
added
size/XXL
📝 WalkthroughWalkthroughParquet parsing now accepts deferred reader openers and file sizes, enabling whole-file, row-group, or streaming access. Importers and memory estimation pass the new metadata, while reader interfaces use ChangesParquet reader refactor
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Importer
participant NewParquetParser
participant prepareReader
participant ObjectStore
Importer->>NewParquetParser: provide opener, path, and file size
NewParquetParser->>prepareReader: choose preload strategy
prepareReader->>ObjectStore: open or preload Parquet data
prepareReader-->>NewParquetParser: return reader and preload base
NewParquetParser-->>Importer: return configured parser
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: 6
🤖 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 `@lightning/pkg/importer/chunk_process.go`:
- Around line 111-117: Resolve the merge conflict in the parser initialization
by removing all conflict markers and retaining the parquetfile.NewParser call
with openReader and chunk.FileMeta.FileSize for preload support. Ensure the
parquetfile package is imported and the surrounding code remains valid Go.
In `@lightning/pkg/importer/get_pre_info.go`:
- Around line 494-502: Resolve the merge conflicts in both parser-construction
sites, removing all conflict markers and retaining the intended
parquetfile.NewParser integration. In the dataFileMeta path and the sampleFile
path, pass each SourceFileMeta’s actual FileSize instead of 0, while preserving
the appropriate reader and metadata arguments so small-file preloading remains
enabled.
In `@pkg/executor/importer/table_import.go`:
- Around line 346-352: Resolve the merge conflict in
TableImporter.EstimateParquetReaderMemory by removing the conflict markers and
retaining the parquetfile.EstimateParquetReaderMemory call with the fileSize
parameter. Update the method signature and delegation consistently so the code
compiles.
In `@pkg/lightning/mydump/parquet_parser_test.go`:
- Around line 39-48: Resolve every remaining merge conflict in the parquet
parser tests by retaining the incoming migration content from
pkg/dumpformat/parquetfile/parser_test.go and removing all conflict markers.
Ensure the file uses the updated imports, newParquetS3StoreForTest, the
FileMeta-aware newParquetParserForTest and NewParser call signatures,
EstimateParquetReaderMemory updates, and the added large-page and whole-file
memory tests.
In `@pkg/lightning/mydump/parquet_wrapper.go`:
- Around line 53-61: Resolve the cherry-pick conflict by removing all markers
and choosing one consistent wrapper/parser naming scheme. In
pkg/lightning/mydump/parquet_wrapper.go:53-61, reconcile
parquetWrapper/readerWrapper and update existing method receivers and
prepareReader accordingly; in pkg/lightning/mydump/parquet_parser.go:419-427,
625-652, 757-782, 801-812, and 959-965, use the same parser type and matching
constructor signature and initialization at getBuilder, constructor, sampling,
and memory-estimation call sites.
- Around line 38-41: Update the wholeFileInMemoryThreshold constant to 128 MiB
so files up to the intended limit use the whole-file preload path; leave the
surrounding memory-bound and object-store behavior unchanged.
🪄 Autofix (Beta)
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: f52832b3-2bae-4319-a779-6a516d66a8ef
📒 Files selected for processing (13)
lightning/pkg/importer/chunk_process.golightning/pkg/importer/get_pre_info.gopkg/dumpformat/parquetfile/BUILD.bazelpkg/dxf/importinto/task_executor.gopkg/executor/importer/import.gopkg/executor/importer/table_import.gopkg/lightning/mydump/csv_parser.gopkg/lightning/mydump/parquet_parser.gopkg/lightning/mydump/parquet_parser_test.gopkg/lightning/mydump/parquet_wrapper.gopkg/lightning/mydump/parser.gopkg/lightning/mydump/reader.gopkg/objstore/storeapi/storage.go
💤 Files with no reviewable changes (1)
- pkg/objstore/storeapi/storage.go
| <<<<<<< HEAD | ||
| parser, err = mydump.NewParquetParser(ctx, store, reader, chunk.FileMeta.Path, chunk.FileMeta.ParquetMeta) | ||
| ======= | ||
| parser, err = parquetfile.NewParser( | ||
| ctx, store, openReader, chunk.FileMeta.Path, chunk.FileMeta.FileSize, chunk.FileMeta.ParquetMeta, | ||
| ) | ||
| >>>>>>> ab79433f43c (importer, mydump: preload small parquet files in a single read (#68250)) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Unresolved git merge-conflict markers left in source.
Lines 111-117 contain literal <<<<<<< HEAD / ======= / >>>>>>> markers — this is invalid Go syntax and will fail to compile. Resolve by keeping the incoming (parquetfile.NewParser) branch that supports the new preload feature, and ensure the parquetfile package is imported.
🐛 Proposed resolution
-<<<<<<< HEAD
- parser, err = mydump.NewParquetParser(ctx, store, reader, chunk.FileMeta.Path, chunk.FileMeta.ParquetMeta)
-=======
parser, err = parquetfile.NewParser(
ctx, store, openReader, chunk.FileMeta.Path, chunk.FileMeta.FileSize, chunk.FileMeta.ParquetMeta,
)
->>>>>>> ab79433f43c (importer, mydump: preload small parquet files in a single read (`#68250`))📝 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.
| <<<<<<< HEAD | |
| parser, err = mydump.NewParquetParser(ctx, store, reader, chunk.FileMeta.Path, chunk.FileMeta.ParquetMeta) | |
| ======= | |
| parser, err = parquetfile.NewParser( | |
| ctx, store, openReader, chunk.FileMeta.Path, chunk.FileMeta.FileSize, chunk.FileMeta.ParquetMeta, | |
| ) | |
| >>>>>>> ab79433f43c (importer, mydump: preload small parquet files in a single read (#68250)) | |
| parser, err = parquetfile.NewParser( | |
| ctx, store, openReader, chunk.FileMeta.Path, chunk.FileMeta.FileSize, chunk.FileMeta.ParquetMeta, | |
| ) |
🤖 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 `@lightning/pkg/importer/chunk_process.go` around lines 111 - 117, Resolve the
merge conflict in the parser initialization by removing all conflict markers and
retaining the parquetfile.NewParser call with openReader and
chunk.FileMeta.FileSize for preload support. Ensure the parquetfile package is
imported and the surrounding code remains valid Go.
| <<<<<<< HEAD | ||
| parser, err = mydump.NewParquetParser( | ||
| ctx, p.srcStorage, reader, | ||
| dataFileMeta.Path, mydump.ParquetFileMeta{}, | ||
| ======= | ||
| parser, err = parquetfile.NewParser( | ||
| ctx, p.srcStorage, openReader, | ||
| dataFileMeta.Path, 0, parquetfile.FileMeta{}, | ||
| >>>>>>> ab79433f43c (importer, mydump: preload small parquet files in a single read (#68250)) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Unresolved merge-conflict markers, and dropped fileSize when resolving.
Both hunks still contain literal <<<<<<</=======/>>>>>>> markers (invalid Go syntax, will not compile). Additionally, the incoming side hardcodes fileSize=0 when calling parquetfile.NewParser, even though dataFileMeta.FileSize (line 494 hunk) and sampleFile.FileSize (line 676 hunk) are readily available on mydump.SourceFileMeta. Passing 0 forces these pre-import sampling paths to skip the new whole-file preload optimization entirely, undermining this PR's stated goal for the many-small-files case.
🐛 Proposed resolution (both sites)
-<<<<<<< HEAD
- parser, err = mydump.NewParquetParser(
- ctx, p.srcStorage, reader,
- dataFileMeta.Path, mydump.ParquetFileMeta{},
-=======
parser, err = parquetfile.NewParser(
ctx, p.srcStorage, openReader,
- dataFileMeta.Path, 0, parquetfile.FileMeta{},
->>>>>>> ab79433f43c (importer, mydump: preload small parquet files in a single read (`#68250`))
+ dataFileMeta.Path, dataFileMeta.FileSize, parquetfile.FileMeta{},
)-<<<<<<< HEAD
- parser, err = mydump.NewParquetParser(
- ctx, p.srcStorage, reader,
- sampleFile.Path, mydump.ParquetFileMeta{},
-=======
parser, err = parquetfile.NewParser(
ctx, p.srcStorage, openReader,
- sampleFile.Path, 0, parquetfile.FileMeta{},
->>>>>>> ab79433f43c (importer, mydump: preload small parquet files in a single read (`#68250`))
+ sampleFile.Path, sampleFile.FileSize, parquetfile.FileMeta{},
)Also applies to: 676-684
🤖 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 `@lightning/pkg/importer/get_pre_info.go` around lines 494 - 502, Resolve the
merge conflicts in both parser-construction sites, removing all conflict markers
and retaining the intended parquetfile.NewParser integration. In the
dataFileMeta path and the sampleFile path, pass each SourceFileMeta’s actual
FileSize instead of 0, while preserving the appropriate reader and metadata
arguments so small-file preloading remains enabled.
| <<<<<<< HEAD | ||
| func (ti *TableImporter) EstimateParquetReaderMemory(ctx context.Context, path string) (int64, error) { | ||
| return mydump.EstimateParquetReaderMemory(ctx, ti.LoadDataController.dataStore, path) | ||
| ======= | ||
| func (ti *TableImporter) EstimateParquetReaderMemory(ctx context.Context, path string, fileSize int64) (int64, error) { | ||
| return parquetfile.EstimateParquetReaderMemory(ctx, ti.LoadDataController.dataStore, path, fileSize) | ||
| >>>>>>> ab79433f43c (importer, mydump: preload small parquet files in a single read (#68250)) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Unresolved git merge-conflict markers left in source.
Same compile-breaking issue as the other files in this cohort — resolve to the parquetfile.EstimateParquetReaderMemory branch with the fileSize parameter.
🐛 Proposed resolution
-<<<<<<< HEAD
-func (ti *TableImporter) EstimateParquetReaderMemory(ctx context.Context, path string) (int64, error) {
- return mydump.EstimateParquetReaderMemory(ctx, ti.LoadDataController.dataStore, path)
-=======
func (ti *TableImporter) EstimateParquetReaderMemory(ctx context.Context, path string, fileSize int64) (int64, error) {
return parquetfile.EstimateParquetReaderMemory(ctx, ti.LoadDataController.dataStore, path, fileSize)
->>>>>>> ab79433f43c (importer, mydump: preload small parquet files in a single read (`#68250`))
}📝 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.
| <<<<<<< HEAD | |
| func (ti *TableImporter) EstimateParquetReaderMemory(ctx context.Context, path string) (int64, error) { | |
| return mydump.EstimateParquetReaderMemory(ctx, ti.LoadDataController.dataStore, path) | |
| ======= | |
| func (ti *TableImporter) EstimateParquetReaderMemory(ctx context.Context, path string, fileSize int64) (int64, error) { | |
| return parquetfile.EstimateParquetReaderMemory(ctx, ti.LoadDataController.dataStore, path, fileSize) | |
| >>>>>>> ab79433f43c (importer, mydump: preload small parquet files in a single read (#68250)) | |
| func (ti *TableImporter) EstimateParquetReaderMemory(ctx context.Context, path string, fileSize int64) (int64, error) { | |
| return parquetfile.EstimateParquetReaderMemory(ctx, ti.LoadDataController.dataStore, path, fileSize) | |
| } |
🤖 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/executor/importer/table_import.go` around lines 346 - 352, Resolve the
merge conflict in TableImporter.EstimateParquetReaderMemory by removing the
conflict markers and retaining the parquetfile.EstimateParquetReaderMemory call
with the fileSize parameter. Update the method signature and delegation
consistently so the code compiles.
| // wholeFileInMemoryThreshold caps whole-file preloading. The lower limit | ||
| // keeps the per-parser memory cost bounded while retaining the object-store | ||
| // request reduction for small files. | ||
| wholeFileInMemoryThreshold = 32 * units.MiB |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Restore the intended 128 MiB whole-file threshold.
This sets 32 MiB, so files from 32–128 MiB miss the single-read preload path described by the PR objective.
Proposed fix
- wholeFileInMemoryThreshold = 32 * units.MiB
+ wholeFileInMemoryThreshold = 128 * units.MiB📝 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.
| // wholeFileInMemoryThreshold caps whole-file preloading. The lower limit | |
| // keeps the per-parser memory cost bounded while retaining the object-store | |
| // request reduction for small files. | |
| wholeFileInMemoryThreshold = 32 * units.MiB | |
| // wholeFileInMemoryThreshold caps whole-file preloading. The lower limit | |
| // keeps the per-parser memory cost bounded while retaining the object-store | |
| // request reduction for small files. | |
| wholeFileInMemoryThreshold = 128 * units.MiB |
🤖 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_wrapper.go` around lines 38 - 41, Update the
wholeFileInMemoryThreshold constant to 128 MiB so files up to the intended limit
use the whole-file preload path; leave the surrounding memory-bound and
object-store behavior unchanged.
Signed-off-by: Ruihao Chen <joechenrh@gmail.com>
|
Cherry-pick conflicts appear resolved; removing the |
ti-chi-bot
removed
the
do-not-merge/hold
Signed-off-by: Ruihao Chen <joechenrh@gmail.com>
ti-chi-bot
Bot
added
size/XL
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/lightning/mydump/parquet_parser_test.go (1)
1393-1395: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAlign the whole-file cutoff with the 128 MiB objective.
This test locks whole-file preloading to 32 MiB, but the PR objective requires files up to 128 MiB to be loaded with one read. Otherwise, 32–128 MiB files will continue down the row-group/multi-read path. Update the implementation and expected threshold together.
🤖 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 1393 - 1395, Update wholeFileInMemoryThreshold and its assertion in TestParquetParserWholeFileInMemory to 128 MiB, matching rowGroupInMemoryThreshold, so files up to 128 MiB use the single-read whole-file path.
🤖 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.
Outside diff comments:
In `@pkg/lightning/mydump/parquet_parser_test.go`:
- Around line 1393-1395: Update wholeFileInMemoryThreshold and its assertion in
TestParquetParserWholeFileInMemory to 128 MiB, matching
rowGroupInMemoryThreshold, so files up to 128 MiB use the single-read whole-file
path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7ad30c97-cbb6-470e-bc48-b2fe4bd1b439
📒 Files selected for processing (6)
lightning/pkg/importer/chunk_process.golightning/pkg/importer/get_pre_info.gopkg/executor/importer/table_import.gopkg/lightning/mydump/parquet_parser.gopkg/lightning/mydump/parquet_parser_test.gopkg/lightning/mydump/parquet_wrapper.go
Signed-off-by: Ruihao Chen <joechenrh@gmail.com>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## release-nextgen-202603 #70193 +/- ##
===========================================================
Coverage ? 76.2307%
===========================================================
Files ? 1937
Lines ? 541423
Branches ? 0
===========================================================
Hits ? 412731
Misses ? 128692
Partials ? 0
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
[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
…se-nextgen-202603
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/lightning/mydump/parquet_parser_test.go (1)
1393-1395: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winAlign the whole-file threshold with the PR target.
Line 1394 locks whole-file preloading to 32 MiB, so files from 33 MiB through 128 MiB take the row-group path instead of the promised whole-file preload path. Set the whole-file threshold to 128 MiB and update this assertion, or revise the PR objective if 32 MiB is intentional. Based on the PR objective, whole-file preload is intended for files up to 128 MiB.
🤖 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 1393 - 1395, Update wholeFileInMemoryThreshold to 128 MiB so whole-file preloading applies through the intended limit, and change TestParquetParserWholeFileInMemory to assert the new value while preserving the existing rowGroupInMemoryThreshold assertion.
🤖 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.
Outside diff comments:
In `@pkg/lightning/mydump/parquet_parser_test.go`:
- Around line 1393-1395: Update wholeFileInMemoryThreshold to 128 MiB so
whole-file preloading applies through the intended limit, and change
TestParquetParserWholeFileInMemory to assert the new value while preserving the
existing rowGroupInMemoryThreshold assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4e8f1052-022d-417e-a34d-bda095856794
📒 Files selected for processing (2)
pkg/lightning/mydump/parquet_parser.gopkg/lightning/mydump/parquet_parser_test.go
b79ac99
into
pingcap:release-nextgen-202603
3 participants
This is an automated cherry-pick of #68250
What problem does this PR solve?
Issue Number: ref #66180
When
IMPORT INTOingests many small parquet files, each file's footer and row-group data are fetched in separate opens — two for footer parsing inNewParquetParser, then one per row group ingetBuilder→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).NewParquetParserreads the file once viaprepareReaderwhen it fits, uses the buffer for footer parsing, and letsgetBuilderserve every column from the same buffer. Larger files still fall through to per-row-group preload or streaming.Benchmark
Staging cluster, 1550 × ~480 KiB parquet files on AWS S3, prod-aligned schema. Baseline and PR back-to-back, 1 node × 1 thread.
takeTimep50Gain lands in the inter-chunk gap (parser-open path);
takeTimeand deliver spikes are unaffected, as expected.Check List
Tests
Side effects
Documentation
Release note
Summary by CodeRabbit