distsql: Add `SelectResultIter` to read rows from cop-task in the iterator way #63319

lcwangchao · 2025-09-02T01:39:39Z

What problem does this PR solve?

Issue Number: close #63318

Problem Summary:

After pingcap/tipb#376 is merged, the cop-task response may contain multiple output channels (i.e., the push down IndexLookup executor), and each of these channels may have a different kind of schema.

SelectResult should support reading the results from multiple channels.

What changed and how does it work?

A new interface SelectResultIter is provided.
SelectResultIter is defined as:

// SelectResultIter is an iterator that is used to iterate the rows from SelectResult.
type SelectResultIter interface {
	// Next returns the next row.
	// If the iterator is drained, the `SelectResultRow.IsEmpty()` returns true.
	Next(ctx context.Context) (SelectResultRow, error)
	// Close closes the iterator.
	Close() error
}

The Next is used to read the rows from the cop-task. SelectResultRow contains an extra field Channel to indicate where the row is from:

// SelectResultRow indicates the row returned by the SelectResultIter
type SelectResultRow struct {
	// `Channel` indicates the index where this row locates.
	// When Channel < len(IntermediateChannels), it means this row is from intermediate result.
	// Otherwise, if Channel == len(IntermediateChannels), it means this row is from final result.
	Channel int
	// Row is the actual data of this row.
	chunk.Row
}

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

Please refer to Release Notes Language Style Guide to write a quality release note.

None

ti-chi-bot · 2025-09-02T01:39:49Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign cfzjywxk for approval. For more information see the Code Review Process.
Please ensure that each of them provides their approval before proceeding.

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

Needs approval from an approver in each of these files:

OWNERS
pkg/distsql/OWNERS

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

lcwangchao · 2025-09-02T01:43:04Z

/retest

codecov · 2025-09-02T01:59:19Z

Codecov Report

❌ Patch coverage is 91.01796% with 15 lines in your changes missing coverage. Please review.
✅ Project coverage is 73.2773%. Comparing base (a1183bc) to head (c77a2e1).
⚠️ Report is 19 commits behind head on master.

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #63319        +/-   ##
================================================
+ Coverage   72.7739%   73.2773%   +0.5033%     
================================================
  Files          1832       1832                
  Lines        495728     497881      +2153     
================================================
+ Hits         360761     364834      +4073     
+ Misses       113021     111165      -1856     
+ Partials      21946      21882        -64

Flag	Coverage Δ
integration	`41.9709% <2.9940%> (?)`
unit	`72.3388% <91.0179%> (+0.0936%)`	⬆️

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

Components	Coverage Δ
dumpling	`52.8700% <ø> (ø)`
parser	`∅ <ø> (∅)`
br	`46.5396% <ø> (+0.0482%)`	⬆️

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

lcwangchao · 2025-09-02T02:09:00Z

/retest

…rator way

cfzjywxk · 2025-09-02T07:28:13Z

pkg/distsql/select_result.go

