executor: fix RU v2 executor accounting#68578
Conversation
|
Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it. 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
Bot
added
the
do-not-merge/release-note-label-needed
|
@meiking I've received your pull request and will start the review. I'll conduct a thorough review covering code quality, potential issues, and implementation details. ⏳ This process typically takes 10-30 minutes depending on the complexity of the changes. ℹ️ Learn more details on Pantheon AI. |
ti-chi-bot
Bot
added
size/L
|
Hi @meiking. Thanks for your PR. I'm waiting for a pingcap member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
|
Hi @meiking. Thanks for your PR. PRs from untrusted users cannot be marked as trusted with I understand the commands that are listed here. 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. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughRecords RUv2 insert work as "rows × inserted-column-count", adds commit write shadow counters to RUv2, replaces legacy insert accounting with DML column-multiply helpers, wires recording into INSERT/UPDATE/DELETE/REPLACE, expands executor-type metric mappings, and updates tests and metric registration. ChangesRUv2 Insert Metrics Refactor: Column-Count Multiplication
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
🚥 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 Tools execution failed with the following error: Failed to run tools: 13 INTERNAL: Received RST_STREAM with code 2 (Internal server error) 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.
🧹 Nitpick comments (1)
pkg/executor/internal/exec/executor_test.go (1)
73-100: ⚡ Quick winExpand this mapping test to cover all newly added concrete keys.
This test catches key-drift regressions, but it currently validates only part of the updated map. Please include the remaining newly introduced entries (
ExpandExec,IndexReaderExecutor,MemTableReaderExec,SelectLockExec,TableReaderExecutor,UnionScanExec,WindowExec, etc.) so future renames fail fast.Proposed test expansion
func TestRUV2ExecutorMetricByTypeIncludesConcreteExecutorTypes(t *testing.T) { cases := map[string]ruv2ExecutorMetric{ + "*executor.ExpandExec": {level: 2, label: "ExpandExec", useCells: false}, + "*executor.IndexLookUpExecutor": {level: 2, label: "IndexLookUpExecutor", useCells: false}, + "*executor.IndexReaderExecutor": {level: 2, label: "IndexReaderExecutor", useCells: false}, + "*executor.MemTableReaderExec": {level: 2, label: "MemTableReaderExec", useCells: false}, "*executor.ProjectionExec": {level: 2, label: "ProjectionExec", useCells: true}, + "*executor.SelectLockExec": {level: 2, label: "SelectLockExec", useCells: true}, + "*executor.TableDualExec": {level: 2, label: "TableDualExec", useCells: false}, + "*executor.TableReaderExecutor": {level: 2, label: "TableReaderExecutor", useCells: false}, + "*executor.UnionScanExec": {level: 2, label: "UnionScanExec", useCells: false}, + "*executor.WindowExec": {level: 2, label: "WindowExec", useCells: false}, "*join.HashJoinV1Exec": {level: 2, label: "HashJoinV1Exec", useCells: false}, "*join.HashJoinV2Exec": {level: 2, label: "HashJoinV2Exec", useCells: false}, "*join.IndexLookUpJoin": {level: 2, label: "IndexLookUpJoin", useCells: true}, "*join.IndexLookUpMergeJoin": {level: 2, label: "IndexLookUpMergeJoin", useCells: true}, "*join.IndexNestedLoopHashJoin": {level: 2, label: "IndexNestedLoopHashJoin", useCells: true}, "*join.MergeJoinExec": {level: 2, label: "MergeJoinExec", useCells: false}, "*sortexec.SortExec": {level: 3, label: "SortExec", useCells: true}, "*sortexec.TopNExec": {level: 2, label: "TopNExec", useCells: true}, }🤖 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/internal/exec/executor_test.go` around lines 73 - 100, Update the TestRUV2ExecutorMetricByTypeIncludesConcreteExecutorTypes test to assert all newly added concrete keys exist and have correct ruv2ExecutorMetric values: locate the ruv2ExecutorMetricByType map and extend the cases map in the test (function TestRUV2ExecutorMetricByTypeIncludesConcreteExecutorTypes) to include entries for ExpandExec, IndexReaderExecutor, MemTableReaderExec, SelectLockExec, TableReaderExecutor, UnionScanExec, WindowExec (and any other new concrete type names added to the map), providing their expected level, label and useCells values, then run the same require.True/require.Equal checks as for existing entries so renames or missing keys fail the test.
🤖 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/internal/exec/executor_test.go`:
- Around line 73-100: Update the
TestRUV2ExecutorMetricByTypeIncludesConcreteExecutorTypes test to assert all
newly added concrete keys exist and have correct ruv2ExecutorMetric values:
locate the ruv2ExecutorMetricByType map and extend the cases map in the test
(function TestRUV2ExecutorMetricByTypeIncludesConcreteExecutorTypes) to include
entries for ExpandExec, IndexReaderExecutor, MemTableReaderExec, SelectLockExec,
TableReaderExecutor, UnionScanExec, WindowExec (and any other new concrete type
names added to the map), providing their expected level, label and useCells
values, then run the same require.True/require.Equal checks as for existing
entries so renames or missing keys fail the test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 028bf31e-7c5b-4610-9ea5-ceb0b06c2cf9
📒 Files selected for processing (13)
pkg/config/config.gopkg/config/config.toml.examplepkg/executor/adapter.gopkg/executor/adapter_test.gopkg/executor/explain.gopkg/executor/explain_unit_test.gopkg/executor/insert.gopkg/executor/insert_common.gopkg/executor/internal/exec/BUILD.bazelpkg/executor/internal/exec/executor.gopkg/executor/internal/exec/executor_test.gopkg/sessionctx/stmtctx/stmtctx.gopkg/util/execdetails/ruv2_metrics.go
💤 Files with no reviewable changes (2)
- pkg/sessionctx/stmtctx/stmtctx.go
- pkg/executor/explain.go
|
/ok-to-test |
ti-chi-bot
Bot
added
ok-to-test
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #68578 +/- ##
================================================
- Coverage 76.3752% 75.1595% -1.2158%
================================================
Files 2039 2023 -16
Lines 562884 569949 +7065
================================================
- Hits 429904 428371 -1533
- Misses 132062 141372 +9310
+ Partials 918 206 -712
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
ti-chi-bot
Bot
added
size/XL
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/metrics/ru_v2.go (1)
47-147:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winClarify the exported help for
executor_l5_insert_rows.After this PR, update/delete/replace paths also feed this counter, but the
/metricshelp still says “insert rows”. Please update the help text to describe the legacy name and the actual rows×columns semantics so operators do not misread it.🤖 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/metrics/ru_v2.go` around lines 47 - 147, The exported help for RUV2ExecutorL5InsertRows in InitRUV2Metrics is misleadingly "insert rows" even though updates/deletes also increment it and it actually measures rows×columns; update the prometheus.CounterOpts.Help for RUV2ExecutorL5InsertRows to mention the legacy name and the true semantics (e.g. "Legacy 'insert rows' name; counts affected rows × columns; includes inserts, updates, deletes") so operators understand what the metric represents.
🤖 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/update.go`:
- Around line 373-386: The metric currently multiplies
sum(content.End-content.Start) by numRows in recordRowsColMultiply2RUV2Metrics,
which counts columns from all table slices even when a particular row's slice is
not updatable; update the counting to only include columns from slices that were
marked updatable during row processing. In practice, in updateRows (after
calling e.prepare(datumRow) or wherever per-row updatable state is known)
accumulate rowsColMultiply by summing int64(content.End-content.Start) only for
indices where e.tableUpdatable[i] is true (using e.tblColPosInfos to get
Start/End), then pass that per-row-aware rowsColMultiply into
recordRowsColMultiply2RUV2Metrics (or adjust recordRowsColMultiply2RUV2Metrics
to accept the precomputed columnCount) so RU charge reflects only
actually-updatable columns.
---
Outside diff comments:
In `@pkg/metrics/ru_v2.go`:
- Around line 47-147: The exported help for RUV2ExecutorL5InsertRows in
InitRUV2Metrics is misleadingly "insert rows" even though updates/deletes also
increment it and it actually measures rows×columns; update the
prometheus.CounterOpts.Help for RUV2ExecutorL5InsertRows to mention the legacy
name and the true semantics (e.g. "Legacy 'insert rows' name; counts affected
rows × columns; includes inserts, updates, deletes") so operators understand
what the metric represents.
🪄 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: 549b6964-51b9-4ee5-bea7-e284541fd0a5
📒 Files selected for processing (9)
pkg/executor/adapter.gopkg/executor/adapter_test.gopkg/executor/delete.gopkg/executor/replace.gopkg/executor/update.gopkg/metrics/metrics.gopkg/metrics/ru_v2.gopkg/util/execdetails/execdetails_test.gopkg/util/execdetails/ruv2_metrics.go
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/update.go`:
- Around line 379-389: The RU overcount happens because
rowsColMultiplyForPreparedRow() sums column ranges for indices marked in
e.tableUpdatable without excluding rows already skipped via e.changed; update
rowsColMultiplyForPreparedRow() to also check the corresponding e.changed entry
(ensure i < len(e.changed)) and skip counting when e.changed[i] is true so only
actually-updated rows contribute to the rows×columns tally for UpdateExec.
🪄 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: 6eb4e3ce-3294-4778-9bcd-bccc2f408d84
📒 Files selected for processing (2)
pkg/executor/adapter_test.gopkg/executor/update.go
|
/retest-required |
|
[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 |
|
/retest-required |
1 similar comment
|
/retest-required |
2 participants
What problem does this PR solve?
Issue Number: ref #68576
Problem Summary:
RU v2 executor accounting may undercount TiDB-side compute cost for some common executors because the executor-level mapping relies on concrete type strings, and some entries no longer match the current executor package/type names. In addition, the insert-specific L5 metric should account for inserted rows multiplied by inserted column count, but the old path used affected rows, which does not match the fitted
executor-l5-insert-rowsmodel input.What changed and how does it work?
InsertValuesasrowCount * len(insertColumns)after successful insert execution, including batched insert and insert-select paths.executor-l5-insert-rowsconfig/metric comments to describe row-column multiplication.pkg/executor/internal/exectests.Check List
Tests
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.