[for binary building]#69977
Open
winoros wants to merge 25 commits into
Open
Conversation
winoros
added
the
do-not-merge/hold
ti-chi-bot
Bot
added
do-not-merge/invalid-title
do-not-merge/needs-linked-issue
do-not-merge/needs-tests-checked
release-note-none
|
[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 |
ti-chi-bot
Bot
added
size/XXL
|
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:
📝 WalkthroughWalkthroughThe PR adds preview RU explain and read-billing collection, including v4 formula evaluation, runtime evidence, mutation tracking, bounded statement-summary storage, INFORMATION_SCHEMA readers, metrics, general-log completion records, and extensive planner, executor, session, and storage tests. ChangesPreview RU model and execution flow
Statement summaries and observability
Runtime-stat correctness
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✨ 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 |
![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/session/session.go (1)
2473-2476: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueUse
types.ExplainFormatRUhere instead of repeating"ru".
StmtCtx.ExplainFormatis already lowercased, so this stays semantically identical and removes the literal duplication.🤖 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/session/session.go` around lines 2473 - 2476, Update the ExplainFormat comparison in the previewKVMutationEnabled expression to use the existing types.ExplainFormatRU constant instead of the literal "ru"; preserve the current case-insensitive comparison and all surrounding conditions.
🤖 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/stmtsummary/statement_summary.go`:
- Around line 769-780: Update initReadBillingDemoStatusOnly to initialize
ssbd.isInternal from sei.IsInternal before the status-only summary aggregation
runs, preserving correct classification for the first internal statement while
leaving subsequent AND-based updates unchanged.
---
Nitpick comments:
In `@pkg/session/session.go`:
- Around line 2473-2476: Update the ExplainFormat comparison in the
previewKVMutationEnabled expression to use the existing types.ExplainFormatRU
constant instead of the literal "ru"; preserve the current case-insensitive
comparison and all surrounding conditions.
🪄 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: 08a31ecd-3764-46fd-bc46-6fb3087feff9
📒 Files selected for processing (77)
.agents/skills/tidb-test-guidelines/references/distsql-case-map.md.agents/skills/tidb-test-guidelines/references/executor-case-map.md.agents/skills/tidb-test-guidelines/references/metrics-case-map.md.agents/skills/tidb-test-guidelines/references/planner-case-map.md.agents/skills/tidb-test-guidelines/references/session-case-map.md.agents/skills/tidb-test-guidelines/references/util-case-map.mddocs/design/2026-07-01-read-billing-demo-ru-model.mddocs/design/2026-07-08-write-billing-units-weights-design.mddocs/design/2026-07-10-preview-ru-tidb-kv-mutation.mddocs/design/2026-07-22-preview-ru-resource-formula-plan.mdpkg/distsql/BUILD.bazelpkg/distsql/select_result.gopkg/distsql/select_result_test.gopkg/executor/BUILD.bazelpkg/executor/adapter.gopkg/executor/adapter_internal_test.gopkg/executor/adapter_test.gopkg/executor/builder.gopkg/executor/compiler.gopkg/executor/explain_test.gopkg/executor/explainfor_test.gopkg/executor/internal/exec/BUILD.bazelpkg/executor/internal/exec/executor.gopkg/executor/internal/exec/executor_test.gopkg/executor/join/hash_join_stats.gopkg/executor/join/hash_join_v1.gopkg/executor/join/hash_join_v2.gopkg/executor/join/hash_table_v1.gopkg/executor/join/hash_table_v1_test.gopkg/executor/join/join_stats_test.gopkg/executor/stmtsummary.gopkg/infoschema/cluster.gopkg/infoschema/tables.gopkg/metrics/BUILD.bazelpkg/metrics/explain_ru.gopkg/metrics/metrics.gopkg/metrics/metrics_internal_test.gopkg/metrics/ru_v2.gopkg/planner/core/BUILD.bazelpkg/planner/core/common_plans.gopkg/planner/core/common_plans_test.gopkg/planner/core/explain_ru.gopkg/planner/core/logical_plan_builder.gopkg/planner/core/operator/logicalop/logical_mem_table.gopkg/planner/core/pb_to_plan.gopkg/planner/core/planbuilder.gopkg/session/BUILD.bazelpkg/session/session.gopkg/session/tidb_test.gopkg/session/txn.gopkg/sessionctx/stmtctx/stmtctx.gopkg/sessionctx/vardef/tidb_vars.gopkg/sessionctx/variable/session.gopkg/sessionctx/variable/sysvar.gopkg/types/explain_format.gopkg/util/chunk/chunk.gopkg/util/chunk/chunk_test.gopkg/util/execdetails/execdetails_test.gopkg/util/execdetails/runtime_stats.gopkg/util/execdetails/ruv2_metrics.gopkg/util/stmtsummary/BUILD.bazelpkg/util/stmtsummary/evicted.gopkg/util/stmtsummary/read_billing.gopkg/util/stmtsummary/read_billing_test.gopkg/util/stmtsummary/reader.gopkg/util/stmtsummary/statement_summary.gopkg/util/stmtsummary/statement_summary_test.gopkg/util/stmtsummary/v2/BUILD.bazelpkg/util/stmtsummary/v2/column.gopkg/util/stmtsummary/v2/column_test.gopkg/util/stmtsummary/v2/logger.gopkg/util/stmtsummary/v2/reader.gopkg/util/stmtsummary/v2/reader_test.gopkg/util/stmtsummary/v2/record.gopkg/util/stmtsummary/v2/record_test.gopkg/util/stmtsummary/v2/stmtsummary.gopkg/util/stmtsummary/v2/stmtsummary_test.go
Comment on lines
+769
to
+780
| func (ssbd *stmtSummaryByDigest) initReadBillingDemoStatusOnly(sei *StmtExecInfo) { | ||
| ssbd.cumulative = *newReadBillingDemoStatusOnlyStats(sei) | ||
| ssbd.schemaName = sei.SchemaName | ||
| ssbd.digest = sei.Digest | ||
| ssbd.planDigest = sei.PlanDigest | ||
| if sei.StmtCtx != nil { | ||
| ssbd.stmtType = sei.StmtCtx.StmtType | ||
| } | ||
| ssbd.normalizedSQL = formatSQL(sei.NormalizedSQL) | ||
| ssbd.history = list.New() | ||
| ssbd.initialized = true | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP 'func \(ssbd \*stmtSummaryByDigest\) init\(' -A 25 pkg/util/stmtsummary/statement_summary.go | rg -n 'isInternal'Repository: pingcap/tidb
Length of output: 150
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE='pkg/util/stmtsummary/statement_summary.go'
# Map the relevant methods first.
ast-grep outline "$FILE" --view expanded | sed -n '1,220p'
echo '--- initReadBillingDemoStatusOnly ---'
rg -n 'initReadBillingDemoStatusOnly|func \(ssbd \*stmtSummaryByDigest\) init\(' -A 20 -B 5 "$FILE"
echo '--- AddReadBillingDemoStatusOnly path ---'
rg -n 'AddReadBillingDemoStatusOnly|addReadBillingDemoStatusOnly|isInternal' -A 20 -B 10 "$FILE"Repository: pingcap/tidb
Length of output: 20816
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE='pkg/util/stmtsummary/statement_summary.go'
# Read the small relevant slices directly with line numbers.
sed -n '430,520p' "$FILE" | cat -n
echo '---'
sed -n '740,790p' "$FILE" | cat -n
echo '---'
sed -n '1160,1210p' "$FILE" | cat -nRepository: pingcap/tidb
Length of output: 7553
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE='pkg/util/stmtsummary/statement_summary.go'
# Show the normal initializer and the special initializer side by side.
rg -n 'func \(ssbd \*stmtSummaryByDigest\) init\(' -A 40 -B 5 "$FILE" || true
echo '---'
rg -n 'func \(ssbd \*stmtSummaryByDigest\) initReadBillingDemoStatusOnly\(' -A 25 -B 5 "$FILE" || true
echo '---'
rg -n 'summary\.isInternal =|ssbd\.isInternal =|IsInternal' -A 5 -B 5 "$FILE" || trueRepository: pingcap/tidb
Length of output: 4840
initReadBillingDemoStatusOnly should copy isInternal.
The status-only path does summary.isInternal = summary.isInternal && sei.IsInternal, so a newly allocated summary stays false unless this initializer sets it first. That misclassifies the first internal statement recorded here.
🤖 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/stmtsummary/statement_summary.go` around lines 769 - 780, Update
initReadBillingDemoStatusOnly to initialize ssbd.isInternal from sei.IsInternal
before the status-only summary aggregation runs, preserving correct
classification for the first internal statement while leaving subsequent
AND-based updates unchanged.
winoros
force-pushed
the
preview-ru-v4
branch
2 times, most recently
from
July 23, 2026 16:37
2d092b3 to
a1b928b
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@docs/design/2026-07-22-preview-ru-resource-formula-plan.md`:
- Line 27: Update the Milestone 1 progress entry to say “seven-weight” instead
of “six-weight,” preserving the listed semantic weights and all other milestone
details.
In `@pkg/executor/join/hash_join_v2.go`:
- Around line 96-112: The hashStateRows method currently saturates overflow to
math.MaxUint64, allowing an invalid count to be recorded by the hash_state_rows
path around the caller at lines 1347-1349. Change hashStateRows and its caller
to propagate an overflow/failure status, suppress hash_state_rows when overflow
occurs, and record only the existing bounded invalid reason instead of billing
the saturated value.
In `@pkg/session/tidb_test.go`:
- Around line 420-429: Update the test around dmlMetrics and commitMetrics to
use a test-only resource-manager write-count injection path that modifies
statement snapshots without emitting the global Prometheus metric; if
unavailable, isolate and restore the process-wide counter using existing test
infrastructure. Preserve the assertions verifying independent snapshots and the
expected counts.
🪄 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: 01576011-b7f3-493d-ba5d-463784474f78
📒 Files selected for processing (31)
.agents/skills/tidb-test-guidelines/references/distsql-case-map.md.agents/skills/tidb-test-guidelines/references/executor-case-map.md.agents/skills/tidb-test-guidelines/references/metrics-case-map.md.agents/skills/tidb-test-guidelines/references/planner-case-map.md.agents/skills/tidb-test-guidelines/references/session-case-map.md.agents/skills/tidb-test-guidelines/references/util-case-map.mddocs/design/2026-07-01-read-billing-demo-ru-model.mddocs/design/2026-07-10-preview-ru-tidb-kv-mutation.mddocs/design/2026-07-22-preview-ru-resource-formula-plan.mdpkg/distsql/select_result_test.gopkg/executor/adapter.gopkg/executor/adapter_internal_test.gopkg/executor/explain_test.gopkg/executor/join/hash_join_stats.gopkg/executor/join/hash_join_v1.gopkg/executor/join/hash_join_v2.gopkg/executor/join/hash_table_v1.gopkg/executor/join/hash_table_v1_test.gopkg/executor/join/join_stats_test.gopkg/infoschema/tables.gopkg/metrics/explain_ru.gopkg/metrics/metrics_internal_test.gopkg/planner/core/BUILD.bazelpkg/planner/core/common_plans_test.gopkg/planner/core/explain_ru.gopkg/session/tidb_test.gopkg/util/execdetails/execdetails_test.gopkg/util/execdetails/runtime_stats.gopkg/util/stmtsummary/read_billing.gopkg/util/stmtsummary/read_billing_test.gopkg/util/stmtsummary/v2/record_test.go
🚧 Files skipped from review as they are similar to previous changes (23)
- .agents/skills/tidb-test-guidelines/references/session-case-map.md
- pkg/executor/join/hash_table_v1.go
- pkg/executor/join/hash_table_v1_test.go
- pkg/executor/join/join_stats_test.go
- .agents/skills/tidb-test-guidelines/references/util-case-map.md
- docs/design/2026-07-10-preview-ru-tidb-kv-mutation.md
- .agents/skills/tidb-test-guidelines/references/executor-case-map.md
- pkg/infoschema/tables.go
- pkg/metrics/metrics_internal_test.go
- pkg/util/stmtsummary/v2/record_test.go
- pkg/executor/join/hash_join_stats.go
- pkg/util/stmtsummary/read_billing_test.go
- pkg/executor/join/hash_join_v1.go
- pkg/distsql/select_result_test.go
- pkg/metrics/explain_ru.go
- docs/design/2026-07-01-read-billing-demo-ru-model.md
- pkg/executor/adapter_internal_test.go
- pkg/util/execdetails/runtime_stats.go
- pkg/util/stmtsummary/read_billing.go
- pkg/executor/explain_test.go
- pkg/planner/core/common_plans_test.go
- pkg/executor/adapter.go
- pkg/planner/core/explain_ru.go
Comment on lines
+96
to
+112
| func (htc *hashTableContext) hashStateRows() uint64 { | ||
| var rows uint64 | ||
| if htc == nil || htc.hashTable == nil { | ||
| return 0 | ||
| } | ||
| for _, table := range htc.hashTable.tables { | ||
| if table != nil && table.rowData != nil { | ||
| validRows := table.rowData.validKeyCount() | ||
| if math.MaxUint64-rows < validRows { | ||
| return math.MaxUint64 | ||
| } | ||
| rows += validRows | ||
| } | ||
| } | ||
| return rows | ||
| } | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject hash-state overflow instead of saturating it.
The v4 contract requires overflow rejection/fail-closed behavior, but this returns math.MaxUint64 and Line 1348 records it as a valid hash-state count. Propagate an overflow status so hash_state_rows is suppressed with a bounded invalid reason rather than billing an invalid saturated value.
Also applies to: 1347-1349
🤖 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/join/hash_join_v2.go` around lines 96 - 112, The hashStateRows
method currently saturates overflow to math.MaxUint64, allowing an invalid count
to be recorded by the hash_state_rows path around the caller at lines 1347-1349.
Change hashStateRows and its caller to propagate an overflow/failure status,
suppress hash_state_rows when overflow occurs, and record only the existing
bounded invalid reason instead of billing the saturated value.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@docs/design/2026-07-22-preview-ru-resource-formula-plan.md`:
- Around line 30-35: Normalize all progress-audit timestamps to a single
timezone, preferably UTC, and reorder the checklist entries chronologically by
their actual instants. Preserve the existing audit details and completion
statuses while ensuring each subsequent entry is later than the preceding one.
In `@pkg/executor/adapter_test.go`:
- Around line 745-774: Strengthen the assertions in the point-lookup
completion-entry test around rawUnits so every unit has op_class equal to
"kv_point_lookup", rather than skipping other classes. Preserve the existing
field and count validations, ensuring the test fails when any unrelated
operation class is emitted.
🪄 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: 0e77f174-1a3c-45d1-a9f1-cad04b773f18
📒 Files selected for processing (32)
.agents/skills/tidb-test-guidelines/references/distsql-case-map.md.agents/skills/tidb-test-guidelines/references/executor-case-map.md.agents/skills/tidb-test-guidelines/references/metrics-case-map.md.agents/skills/tidb-test-guidelines/references/planner-case-map.md.agents/skills/tidb-test-guidelines/references/session-case-map.md.agents/skills/tidb-test-guidelines/references/util-case-map.mddocs/design/2026-07-01-read-billing-demo-ru-model.mddocs/design/2026-07-10-preview-ru-tidb-kv-mutation.mddocs/design/2026-07-22-preview-ru-resource-formula-plan.mdpkg/distsql/select_result_test.gopkg/executor/adapter.gopkg/executor/adapter_internal_test.gopkg/executor/adapter_test.gopkg/executor/explain_test.gopkg/executor/join/hash_join_stats.gopkg/executor/join/hash_join_v1.gopkg/executor/join/hash_join_v2.gopkg/executor/join/hash_table_v1.gopkg/executor/join/hash_table_v1_test.gopkg/executor/join/join_stats_test.gopkg/infoschema/tables.gopkg/metrics/explain_ru.gopkg/metrics/metrics_internal_test.gopkg/planner/core/BUILD.bazelpkg/planner/core/common_plans_test.gopkg/planner/core/explain_ru.gopkg/session/tidb_test.gopkg/util/execdetails/execdetails_test.gopkg/util/execdetails/runtime_stats.gopkg/util/stmtsummary/read_billing.gopkg/util/stmtsummary/read_billing_test.gopkg/util/stmtsummary/v2/record_test.go
🚧 Files skipped from review as they are similar to previous changes (27)
- .agents/skills/tidb-test-guidelines/references/metrics-case-map.md
- .agents/skills/tidb-test-guidelines/references/session-case-map.md
- pkg/executor/join/hash_table_v1.go
- .agents/skills/tidb-test-guidelines/references/planner-case-map.md
- pkg/executor/join/hash_table_v1_test.go
- .agents/skills/tidb-test-guidelines/references/distsql-case-map.md
- pkg/util/stmtsummary/read_billing_test.go
- pkg/executor/join/hash_join_v1.go
- pkg/executor/join/hash_join_v2.go
- pkg/planner/core/BUILD.bazel
- pkg/util/stmtsummary/v2/record_test.go
- pkg/metrics/explain_ru.go
- pkg/executor/adapter_internal_test.go
- .agents/skills/tidb-test-guidelines/references/util-case-map.md
- docs/design/2026-07-10-preview-ru-tidb-kv-mutation.md
- pkg/util/execdetails/execdetails_test.go
- pkg/infoschema/tables.go
- pkg/metrics/metrics_internal_test.go
- pkg/executor/join/hash_join_stats.go
- pkg/session/tidb_test.go
- docs/design/2026-07-01-read-billing-demo-ru-model.md
- pkg/executor/adapter.go
- pkg/distsql/select_result_test.go
- pkg/util/stmtsummary/read_billing.go
- pkg/executor/explain_test.go
- pkg/planner/core/explain_ru.go
- pkg/planner/core/common_plans_test.go
Comment on lines
+30
to
+35
| - [x] (2026-07-22 20:55Z) Completed the implementation Ready profile: `make bazel_prepare`, all six prescribed failpoint-wrapped package test sets, the tagged metrics test, `make lint`, and `git diff --check` passed; the HashJoin set additionally ran `TestHashRowContainer` for V1 NAAJ state coverage. | ||
| - [x] (2026-07-22 22:10Z) A completion audit found and fixed a v4 migration regression: the write constructor had stopped consulting the existing statement mutation snapshot's `Pipelined` flag. Pipelined DML and COMMIT now retain mutation evidence but fail the write-request component closed with `pipelined_tikv_payload_unsupported`; focused DML and COMMIT regressions cover the restored gate. | ||
| - [x] (2026-07-22 04:25+08:00) The final completion audit found that the ordering gate rejected residual scalar functions but did not prove that column `ByItems` belonged to the direct child schema. Sort/TopN now validate the executed flat child, including Projection expression/schema alignment, and a regression was observed failing before the fix and passing afterward. | ||
| - [x] (2026-07-22 04:30+08:00) Recompleted the Ready gate after the ordering fix: the full planner/core and executor target sets passed with failpoint cleanup, the earlier same-state join/session/statement-summary/metrics runs remained applicable, `make lint` passed after the code edit, and `git diff --check` plus API/range review found no remaining mismatch. | ||
| - [x] (2026-07-22 05:35+08:00) A requirement-by-requirement completion audit found that the private weight container omitted its specified immutable version, so a test-only calibrated set could publish a total under the production uncalibrated label. Added version ownership and validation to the weight container, propagated its active version to every output, and observed the focused regression fail before the fix and pass afterward. | ||
| - [x] (2026-07-22 05:45+08:00) Recompleted the full Ready gate from the current worktree: `make bazel_prepare` produced no generated diff; all six failpoint-wrapped target groups, the HashJoin NAAJ coverage, and tagged metrics tests passed; `make lint`, `git diff --check`, API-boundary searches, and the baseline-to-HEAD scope review passed. Failpoint refcount returned to zero after every run. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Keep the progress audit chronological.
These entries mix Z and +08:00; for example, 2026-07-22 04:25+08:00 is earlier than the preceding 2026-07-22 20:55Z. Normalize the timestamps or reorder the entries so the audit trail remains trustworthy.
🤖 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 `@docs/design/2026-07-22-preview-ru-resource-formula-plan.md` around lines 30 -
35, Normalize all progress-audit timestamps to a single timezone, preferably
UTC, and reorder the checklist entries chronologically by their actual instants.
Preserve the existing audit details and completion statuses while ensuring each
subsequent entry is later than the preceding one.
Comment on lines
+745
to
+774
| entries := recorded.TakeAll() | ||
| var completionEntries []observer.LoggedEntry | ||
| for _, entry := range entries { | ||
| if entry.Message == "GENERAL_LOG_RU_UNITS" { | ||
| completionEntries = append(completionEntries, entry) | ||
| } | ||
| } | ||
| require.Len(t, completionEntries, 1) | ||
| requireReadBillingDemoGeneralLogIdentity(t, completionEntries[0], tc.querySQL) | ||
| rawUnits, ok := completionEntries[0].ContextMap()["units"].([]any) | ||
| require.True(t, ok) | ||
| pointUnits := 0 | ||
| for _, rawUnit := range rawUnits { | ||
| unit, ok := rawUnit.(map[string]any) | ||
| require.True(t, ok) | ||
| if unit["op_class"] != "kv_point_lookup" { | ||
| continue | ||
| } | ||
| pointUnits++ | ||
| require.Equal(t, "tikv", unit["site"]) | ||
| require.Equal(t, tc.operatorKind, unit["operator_kind"]) | ||
| require.Equal(t, "read_request_count", unit["unit"]) | ||
| require.Equal(t, "ruv2_metrics", unit["input_source"]) | ||
| require.Equal(t, "all", unit["input_side"]) | ||
| require.Equal(t, float64(tc.requests), unit["value"]) | ||
| } | ||
| require.Equal(t, 1, pointUnits) | ||
| }) | ||
| } | ||
| }) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Test doesn't actually verify "point lookup rpc only".
The loop at lines 757-770 skips (continue) any unit whose op_class != "kv_point_lookup" and only checks pointUnits == 1 (line 771). It never asserts that rawUnits contains only point-lookup units, so a regression that emits extra, unrelated op_class entries for a point-get query would pass silently, despite the test name promising "point lookup rpc only".
🧪 Proposed fix to enforce exclusivity
require.Equal(t, 1, pointUnits)
+ require.Len(t, rawUnits, pointUnits, "expected units to contain only kv_point_lookup entries")📝 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.
Suggested change
| entries := recorded.TakeAll() | |
| var completionEntries []observer.LoggedEntry | |
| for _, entry := range entries { | |
| if entry.Message == "GENERAL_LOG_RU_UNITS" { | |
| completionEntries = append(completionEntries, entry) | |
| } | |
| } | |
| require.Len(t, completionEntries, 1) | |
| requireReadBillingDemoGeneralLogIdentity(t, completionEntries[0], tc.querySQL) | |
| rawUnits, ok := completionEntries[0].ContextMap()["units"].([]any) | |
| require.True(t, ok) | |
| pointUnits := 0 | |
| for _, rawUnit := range rawUnits { | |
| unit, ok := rawUnit.(map[string]any) | |
| require.True(t, ok) | |
| if unit["op_class"] != "kv_point_lookup" { | |
| continue | |
| } | |
| pointUnits++ | |
| require.Equal(t, "tikv", unit["site"]) | |
| require.Equal(t, tc.operatorKind, unit["operator_kind"]) | |
| require.Equal(t, "read_request_count", unit["unit"]) | |
| require.Equal(t, "ruv2_metrics", unit["input_source"]) | |
| require.Equal(t, "all", unit["input_side"]) | |
| require.Equal(t, float64(tc.requests), unit["value"]) | |
| } | |
| require.Equal(t, 1, pointUnits) | |
| }) | |
| } | |
| }) | |
| entries := recorded.TakeAll() | |
| var completionEntries []observer.LoggedEntry | |
| for _, entry := range entries { | |
| if entry.Message == "GENERAL_LOG_RU_UNITS" { | |
| completionEntries = append(completionEntries, entry) | |
| } | |
| } | |
| require.Len(t, completionEntries, 1) | |
| requireReadBillingDemoGeneralLogIdentity(t, completionEntries[0], tc.querySQL) | |
| rawUnits, ok := completionEntries[0].ContextMap()["units"].([]any) | |
| require.True(t, ok) | |
| pointUnits := 0 | |
| for _, rawUnit := range rawUnits { | |
| unit, ok := rawUnit.(map[string]any) | |
| require.True(t, ok) | |
| if unit["op_class"] != "kv_point_lookup" { | |
| continue | |
| } | |
| pointUnits++ | |
| require.Equal(t, "tikv", unit["site"]) | |
| require.Equal(t, tc.operatorKind, unit["operator_kind"]) | |
| require.Equal(t, "read_request_count", unit["unit"]) | |
| require.Equal(t, "ruv2_metrics", unit["input_source"]) | |
| require.Equal(t, "all", unit["input_side"]) | |
| require.Equal(t, float64(tc.requests), unit["value"]) | |
| } | |
| require.Equal(t, 1, pointUnits) | |
| require.Len(t, rawUnits, pointUnits, "expected units to contain only kv_point_lookup entries") | |
| }) | |
| } | |
| }) |
🤖 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/adapter_test.go` around lines 745 - 774, Strengthen the
assertions in the point-lookup completion-entry test around rawUnits so every
unit has op_class equal to "kv_point_lookup", rather than skipping other
classes. Preserve the existing field and count validations, ensuring the test
fails when any unrelated operation class is emitted.
|
[FORMAT CHECKER NOTIFICATION] Notice: To remove the The title description (the part after 📖 For more info, you can check the "Contribute Code" section in the development guide. Notice: To remove the 📖 For more info, you can check the "Contribute Code" section in the development guide. Notice: To remove the For example:
📖 For more info, you can check the "Contribute Code" section in the development guide. |
|
@winoros: 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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Issue Number: close #xxx
Problem Summary:
What changed and how does it work?
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
EXPLAIN ANALYZE FORMAT='RU'preview resource-unit reporting, including RU demo modeling and status-only diagnostics.tidb_enable_read_billing_demo) with new statement-summaryINFORMATION_SCHEMAtables (base-units + status, current + history), plus RU demo general-log and Prometheus observability.