util: add support columnar scan details (#69331)#69424
Conversation
Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
ti-chi-bot
added
do-not-merge/hold
|
@yongman 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. |
|
[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 |
📝 WalkthroughWalkthroughBumps ChangesTiFlash Columnar Scan Stats
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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)level=error msg="Running error: context loading failed: failed to load packages: failed to load packages: failed to load with go/packages: err: exit status 1: stderr: go: errors parsing go.mod:\ngo.mod:109: malformed module path "<<<<<<<": invalid char '<'\ngo.mod:111: usage: require module/path v1.2.3\ngo.mod:113: usage: require module/path v1.2.3\n" 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 |
|
@ti-chi-bot: The following tests failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pkg/util/execdetails/tiflash_stats.go (1)
562-707: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTighten the exported method docs on
TiFlashColumnarScanContext.
Stringis missing a doc comment, and theClone/Merge/Emptycomments mostly restate the method names instead of the behavioral contract. Please document the actual semantics here (for example, thatMergesums counters but keeps maxima for schema-shape fields) so the exported API is self-explanatory.Suggested comment cleanup
-// Clone implements the deep copy of * TiFlashColumnarScanContext +// Clone returns a copy of the aggregated columnar scan statistics. func (context *TiFlashColumnarScanContext) Clone() TiFlashColumnarScanContext { +// String formats the columnar scan statistics for EXPLAIN ANALYZE output. func (context *TiFlashColumnarScanContext) String() string { -// Merge make sum to merge the information in TiFlashColumnarScanContext +// Merge accumulates per-task counters and keeps the maximum schema-shape fields. func (context *TiFlashColumnarScanContext) Merge(other TiFlashColumnarScanContext) { -// Empty check whether TiFlashColumnarScanContext is empty. +// Empty reports whether no columnar scan statistics have been recorded. func (context *TiFlashColumnarScanContext) Empty() bool {🤖 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/util/execdetails/tiflash_stats.go` around lines 562 - 707, The exported methods on TiFlashColumnarScanContext need clearer documentation: add a proper doc comment for String and rewrite the existing Clone, Merge, and Empty comments in terms of behavior instead of restating names. Use the method symbols Clone, String, Merge, and Empty to describe the contract, especially that Merge accumulates counters while preserving maxima for shape fields like physicalTables and columns, and that Clone returns a deep copy of the context.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 `@DEPS.bzl`:
- Around line 6585-6599: Resolve the unresolved merge conflict in the tipb
go_repository stanza by removing the conflict markers and keeping a single
consistent set of sha256, strip_prefix, and urls in DEPS.bzl; ensure the
selected version matches the intended final go.mod pin so
`@com_github_pingcap_tipb//go-tipb` remains valid.
In `@go.mod`:
- Around line 109-113: Resolve the merge conflict in go.mod by removing the
conflict markers and leaving a single github.com/pingcap/tipb version; choose
the revision that includes ColumnarScanContext so
pkg/util/execdetails/runtime_stats.go and
pkg/util/execdetails/execdetails_test.go stay compatible. After updating go.mod,
make sure the same tipb revision is reflected consistently in DEPS.bzl.
---
Nitpick comments:
In `@pkg/util/execdetails/tiflash_stats.go`:
- Around line 562-707: The exported methods on TiFlashColumnarScanContext need
clearer documentation: add a proper doc comment for String and rewrite the
existing Clone, Merge, and Empty comments in terms of behavior instead of
restating names. Use the method symbols Clone, String, Merge, and Empty to
describe the contract, especially that Merge accumulates counters while
preserving maxima for shape fields like physicalTables and columns, and that
Clone returns a deep copy of the context.
🪄 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
Run ID: 0f8998fb-764f-402b-b7ee-180a53c358f9
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (5)
DEPS.bzlgo.modpkg/util/execdetails/execdetails_test.gopkg/util/execdetails/runtime_stats.gopkg/util/execdetails/tiflash_stats.go
| <<<<<<< HEAD | ||
| sha256 = "2d6aaef873e175599c39f6fe3cf85cabce88a69c157ff7ec4c22fbcbc97648dd", | ||
| strip_prefix = "github.com/pingcap/tipb@v0.0.0-20260210113932-1447c9d7e9fe", | ||
| urls = [ | ||
| "http://bazel-cache.pingcap.net:8080/gomod/github.com/pingcap/tipb/com_github_pingcap_tipb-v0.0.0-20260210113932-1447c9d7e9fe.zip", | ||
| "http://ats.apps.svc/gomod/github.com/pingcap/tipb/com_github_pingcap_tipb-v0.0.0-20260210113932-1447c9d7e9fe.zip", | ||
| "https://cache.hawkingrei.com/gomod/github.com/pingcap/tipb/com_github_pingcap_tipb-v0.0.0-20260210113932-1447c9d7e9fe.zip", | ||
| "https://storage.googleapis.com/pingcapmirror/gomod/github.com/pingcap/tipb/com_github_pingcap_tipb-v0.0.0-20260210113932-1447c9d7e9fe.zip", | ||
| ======= | ||
| sha256 = "5a8c6a6e4487592fea5550c2a550826bd0ad7025c7bbec12365ecc7aed81db0d", | ||
| strip_prefix = "github.com/pingcap/tipb@v0.0.0-20260617071407-7c071244534b", | ||
| urls = [ | ||
| "https://cache.hawkingrei.com/gomod/github.com/pingcap/tipb/com_github_pingcap_tipb-v0.0.0-20260617071407-7c071244534b.zip", | ||
| "https://storage.googleapis.com/pingcapmirror/gomod/github.com/pingcap/tipb/com_github_pingcap_tipb-v0.0.0-20260617071407-7c071244534b.zip", | ||
| >>>>>>> 9f419ca60ca (util: add support columnar scan details (#69331)) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Fix the unresolved conflict in the Bazel tipb pin.
This go_repository stanza is currently invalid Starlark because of the conflict markers, so Bazel cannot resolve @com_github_pingcap_tipb//go-tipb. Resolve it to one sha256/strip_prefix/urls set that matches the final go.mod version.
🤖 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 `@DEPS.bzl` around lines 6585 - 6599, Resolve the unresolved merge conflict in
the tipb go_repository stanza by removing the conflict markers and keeping a
single consistent set of sha256, strip_prefix, and urls in DEPS.bzl; ensure the
selected version matches the intended final go.mod pin so
`@com_github_pingcap_tipb//go-tipb` remains valid.
| <<<<<<< HEAD | ||
| github.com/pingcap/tipb v0.0.0-20260210113932-1447c9d7e9fe | ||
| ======= | ||
| github.com/pingcap/tipb v0.0.0-20260617071407-7c071244534b | ||
| >>>>>>> 9f419ca60ca (util: add support columnar scan details (#69331)) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Resolve the tipb merge conflict before merging.
These conflict markers make go.mod unparsable, and this PR also needs the resolved github.com/pingcap/tipb revision to be the one that exposes ColumnarScanContext used by pkg/util/execdetails/runtime_stats.go and pkg/util/execdetails/execdetails_test.go. Please collapse this to a single version and keep it consistent with DEPS.bzl.
🤖 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 `@go.mod` around lines 109 - 113, Resolve the merge conflict in go.mod by
removing the conflict markers and leaving a single github.com/pingcap/tipb
version; choose the revision that includes ColumnarScanContext so
pkg/util/execdetails/runtime_stats.go and
pkg/util/execdetails/execdetails_test.go stay compatible. After updating go.mod,
make sure the same tipb revision is reflected consistently in DEPS.bzl.
|
/close |
|
@yongman: Closed this PR. 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 kubernetes-sigs/prow repository. |
2 participants
This is an automated cherry-pick of #69331
What problem does this PR solve?
Issue Number: close #69353
Problem Summary:
There is no execution stats summary when the query using columnar instead of tiflash.
What changed and how does it work?
Add
columnarScanContexttoTiflashStatsand collect the stats fromtipb::ColumnarScanContext.Deps
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
New Features
Bug Fixes
Tests