This is an automated email from the ASF dual-hosted git repository.
englefly pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/doris.git
The following commit(s) were added to refs/heads/master by this push:
new 4c616855c0e [doc](fe) Clarify optimizer review output style (#64490)
4c616855c0e is described below
commit 4c616855c0e51bf4f0af45c19b940ad6d4e59ddc
Author: minghong <[email protected]>
AuthorDate: Mon Jun 15 10:16:55 2026 +0800
[doc](fe) Clarify optimizer review output style (#64490)
### What problem does this PR solve?
Issue Number: close #xxx
Related PR: #xxx
Problem Summary: Optimizer review comments can be hard to understand
when they describe plan rewrite bugs only in prose. This change updates
the code-review skill to ask reviewers to explain Nereids optimizer
findings with a minimal plan tree, rewrite steps, the incorrect
rewritten shape or expression, and the semantic difference before giving
the fix direction.
### Release note
None
### Check List (For Author)
- Test: No need to test (documentation-only skill update)
- Behavior changed: No
- Does this need documentation: No
### What problem does this PR solve?
Issue Number: close #xxx
Related PR: #xxx
Problem Summary:
### Release note
None
### Check List (For Author)
- Test <!-- At least one of them must be included. -->
- [ ] Regression test
- [ ] Unit Test
- [ ] Manual test (add detailed scripts or steps below)
- [ ] No need to test or manual test. Explain why:
- [ ] This is a refactor/code format and no logic has been changed.
- [ ] Previous test can cover this change.
- [ ] No code files have been changed.
- [ ] Other reason <!-- Add your reason? -->
- Behavior changed:
- [ ] No.
- [ ] Yes. <!-- Explain the behavior change -->
- Does this need documentation?
- [ ] No.
- [ ] Yes. <!-- Add document PR link here. eg:
https://github.com/apache/doris-website/pull/1214 -->
### Check List (For Reviewer who merge this PR)
- [ ] Confirm the release note
- [ ] Confirm test cases
- [ ] Confirm document
- [ ] Add branch pick label <!-- Add branch pick label that this PR
should merge into -->
---
.claude/skills/code-review/SKILL.md | 26 ++++++++++++++++++++++++++
1 file changed, 26 insertions(+)
diff --git a/.claude/skills/code-review/SKILL.md
b/.claude/skills/code-review/SKILL.md
index af69f7fdee9..539e36224d9 100644
--- a/.claude/skills/code-review/SKILL.md
+++ b/.claude/skills/code-review/SKILL.md
@@ -40,6 +40,32 @@ Always focus on the following core invariants during review:
- **Evidence Speaks**: All issues with code itself (not memory or environment)
must be clearly identified as either having problems or not. For any erroneous
situation, if it cannot be confirmed locally, you must provide the specific
path or logic where the error occurs. That is, if you believe that if A then B,
you must specify a clear scenario where A occurs.
- **Review Holistically**: For any new feature or modification, you must
analyze its upstream and downstream code to understand the real invocation
chain. Identify all implicit assumptions and constraints throughout the flow,
then verify carefully that the current change works correctly within the entire
end-to-end process. Also determine whether a seemingly problematic local
pattern is actually safe due to strong guarantees from upstream or downstream,
or whether a conventional local im [...]
+### 1.2.1 Optimizer/Nereids Review Output Style
+
+When reviewing optimizer rules, plan rewrites, expression rewrites,
slot/ExprId handling, predicate movement, join rewrites, TopN/sort/project
rewrites, materialization, or other Nereids planner behavior, findings should
be explained around a concrete plan tree whenever possible.
+
+Prefer this structure for each optimizer finding:
+
+1. Show a minimal input plan tree that triggers the issue.
+2. Mark the critical expressions, slots, ExprIds, order keys, join conditions,
or nullable sides in the tree.
+3. Explain the rewrite steps using the actual local function names, for
example `collectFromNode`, `simplifyProject`, `addUpperProject`, `replace`,
`infer`, or `pushDown`.
+4. Show the incorrect rewritten tree or the key wrong expression produced by
the rewrite.
+5. State the semantic difference: wrong rows, wrong nullability, invalid child
output, missed error, duplicated evaluation, changed volatile behavior, wrong
join semantics, or missed optimization.
+6. Then give the concise fix direction.
+
+Avoid long prose-only descriptions when a plan tree can make the issue
concrete. For example, instead of only writing that "a replacement map bypasses
canPullUp for aliases that were intentionally rejected", write:
+
+```text
+TopN(order by id)
+ Project(id, assert_true(x > 0) AS c)
+ Project(id, a + 1 AS x)
+ Scan(id, a)
+```
+
+Then explain that `c` is not pullable because it contains
`NoneMovableFunction`, but `x` is pullable. If `collectFromNode` records both
`c -> assert_true(x > 0)` and `x -> a + 1`, `simplifyProject` can remove `x`
below TopN and `addUpperProject` can synthesize `assert_true(a + 1 > 0) AS c`
above TopN. That moves a non-movable expression above TopN and may suppress
errors for rows discarded by TopN. The fix direction is to let non-forwarding
aliases that cannot be synthesized above TopN b [...]
+
+The plan tree does not need to be fully executable SQL, but it must preserve
the relevant output slots and operator dependencies. If the exact tree is
unknown, state that it is a reduced tree inferred from the code path.
+
### 1.3 Critical Checkpoints (Review Priority)
For PRs with not only local minor modifications, before answering specific
questions, you must read and deeply understand the complete process involved in
the code modification, thoroughly comprehend the role, function, and expected
functionality of the reviewed functions and modules, the actual triggering
methods, potential concurrency, and lifecycle. It is necessary to understand
the specific triggering methods and runtime interaction relationships, as well
as dependencies, of the spec [...]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
