Repository navigation
planner: use ExplainInfo/StringWithCtx for columns in EXPLAIN output paths - #67340
Conversation
…utput paths Five call sites in `EXPLAIN` rendering used `Column.String()` or `%v` to format columns, which always emits "Column#N" regardless of the explain format. Switch to `ExplainInfo(ctx)` or `StringWithCtx(ctx, redact)` so that "Column#N" is stripped when `format='plan_tree'`. - index_join_path.go: In `indexJoinPathRangeInfo`, use `StringWithCtx` to also respect `tidb_redact_log`, consistent with adjacent access condition rendering. - physical_plan_misc.go: In `RuntimeFilter.ExplainInfo`, add an `EvalContext` parameter, and use `ExplainInfo(ctx)` for source/target expressions. - physical_hash_join.go: Pass `evalCtx` to `RuntimeFilter.ExplainInfo`. - physical_table_scan.go: Pass `ectx` to `RuntimeFilter.ExplainInfo`; use `ExplainInfo` for ANN index distance projection column. - physical_window.go: Use `ExplainInfo` for window function output columns in `FormatWindowFuncDescs`. No functional changes for existing explain formats (brief, row, verbose) since `shouldRemoveColumnNumbers` only activates for plan_tree.
|
Review Complete Findings: 0 issues ℹ️ Learn more details on Pantheon AI. |
|
Hi @henrybw. 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 due to trivial changes (3)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR threads evaluation context into planner explain rendering: ExplainInfo/StringWithCtx now receive an EvalContext, and various explain outputs (runtime filters, window function columns, index-join range predicates) are rendered with context-aware formatting; many test expectations were updated to remove Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
|
/ok-to-test |
There was a problem hiding this comment.
Pull request overview
This PR standardizes how columns/expressions are formatted in planner EXPLAIN rendering paths by switching a few remaining call sites from Column.String() / %v to context-aware formatting (ExplainInfo(ctx) / StringWithCtx(ctx, redact)), which allows format='plan_tree' to omit synthetic Column#N suffixes.
Changes:
- Use
Column.ExplainInfo(ctx)for window output columns and ANN distance projection columns inEXPLAINstrings. - Update
RuntimeFilter.ExplainInfoto accept anexpression.EvalContextand render source/target expressions viaExplainInfo(ctx). - Ensure index-join range info uses
StringWithCtx(ectx, redact)so output respectstidb_redact_log.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/planner/core/operator/physicalop/physical_window.go | Render window function output columns via ExplainInfo(ctx) to support plan_tree formatting. |
| pkg/planner/core/operator/physicalop/physical_table_scan.go | Pass eval context into runtime filter explain; use ExplainInfo for ANN distance projection column. |
| pkg/planner/core/operator/physicalop/physical_plan_misc.go | Extend RuntimeFilter.ExplainInfo with eval context and use ExplainInfo(ctx) for columns. |
| pkg/planner/core/operator/physicalop/physical_hash_join.go | Pass eval context into runtime filter explain rendering. |
| pkg/planner/core/index_join_path.go | Use StringWithCtx(ectx, redact) for join key columns in range info, respecting redaction settings. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Hmm, seems like I missed some test cases that need updating. |
|
/hold |
a6c8c19 to
0d76105
Compare
0d76105 to
d2872a9
Compare
|
/unhold |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #67340 +/- ##
================================================
- Coverage 77.8320% 77.6315% -0.2005%
================================================
Files 2023 1943 -80
Lines 556131 543641 -12490
================================================
- Hits 432848 422037 -10811
- Misses 121539 121602 +63
+ Partials 1744 2 -1742
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
/retest-required |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 0xPoe, hawkingrei 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 |
[LGTM Timeline notifier]Timeline:
|
|
/test all |
|
/retest |
1 similar comment
|
/retest |
What problem does this PR solve?
Issue Number: ref #67112
Problem Summary:
Five call sites in
EXPLAINrendering usedColumn.String()or%vto format columns, which always emits "Column#N" regardless of the explain format. Switch toExplainInfo(ctx)orStringWithCtx(ctx, redact)so that "Column#N" is stripped whenformat='plan_tree'.What changed and how does it work?
indexJoinPathRangeInfo, useStringWithCtxto also respecttidb_redact_log, consistent with adjacent access condition rendering.RuntimeFilter.ExplainInfo, add anEvalContextparameter, and useExplainInfo(ctx)for source/target expressions.evalCtxtoRuntimeFilter.ExplainInfo.ectxtoRuntimeFilter.ExplainInfo; useExplainInfofor ANN index distance projection column.ExplainInfofor window function output columns inFormatWindowFuncDescs.format='plan_tree'output.No functional changes for existing explain formats (brief, row, verbose) since
shouldRemoveColumnNumbersonly activates for plan_tree.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