Repository navigation
executor: tiny optimize import into from select performance (#60384) - #70287
ti-chi-bot wants to merge 2 commits into
Conversation
Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
ti-chi-bot
added
do-not-merge/hold
|
@crazycs520 This PR has conflicts, I have hold it. |
|
This cherry pick PR is for a release branch and has not yet been approved by triage owners. To merge this cherry pick:
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 kubernetes-sigs/prow repository. |
|
@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. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
📝 WalkthroughWalkthroughThe import-from-select path now streams buffered ChangesChunk-based import streaming
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant SelectExecutor
participant TableImporter
participant QueryChunkProcessor
participant KVEncoder
SelectExecutor->>TableImporter: send QueryChunk batches
TableImporter->>QueryChunkProcessor: pass chunk channel
QueryChunkProcessor->>KVEncoder: encode rows converted from chunks
SelectExecutor->>TableImporter: close chunk channel
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/executor/importer/chunk_process.go (1)
110-136: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winResolve all unresolved Git merge-conflict markers left by the cherry-pick.
This automated cherry-pick of
#60384intorelease-8.5left literal<<<<<<< HEAD/=======/>>>>>>>markers in five locations across three files. None of these files compile as shown. This matches the PR's held status pending conflict resolution; resolve each site by keeping the chunk-based (QueryChunk) branch and removing the row-based (QueryRow) branch and the markers.
pkg/executor/importer/chunk_process.go#L110-L136: inreadRow, keep ther.currChk = currChk; r.cursor = 0; r.numRows = ...branch and drop the oldQueryRow-basedrowToEncodebranch.pkg/executor/importer/chunk_process.go#L545-L552: innewQueryChunkProcessor, keep thechunkCh chan QueryChunk, encoder KVEncodersignature and drop therowCh chan QueryRow, encoder *TableKVEncodersignature.pkg/executor/importer/importer_testkit_test.go#L41-L45: keep themysqlimport needed formysql.TypeLong, and confirm whetherpmodelis still required elsewhere in the file.pkg/executor/importer/importer_testkit_test.go#L333-L390: in the "query chunk" subtest, keep thechkCh/QueryChunk-based setup and drop the oldrowsCh/QueryRow-based setup.pkg/executor/importer/table_import_testkit_test.go#L27-L38: keep themysqlimport needed formysql.TypeLong;pmodelremains required elsewhere in this file (lines 71, 73, 78, 81), so keep it too.🤖 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/chunk_process.go` around lines 110 - 136, Resolve all merge-conflict markers by retaining the QueryChunk implementation: in pkg/executor/importer/chunk_process.go ranges 110-136 and 545-552, keep the chunk state updates in readRow and the chunkCh chan QueryChunk, encoder KVEncoder signature; in pkg/executor/importer/importer_testkit_test.go ranges 41-45 and 333-390, keep the mysql import, preserve pmodel only if used elsewhere, and retain the chkCh/QueryChunk query-chunk subtest setup; in pkg/executor/importer/table_import_testkit_test.go range 27-38, keep both mysql and pmodel imports, removing the obsolete QueryRow branches and all conflict markers.
🧹 Nitpick comments (2)
pkg/executor/importer/table_import.go (1)
634-637: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUpdate the stale doc comment on
SetSelectedChunkCh.The comment still says the setter "sets the channel to receive selected rows." The channel now carries
QueryChunkbatches, not individual rows. Update the wording to describe chunks.Based on learnings, "Keep exported-symbol doc comments, and prefer semantic constraints over name restatement."📝 Proposed doc comment fix
-// SetSelectedChunkCh sets the channel to receive selected rows. +// SetSelectedChunkCh sets the channel to receive selected query-result chunks. func (ti *TableImporter) SetSelectedChunkCh(ch chan QueryChunk) { ti.chunkCh = ch }🤖 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 634 - 637, Update the exported SetSelectedChunkCh method’s doc comment to describe that it configures the channel for receiving QueryChunk batches, replacing the stale reference to individual selected rows while preserving the setter implementation.Source: Coding guidelines
pkg/executor/importer/chunk_process.go (1)
149-155: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename
queryRowEncodeReaderto reflect chunk-based behavior.
queryRowEncodeReaderwrapsqueryChunkEncodeReaderand consumes achan QueryChunk, not individual rows. Keeping the "Row" name next to the "Chunk"-named type it wraps is confusing for future readers.Based on learnings, Go code "SHOULD be self-documenting through clear naming and structure."♻️ Proposed rename
-// queryRowEncodeReader wraps a queryChunkEncodeReader as a encodeReaderFn. -func queryRowEncodeReader(chunkCh <-chan QueryChunk) encodeReaderFn { +// queryChunkEncodeReaderFn wraps a queryChunkEncodeReader as an encodeReaderFn. +func queryChunkEncodeReaderFn(chunkCh <-chan QueryChunk) encodeReaderFn { reader := queryChunkEncodeReader{chunkCh: chunkCh} return func(ctx context.Context) (data rowToEncode, closed bool, err error) { return reader.readRow(ctx) } }🤖 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/chunk_process.go` around lines 149 - 155, Rename queryRowEncodeReader to a chunk-oriented name that matches queryChunkEncodeReader and its QueryChunk channel input, and update every reference to the function accordingly. Preserve its wrapper behavior and return signature unchanged.Source: Coding guidelines
🤖 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/executor/importer/chunk_process.go`:
- Around line 138-147: Update the rowToEncode construction in the query-row path
to set startPos explicitly to the -1 sentinel, preserving “not applicable”
diagnostics for encode failures. Add a brief comment beside the rowID expression
in this block explaining that r.cursor is incremented first and therefore
represents the 1-based position within the chunk.
---
Outside diff comments:
In `@pkg/executor/importer/chunk_process.go`:
- Around line 110-136: Resolve all merge-conflict markers by retaining the
QueryChunk implementation: in pkg/executor/importer/chunk_process.go ranges
110-136 and 545-552, keep the chunk state updates in readRow and the chunkCh
chan QueryChunk, encoder KVEncoder signature; in
pkg/executor/importer/importer_testkit_test.go ranges 41-45 and 333-390, keep
the mysql import, preserve pmodel only if used elsewhere, and retain the
chkCh/QueryChunk query-chunk subtest setup; in
pkg/executor/importer/table_import_testkit_test.go range 27-38, keep both mysql
and pmodel imports, removing the obsolete QueryRow branches and all conflict
markers.
---
Nitpick comments:
In `@pkg/executor/importer/chunk_process.go`:
- Around line 149-155: Rename queryRowEncodeReader to a chunk-oriented name that
matches queryChunkEncodeReader and its QueryChunk channel input, and update
every reference to the function accordingly. Preserve its wrapper behavior and
return signature unchanged.
In `@pkg/executor/importer/table_import.go`:
- Around line 634-637: Update the exported SetSelectedChunkCh method’s doc
comment to describe that it configures the channel for receiving QueryChunk
batches, replacing the stale reference to individual selected rows while
preserving the setter implementation.
🪄 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: 06b513a3-ef5a-4ea1-bc16-9e46c2f4a1ef
📒 Files selected for processing (7)
pkg/executor/import_into.gopkg/executor/importer/BUILD.bazelpkg/executor/importer/chunk_process.gopkg/executor/importer/engine_process.gopkg/executor/importer/importer_testkit_test.gopkg/executor/importer/table_import.gopkg/executor/importer/table_import_testkit_test.go
| row := r.currChk.Chk.GetRow(r.cursor) | ||
| r.cursor++ | ||
| data = rowToEncode{ | ||
| row: row.GetDatumRow(r.currChk.Fields), | ||
| rowID: r.currChk.RowIDOffset + int64(r.cursor), | ||
| endOffset: -1, | ||
| resetFn: func() {}, | ||
| } | ||
| return | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Set startPos explicitly for query-sourced rows.
The previous implementation set startPos: -1 to mark that query-sourced rows have no file offset. This construction omits startPos, so it defaults to 0. data.startPos feeds into the ErrEncodeKV error message in encodeLoop, so an encode failure on a query-sourced row will report offset 0 instead of indicating "not applicable." Restore the -1 sentinel to avoid misleading diagnostics.
Add a short comment on the rowID computation too: RowIDOffset + int64(r.cursor) relies on r.cursor already being incremented to a 1-based count within the chunk. This is easy to misread as an off-by-one error without a note.
🔧 Proposed fix
row := r.currChk.Chk.GetRow(r.cursor)
r.cursor++
+ // r.cursor is now the 1-based count of rows consumed from this chunk;
+ // RowIDOffset holds the cumulative row count before this chunk, so their
+ // sum reproduces the previous 1-based QueryRow.ID scheme.
data = rowToEncode{
row: row.GetDatumRow(r.currChk.Fields),
rowID: r.currChk.RowIDOffset + int64(r.cursor),
endOffset: -1,
+ startPos: -1,
resetFn: func() {},
}
return📝 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.
| row := r.currChk.Chk.GetRow(r.cursor) | |
| r.cursor++ | |
| data = rowToEncode{ | |
| row: row.GetDatumRow(r.currChk.Fields), | |
| rowID: r.currChk.RowIDOffset + int64(r.cursor), | |
| endOffset: -1, | |
| resetFn: func() {}, | |
| } | |
| return | |
| } | |
| row := r.currChk.Chk.GetRow(r.cursor) | |
| r.cursor++ | |
| // r.cursor is now the 1-based count of rows consumed from this chunk; | |
| // RowIDOffset holds the cumulative row count before this chunk, so their | |
| // sum reproduces the previous 1-based QueryRow.ID scheme. | |
| data = rowToEncode{ | |
| row: row.GetDatumRow(r.currChk.Fields), | |
| rowID: r.currChk.RowIDOffset + int64(r.cursor), | |
| endOffset: -1, | |
| startPos: -1, | |
| resetFn: func() {}, | |
| } | |
| return |
🤖 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/chunk_process.go` around lines 138 - 147, Update the
rowToEncode construction in the query-row path to set startPos explicitly to the
-1 sentinel, preserving “not applicable” diagnostics for encode failures. Add a
brief comment beside the rowID expression in this block explaining that r.cursor
is incremented first and therefore represents the 1-based position within the
chunk.
|
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/executor/importer/chunk_process.go (1)
137-143: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename the row-based reader wrapper.
queryRowEncodeReaderconsumesQueryChunkvalues and wrapsqueryChunkEncodeReader. Rename it toqueryChunkEncodeReaderFn, and update the call at Line 558. This makes the chunk-based contract clear. (raw.githubusercontent.com)As per coding guidelines, Go code SHOULD be self-documenting through clear naming and structure.
Proposed rename
-// queryRowEncodeReader wraps a queryChunkEncodeReader as an encodeReaderFn. -func queryRowEncodeReader(chunkCh <-chan QueryChunk) encodeReaderFn { +// queryChunkEncodeReaderFn wraps a queryChunkEncodeReader as an encodeReaderFn. +func queryChunkEncodeReaderFn(chunkCh <-chan QueryChunk) encodeReaderFn {- queryRowEncodeReader(chunkCh), + queryChunkEncodeReaderFn(chunkCh),Also applies to: 558-558
🤖 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/chunk_process.go` around lines 137 - 143, Rename the wrapper function queryRowEncodeReader to queryChunkEncodeReaderFn and update its call site near the referenced execution path, preserving the existing reader construction and readRow behavior.Source: Coding guidelines
🤖 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/executor/importer/chunk_process.go`:
- Around line 137-143: Rename the wrapper function queryRowEncodeReader to
queryChunkEncodeReaderFn and update its call site near the referenced execution
path, preserving the existing reader construction and readRow behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 56ee0d1c-a8cc-4ce3-907d-a3841974e614
📒 Files selected for processing (3)
pkg/executor/importer/chunk_process.gopkg/executor/importer/importer_testkit_test.gopkg/executor/importer/table_import_testkit_test.go
💤 Files with no reviewable changes (2)
- pkg/executor/importer/importer_testkit_test.go
- pkg/executor/importer/table_import_testkit_test.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## release-8.5 #70287 +/- ##
================================================
Coverage ? 55.1575%
================================================
Files ? 1849
Lines ? 666418
Branches ? 0
================================================
Hits ? 367580
Misses ? 271413
Partials ? 27425
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
This branch has not been deployed
3 participants
This is an automated cherry-pick of #60384
What problem does this PR solve?
Issue Number: ref #60377, close #60373
Problem Summary: tiny optimize import into from select performance
What changed and how does it work?
Tiny fix for better pipeline.
Manual test:
Before:
related tidb log:
The
readDurhas43.39s.This PR:
related tidb log:
The
readDuronly has6.5s.Check List
Tests
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.
Summary by CodeRabbit
Performance
Reliability