@@ -381,6 +415,16 @@ func (r *selectResult) fetchResp(ctx context.Context) error {
 		if err != nil {
 			return errors.Trace(err)
 		}
+
+		if len(r.selectResp.IntermediateOutputs) != len(intermediateOutputTypes) {
+			return errors.Errorf(


Does it indicates inconsistency? If so the inconsistencyReporter could be used to log necessary information.

This situation should never happen, but it has no relationship with the data consistency. If it happens, it is more like the TiDB or TiKV code has a bug that makes the wrong assumption of the data channels.

Copilot

Pull Request Overview

This PR introduces a new SelectResultIter interface that enables reading rows from coprocessor tasks in an iterator-based manner, supporting multiple output channels from cop-task responses. This change accommodates scenarios where cop-task responses contain multiple output channels (such as push-down IndexLookup executors) with different schemas per channel.

Key changes:

Added SelectResultIter interface with Next() and Close() methods for iterating over rows
Introduced SelectResultRow struct to encapsulate row data with a channel identifier
Implemented channel-based iteration logic to handle intermediate outputs from coprocessor responses

Reviewed Changes

Copilot reviewed 7 out of 8 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
pkg/distsql/select_result.go	Core implementation of SelectResultIter interface and channel-based row iteration logic
pkg/distsql/select_result_test.go	Comprehensive test coverage for the new iterator functionality and channel handling
pkg/executor/table_readers_required_rows_test.go	Added stub implementation of IntoIter method to maintain interface compatibility
pkg/distsql/distsql_test.go	Enhanced mock infrastructure to support intermediate outputs testing
pkg/distsql/BUILD.bazel	Increased test shard count to accommodate additional test cases
go.mod, DEPS.bzl	Updated tipb and other dependencies to support new features

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

pkg/distsql/select_result.go

lcwangchao · 2025-09-02T09:34:37Z

/retest

lcwangchao · 2025-09-03T04:43:03Z

/retest

pkg/distsql/select_result.go

ekexium · 2025-09-04T13:50:23Z

pkg/distsql/select_result.go

+}
+
+func newSelRespChannelIter(result *selectResult, channel int) (*selRespChannelIter, error) {
+	intest.Assert(result != nil && result.selectResp != nil && len(result.selectResp.IntermediateOutputs) == len(result.intermediateOutputTypes))


Should we return an error in production?

This is an internal call, and some conditions have been checked outside: https://github.com/pingcap/tidb/pull/63319/files#diff-f45688fe21dcc5e4a40e356dc44d0fe3622a8557ac3f8e58ce6d86d3efc0ed43R425

Seems it's more simple to just use an assert

cfzjywxk · 2025-09-05T11:58:02Z

pkg/distsql/select_result.go

+	// `Channel` indicates the index where this row locates.
+	// When Channel < len(IntermediateChannels), it means this row is from intermediate result.
+	// Otherwise, if Channel == len(IntermediateChannels), it means this row is from final result.
+	Channel int


Suggested change

Channel int

ChannelIndex int

Seems better.

cfzjywxk · 2025-09-05T12:06:29Z

pkg/distsql/select_result.go

+					// some intermediate output contains data,
+					// we should return the response even if the main output is empty.
+					return nil
+				}


Is it expected to contain zero lengh chunk in the IntermediateOutputs results? If not should we add assertions here too?

ti-chi-bot bot added the release-note-none Denotes a PR that doesn't merit a release note. label Sep 2, 2025

ti-chi-bot bot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Sep 2, 2025

distsql: Add SelectResultIter to read rows from cop-task in the ite…

ab13c1f

…rator way

lcwangchao force-pushed the select_resp_row_iter branch from 614903c to ab13c1f Compare September 2, 2025 03:11

lcwangchao requested a review from Copilot September 2, 2025 07:15

This comment was marked as outdated.

Sign in to view

cfzjywxk requested review from cfzjywxk, zyguan, you06 and ekexium September 2, 2025 07:25

cfzjywxk reviewed Sep 2, 2025

View reviewed changes

lcwangchao requested a review from Copilot September 2, 2025 08:07

This comment was marked as outdated.

Sign in to view

address AI comments

c77a2e1

lcwangchao force-pushed the select_resp_row_iter branch from c7c122c to c77a2e1 Compare September 2, 2025 08:09

lcwangchao requested a review from Copilot September 2, 2025 08:10

Copilot AI reviewed Sep 2, 2025

View reviewed changes

pkg/distsql/select_result.go Show resolved Hide resolved

pkg/distsql/select_result.go Show resolved Hide resolved

ekexium reviewed Sep 4, 2025

View reviewed changes

cfzjywxk reviewed Sep 5, 2025

View reviewed changes

distsql: Add SelectResultIter to read rows from cop-task in the iterator way #63319

Are you sure you want to change the base?

distsql: Add SelectResultIter to read rows from cop-task in the iterator way #63319

Conversation

lcwangchao commented Sep 2, 2025

What problem does this PR solve?

What changed and how does it work?

Check List

Release note

Uh oh!

ti-chi-bot bot commented Sep 2, 2025

Uh oh!

lcwangchao commented Sep 2, 2025

Uh oh!

codecov bot commented Sep 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

lcwangchao commented Sep 2, 2025

Uh oh!

This comment was marked as outdated.

Uh oh!

cfzjywxk Sep 2, 2025

Choose a reason for hiding this comment

Uh oh!

lcwangchao Sep 2, 2025

Choose a reason for hiding this comment

Uh oh!

This comment was marked as outdated.

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

lcwangchao commented Sep 2, 2025

Uh oh!

lcwangchao commented Sep 3, 2025

Uh oh!

Uh oh!

ekexium Sep 4, 2025

Choose a reason for hiding this comment

Uh oh!

lcwangchao Sep 4, 2025

Choose a reason for hiding this comment

Uh oh!

cfzjywxk Sep 5, 2025

Choose a reason for hiding this comment

Uh oh!

cfzjywxk Sep 5, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

distsql: Add `SelectResultIter` to read rows from cop-task in the iterator way #63319

distsql: Add `SelectResultIter` to read rows from cop-task in the iterator way #63319

codecov bot commented Sep 2, 2025 •

edited

Loading