util: add support columnar scan details#69331
Conversation
Signed-off-by: Ray Yan <yming0221@gmail.com>
|
Skipping CI for Draft Pull Request. |
ti-chi-bot
Bot
added
release-note-none
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe PR bumps the ChangesTiFlash Columnar Scan Context Stats
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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)
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
Bot
added
size/L
ti-chi-bot
Bot
removed
the
do-not-merge/work-in-progress
![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
🧹 Nitpick comments (1)
pkg/util/execdetails/execdetails_test.go (1)
754-783: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd one assertion for
s.stats.String()to lock both TiFlash string paths.This test currently covers
CopRuntimeStats.String()only; adding thebasicCopRuntimeStats.String()assertion here will catch columnar/reporting divergence earlier.Suggested test addition
diff --git a/pkg/util/execdetails/execdetails_test.go b/pkg/util/execdetails/execdetails_test.go @@ stats.RecordOneCopTask(1, kv.TiFlash, execSummary) s := stats.GetCopStats(1) require.Equal(t, "tiflash_task:{time:1ns, loops:2, threads:1}, columnar_scan:{mvcc_input_rows:100, mvcc_input_bytes:4096, mvcc_output_rows:80, regions:2, read_tasks:4, physical_tables:3, columns:5, user_read_bytes:2048, read_block:7ms, serialize_block:8ms, init_reader:9ms, prefetch:10ms, deserialize_block:17ms, rough_check:{total:11, selected:12, skipped:13, unknown:14}, remote_segments:15, total_segments:16}", s.String()) + require.Contains(t, s.stats.String(), "columnar_scan:{") + require.NotContains(t, s.stats.String(), "tiflash_scan:{")🤖 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/execdetails_test.go` around lines 754 - 783, The test function TestColumnarScanContextStats currently only validates the String() output of the CopRuntimeStats object returned by GetCopStats, but does not test the underlying basicCopRuntimeStats.String() representation. Add an additional assertion after the first require.Equal call that validates s.stats.String() to ensure both the columnar and basic reporting string paths are tested. This will help catch any divergence between these two representations earlier in testing.
🤖 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/util/execdetails/runtime_stats.go`:
- Around line 170-175: The basicCopRuntimeStats.String method unconditionally
renders legacy scan stats via scanContext.String() even when columnarScanContext
is populated, which hides columnar metrics. Update the String method to check if
columnarScanContext exists and is non-nil, and if so, render
columnarScanContext.String() instead of scanContext.String(). This ensures
consistency with how CopRuntimeStats.String handles this scenario and prevents
columnar metrics from being hidden behind legacy scan stats.
---
Nitpick comments:
In `@pkg/util/execdetails/execdetails_test.go`:
- Around line 754-783: The test function TestColumnarScanContextStats currently
only validates the String() output of the CopRuntimeStats object returned by
GetCopStats, but does not test the underlying basicCopRuntimeStats.String()
representation. Add an additional assertion after the first require.Equal call
that validates s.stats.String() to ensure both the columnar and basic reporting
string paths are tested. This will help catch any divergence between these two
representations earlier in testing.
🪄 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: e9f7ae63-adeb-4707-b7f2-b9288df5a930
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (7)
DEPS.bzlbr/pkg/task/BUILD.bazelbr/pkg/task/operator/BUILD.bazelgo.modpkg/util/execdetails/execdetails_test.gopkg/util/execdetails/runtime_stats.gopkg/util/execdetails/tiflash_stats.go
Signed-off-by: Ray Yan <yming0221@gmail.com>
|
/retest |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #69331 +/- ##
================================================
- Coverage 76.3197% 74.1166% -2.2031%
================================================
Files 2041 2040 -1
Lines 561547 580049 +18502
================================================
+ Hits 428571 429913 +1342
- Misses 132072 149677 +17605
+ Partials 904 459 -445
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Signed-off-by: Ray Yan <yming0221@gmail.com>
ti-chi-bot
Bot
added
size/XL
|
@JaySon-Huang: adding LGTM is restricted to approvers and reviewers in OWNERS files. 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. |
|
/retest-required |
ti-chi-bot
Bot
added
lgtm
and removed
needs-1-more-lgtm
|
/assign @bb7133 |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 3pointer, bb7133, coocood, JaySon-Huang 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 |
|
/check-cla |
|
/cherry-pick release-nextgen-202603 |
|
@yongman: new pull request created to branch 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 ti-community-infra/tichi repository. |
7 participants
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
Chores
Tests