shuke987 commented on PR #68499: URL: https://github.com/apache/doris/pull/68499#issuecomment-6213586104
## Recovered automated review findings The review findings below were recovered from failed runs [38860079169](https://github.com/apache/doris/actions/runs/38860079169) and [38870748242](https://github.com/apache/doris/actions/runs/38870748242), both reviewing commit `9bb9cbd13145a10d3cf8444bf545dba8e94e6e5b` (base `e62ed744ae02ffc2b8541c3ed9aa65ea8683f8a0`). The PR still points to that head. Both runs produced a final payload, but their initial GitHub review POST failed and subsequent readback found no delivered review. The recovered payloads were checked against the hashes recorded in their traces. This is a manual recovery of the original static-review findings, not a new review or a successful rerun. No builds or tests were performed. Both runs reached three rounds with **converged=false**. This comment does not change the workflow/check result. After deduplication, this comment restores **18 previously undelivered findings: 9 P1 and 9 P2**. The later run also classified the schema-fingerprint capture race as new, but it duplicates [existing comment 4123270541](https://github.com/apache/doris/pull/68499#discussion_r4123270541), so it is linked rather than reposted. Source links below use the line anchors recorded and checked against the pinned-head diff in the original runs. ## Findings recovered from run 38870748242 ### 1. [P1] Omit the internal frame when serializing LAG and LEAD [SPMExprSqlBuilder.java:471](https://github.com/apache/doris/blob/9bb9cbd13145a10d3cf8444bf545dba8e94e6e5b/fe/fe-core/src/main/java/org/apache/doris/nereids/spm/builder/SPMExprSqlBuilder.java#L471) Capture `SELECT lag(k) OVER (ORDER BY k) AS p FROM t WHERE k IN (1,2)`. `WindowFunctionChecker.visitLag` adds an internal ROWS frame, which this branch emits as explicit SQL. The retained `SpmConstList` makes `validateFrozenTextReplannable` return without planning because it reconstructs only scalar placeholder values. Thus the invalid frozen text is stored. On an identical hit, list substitution succeeds, but planning the replay reaches `visitLag` again and throws `WindowFrame for LAG() must be null`; the query fails with the default `enable_spm_fallback=false`. LEAD has the same path. Preserve the user-level absence of a frame for these functions and validate list-containing frozen plans before accepting them. ### 2. [P2] Expand following stars before positioning the suffix [SPMPlanTreeSupport.java:3412](https://github.com/apache/doris/blob/9bb9cbd13145a10d3cf8444bf545dba8e94e6e5b/fe/fe-core/src/main/java/org/apache/doris/nereids/spm/SPMPlanTreeSupport.java#L3412) Capture `SELECT u.*, s.* FROM u CROSS JOIN (SELECT k + 1, v + 1 FROM t) s`, with one column in u, then replay the +2 variant. The caller is `Project(u.*,s.*) -> CrossJoin(Scan(u),Subquery(s,Project(k+2,v+2,Scan(t))))`. The open u.* reaches this fallback, but `trailingCount` counts s.* as one output although it expands to two. The suffix loop then reads `outputLabelOf(s.*)`, which is null, leaving both captured +1 headers on the substituted +2 values. Expand the following items' labels and widths before computing `tailStart`. This is distinct from the existing preceding-star thread: `precedingLabels` fixes that side, while this following-star calculation still assumes one output per SELECT item. ### 3. [P1] Preserve tuple identity when reconstructing DISTINCT counts [SPMPlan2SQLBuilder.java:2837](https://github.com/apache/doris/blob/9bb9cbd13145a10d3cf8444bf545dba8e94e6e5b/fe/fe-core/src/main/java/org/apache/doris/nereids/spm/builder/SPMPlan2SQLBuilder.java#L2837) For nullable a,b containing `(1,7),(2,7)`, capture `SELECT COUNT(DISTINCT a,b) AS c FROM t`. Its multi-phase plan is reduced to `Count(IF(a IS NULL,NULL,b)) -> GROUP BY(a,b) -> Scan(t)`; `SplitAggBaseRule` deliberately uses a plain scalar count because the lower stage already deduplicates tuples. The IF argument is not marked as a distinct-merge buffer, so this fallback adds DISTINCT and freezes `COUNT(DISTINCT IF(a IS NULL,NULL,b))` over the retained grouping. Both tuples now contribute the same scalar 7, returning 1 instead of 2. The creation probe accepts this syntactically valid SQL. Recover the original tuple arguments, or retain tuple deduplication and emit its scalar count without another DISTINCT. ### 4. [P2] Retire hidden same-key twins before dropping their winner [BaselineManager.java:2325](https://github.com/apache/doris/blob/9bb9cbd13145a10d3cf8444bf545dba8e94e6e5b/fe/fe-core/src/main/java/org/apache/doris/nereids/spm/manager/BaselineManager.java#L2325) A handoff can leave the same baseline key enabled at ids 40 and 41. Matching now correctly collapses them to 41, but leader repair is best-effort and a follower cannot repair them. If DROP 41 runs before successful repair, this path deletes/tombstones only id 41; `wipeDurableRowsById` also only reads 41. The hash remains known, and the next cold lookup sees enabled 40 alone, so even the corrected collapse replays the baseline that was just removed. Durably retire the superseded ids, or preserve key supersession independently of the winner row, before DROP succeeds. This is distinct from the existing cache-miss DISABLE thread: that collapse is fixed, while deleting its winner removes the evidence it needs to suppress the loser. ### 5. [P1] Compare generator column collisions without case sensitivity [SPMPlan2SQLBuilder.java:1456](https://github.com/apache/doris/blob/9bb9cbd13145a10d3cf8444bf545dba8e94e6e5b/fe/fe-core/src/main/java/org/apache/doris/nereids/spm/builder/SPMPlan2SQLBuilder.java#L1456) With `t(X INT, arr ARRAY<INT>, k INT)`, capture `SELECT t.X AS a, lv.x AS b FROM t LATERAL VIEW explode(t.arr) lv AS x WHERE t.k IN (1,2)`. Binding preserves source X and generator x, but the case-sensitive `takenNames` set misses their collision and registers both as bare references. The frozen projection then uses `X AS a, x AS b` in a scope where Doris resolves column names case-insensitively, making both inputs ambiguous. The IN-list marker causes the creation-time replan probe to skip validation; identical replay substitutes the list, fails analysis, and surfaces an error with default fallback disabled. Normalize occupied and generated names consistently with the existing join/project collision logic, or preserve source qualifiers. ### 6. [P1] Register GROUPING_ID using its actual virtual expression [SPMPlan2SQLBuilder.java:4093](https://github.com/apache/doris/blob/9bb9cbd13145a10d3cf8444bf545dba8e94e6e5b/fe/fe-core/src/main/java/org/apache/doris/nereids/spm/builder/SPMPlan2SQLBuilder.java#L4093) For t(a,b,a_b) containing `(1,2,3)`, capture `SELECT a,b,a_b,GROUPING_ID(a,b) AS g FROM t GROUP BY GROUPING SETS ((a,b,a_b),())`. `NormalizeRepeat` exposes `GroupingId(a,b)` as a virtual slot named `GROUPING_PREFIX_a_b`, but this loop registers only `Grouping`. The unregistered-slot fallback reconstructs that name as `GROUPING(a_b)`, and `appendAggSelect` exports it under a valid alias. Since a_b is itself a grouping key, the frozen SQL passes the replan probe but returns grand-total g=1 instead of the caller's g=3. Register all grouping scalar functions from their actual expression and arguments; a concatenated virtual-slot name cannot recover their semantics. ## Findings recovered from run 38860079169 ### 7. [P1] Preserve SET_VAR application order across CTE blocks [SPMPlanTreeSupport.java:4000](https://github.com/apache/doris/blob/9bb9cbd13145a10d3cf8444bf545dba8e94e6e5b/fe/fe-core/src/main/java/org/apache/doris/nereids/spm/SPMPlanTreeSupport.java#L4000) For `WITH c AS (SELECT /*+ SET_VAR(time_zone='+08:00') */ k FROM t) SELECT /*+ SET_VAR(time_zone='+00:00') */ from_unixtime(k) FROM c`, with BIGINT `k=0`, the caller has this reduced tree: ```text CTE(c := Hint(+08, Project(k, Scan(t)))) Hint(UTC, Project(from_unixtime(k), Ref(c))) ``` `AnalyzeCTE` analyzes definitions before the main query, so ordinary execution ends in UTC. `walkPlans` visits the main child before CTE `extraPlans()`, so this helper collects `[UTC,+08]` into one wrapper; `EliminateLogicalSelectHint` applies that order and frozen replay ends in +08. The identical query still matches, but its value changes from `1970-01-01 00:00:00` to `1970-01-01 08:00:00`. Preserve the actual analysis order/effective settings, or decline conflicting assignments. The existing missing-hint thread is addressed; this is a distinct ordering failure introduced by carrying multiple blocks' hints. ### 8. [P1] Preserve every NULL-aware hash key during decompilation [SPMPlan2SQLBuilder.java:2143](https://github.com/apache/doris/blob/9bb9cbd13145a10d3cf8444bf545dba8e94e6e5b/fe/fe-core/src/main/java/org/apache/doris/nereids/spm/builder/SPMPlan2SQLBuilder.java#L2143) Capture `SELECT l.a FROM l LEFT NULL_AWARE ANTI JOIN r ON l.a=r.c AND l.b=r.d AND r.f>l.z`, with `l=(1,2,1)` and nullable keys in `r=(NULL,NULL,2)`: ```text NAAJ(hash=[a=c,b=d], other=[f>z], l, r) ``` Native execution retains the NULL-key build row; its true residual marks the probe matched, so the anti join returns no row. This loop chooses one scalar NOT IN key and moves the other equality into WHERE. Frozen `a NOT IN (SELECT c FROM r WHERE b=d AND f>z)` rejects the NULL `d` row, sees an empty subquery and returns `a=1`. Both build keys NULL remove key-order dependence. Keep native multi-key NAAJ or decline scalarization. The existing residual-loss thread is fixed; this distinct case changes a secondary hash key's NULL semantics. ### 9. [P1] Preserve nonmovable evaluation above a child LIMIT [SPMPlan2SQLBuilder.java:3230](https://github.com/apache/doris/blob/9bb9cbd13145a10d3cf8444bf545dba8e94e6e5b/fe/fe-core/src/main/java/org/apache/doris/nereids/spm/builder/SPMPlan2SQLBuilder.java#L3230) With `disable_nereids_rules='PUSH_DOWN_PROJECT_THROUGH_LIMIT'`, capture `SELECT bitmap_to_string(to_bitmap_with_check(v)) AS b FROM (SELECT k,v FROM t ORDER BY k LIMIT 1) s`, for rows `(1,1),(2,-1)`: ```text Captured: Project(to_bitmap_with_check(v)) -> TopN(k,1) -> Scan Frozen: TopN(k,1) -> Project(to_bitmap_with_check(v)) -> Scan ``` This hoist puts the child's clauses above the computed projection, so replay evaluates discarded `v=-1` and errors instead of returning the valid first row. The function is `NoneMovableFunction`, so ordinary project pull-up cannot repair this. SPM preserves the disabled rule; the probe only plans, and cap/fingerprint checks ignore expression placement. Hoist only through a proven movable projection or decline this freeze, and test an erroring function on a discarded row. ### 10. [P1] Reject UNKNOWN before condemning a CREATE reservation [BaselineManager.java:6212](https://github.com/apache/doris/blob/9bb9cbd13145a10d3cf8444bf545dba8e94e6e5b/fe/fe-core/src/main/java/org/apache/doris/nereids/spm/manager/BaselineManager.java#L6212) Every successful CREATE leaves a plain reservation. After that reservation has been idle for more than five minutes, repeat the identical CREATE and let one baseline id query fail while the sequence/tombstone store remains available. `probeRowFromTable` returns UNKNOWN, but this branch distinguishes only PRESENT: UNKNOWN reaches expiry handling and durably tombstones the healthy identity before duplicate validation. Refresh filters/repairs the earlier successful baseline away even if the replacement CREATE subsequently fails. Fail UNKNOWN retryably without altering the old identity; only a successful absence read should reach expiry handling. Cover an aged, already-successful reservation whose row probe fails, asserting that no tombstone or replacement write occurs. ### 11. [P1] Invalidate in-flight hash loads when an uncached DROP completes [BaselineManager.java:2441](https://github.com/apache/doris/blob/9bb9cbd13145a10d3cf8444bf545dba8e94e6e5b/fe/fe-core/src/main/java/org/apache/doris/nereids/spm/manager/BaselineManager.java#L2441) Let hash H be known but evicted. A query reads its ENABLED row, finishes tombstone filtering and `reconcileMatchingRows`, then pauses before acquiring the publication lock. This uncached DROP deletes the row, confirms its tombstones and returns, but neither advances H's epoch nor invalidates a bucket published concurrently. The query resumes, passes the unchanged `seqBefore == knownHashes[H]` check, and caches its old ENABLED row. Subsequent cache hits return directly without checking the new absence fence or tombstones, so later queries replay a successfully dropped baseline until refresh. Publish DROP's hash-epoch/cache change under `stateLock`, including this uncached path, and cover a load paused after reconciliation. This is a separate DROP completion path from the existing status/refresh publication thread; the new epoch updates there do not cover it. ### 12. [P2] Use supported DELETE predicates for an empty fingerprint [BaselineManager.java:599](https://github.com/apache/doris/blob/9bb9cbd13145a10d3cf8444bf545dba8e94e6e5b/fe/fe-core/src/main/java/org/apache/doris/nereids/spm/manager/BaselineManager.java#L599) A valid global baseline for `SELECT number FROM numbers("number"="5")` has no catalog relation or scalar function dependency, so its stored fingerprint is empty. DROP therefore executes `IFNULL(schema_fingerprint,'')=''` on the DUPLICATE KEY baseline table. With the nullable column this requires `schema_fingerprint IS NULL OR schema_fingerprint=''`: `DeleteFromCommand` rejects either a remaining function on the comparison's left side or the rewritten OR, and its `DeleteFromUsingCommand` fallback supports only UNIQUE KEY tables. Thus this newly created baseline cannot be dropped; pre-column rows with NULL fingerprints have the same problem. Emit supported simple predicates (separate NULL/empty deletes if needed) while preserving identity, and add a production SQL regression for a TVF-only CREATE/DROP. Current string-assertion protocol tests do not exercise DELETE lowering. ### 13. [P2] Preserve literal template syntax in persisted SQL [BaselineManager.java:7653](https://github.com/apache/doris/blob/9bb9cbd13145a10d3cf8444bf545dba8e94e6e5b/fe/fe-core/src/main/java/org/apache/doris/nereids/spm/manager/BaselineManager.java#L7653) Valid SQL can contain this explicit alias, which remains in its canonical digest: ```sql SELECT k AS `${id}` FROM t ``` These SQL-valued parameters feed a default `StringSubstitutor`, which [recursively expands inserted values](https://raw.githubusercontent.com/apache/commons-text/rel/commons-text-1.15.0/src/main/java/org/apache/commons/text/StringSubstitutor.java). Reservation maps leave `${id}` unchanged, but this INSERT map defines `id`, so it substitutes the numeric allocated id inside bind SQL, digest and plan SQL. Visibility confirmation compares the original digest against the corrupted row and GLOBAL CREATE fails. `${bindSqlDigest}` can instead cause a substitution cycle. Disable substitution inside replacement values for SPM rendering, or use a single-pass renderer, and cover literal template syntax in aliases; SQL escaping does not preserve these bytes. ### 14. [P2] Retain the last join input's known label prefix [SPMPlanTreeSupport.java:3829](https://github.com/apache/doris/blob/9bb9cbd13145a10d3cf8444bf545dba8e94e6e5b/fe/fe-core/src/main/java/org/apache/doris/nereids/spm/SPMPlanTreeSupport.java#L3829) Capture `SELECT * FROM (SELECT k AS x FROM a) l CROSS JOIN (SELECT v+1, u.* FROM t CROSS JOIN u) s`, with distinct table column names, and match the `v+2` variant: ```text Project(*) CrossJoin(Project(k AS x, a), Project(v+2, u.*, CrossJoin(t,u))) ``` The right subquery expands to `StarLabels([v+2], true)`, but this return keeps only the preceding `[x]` and discards `child.labels`. `alignRootOutputLabels` then treats the second output as unknown tail and retains the captured `v+1` header while computing `v+2`. Append the known child prefix before returning the open tail; the analogous USING branch needs its non-key prefix preserved too. The existing multi-star thread fixes SELECT-item positioning at the root; this distinct nested join-composition branch loses labels already derived by its child. ### 15. [P2] Expand inherited CTEs in their defining scope [SPMPlanTreeSupport.java:3751](https://github.com/apache/doris/blob/9bb9cbd13145a10d3cf8444bf545dba8e94e6e5b/fe/fe-core/src/main/java/org/apache/doris/nereids/spm/SPMPlanTreeSupport.java#L3751) Consider this frozen WITH query, even with identical SQL on replay: ```sql WITH c AS (SELECT k AS a FROM t), d AS (SELECT * FROM c) SELECT * FROM (WITH c AS (SELECT v AS b FROM u) SELECT * FROM d) s ``` `AnalyzeCTE` binds `d` to the outer `c`, so the result header is `a`. The nested scope inherits `d` as a bare Plan and shadows `c`; this call expands `d` against that nested scope, so its `Ref(c)` now derives inner header `b`. Root alignment renames the correctly frozen `a` to `b`. Retain each CTE binding's definition-time environment, including declaration order, and use it when expanding the body. The existing sibling-scope thread is fixed by copying maps; this distinct lexical binding failure occurs without mutating any sibling's map. ### 16. [P2] Align the output list of GROUPING SETS queries [SPMPlanTreeSupport.java:2854](https://github.com/apache/doris/blob/9bb9cbd13145a10d3cf8444bf545dba8e94e6e5b/fe/fe-core/src/main/java/org/apache/doris/nereids/spm/SPMPlanTreeSupport.java#L2854) Capture `SELECT sum(v+1) FROM t GROUP BY ROLLUP(k)` and match its `v+2` variant. The caller and frozen output shapes are: ```text Caller: Repeat(groupingSets=[k,[]], outputs=[sum(v+2)]) -> Scan(t) Replay: Project(c_N AS `sum(v+1)`) -> Aggregate/Repeat -> Scan(t) ``` `withAggregate` creates `LogicalRepeat` for ROLLUP/CUBE/GROUPING SETS, and `withProjection` returns it directly. Its output expressions are parameterized and match, but this heterogeneous-output allowance covers only `LogicalAggregate`; `LogicalRepeat` also lacks support in the output-list helpers. Alignment returns the captured projection unchanged, so replay computes `sum(v+2)` with the `sum(v+1)` header. Handle Repeat's output list consistently or decline this replay. This is the unhandled grouping-sets operator parallel to the existing, now-fixed ordinary aggregate header case. ### 17. [P2] Derive RIGHT USING key names from the left input [SPMPlanTreeSupport.java:3911](https://github.com/apache/doris/blob/9bb9cbd13145a10d3cf8444bf545dba8e94e6e5b/fe/fe-core/src/main/java/org/apache/doris/nereids/spm/SPMPlanTreeSupport.java#L3911) For `SELECT * FROM (SELECT k AS K,x FROM t) l RIGHT JOIN (SELECT k AS k,y FROM u) r USING(k)`, the reduced output is `Project(*, RightUsingJoin(l[K,x], r[k,y]))`. `BindExpression.bindUsingJoin` always takes `colName=leftSlot.getName()`, including RIGHT joins: it uses the right value but aliases it as `K`. This helper instead selects the right source and derives `k`, so `alignRootOutputLabels` renames the correctly frozen `K` header to `k` even for identical SQL. RIGHT SEMI/ANTI follow the same branch. Derive the name from the left key independently of which side supplies the value/non-key columns. The earlier case-insensitive key-consumption thread is fixed; this separate choice of naming source remains inconsistent with the binder. ### 18. [P2] Record keep-all needs from CTE consumers below UNION [SPMPlan2SQLBuilder.java:379](https://github.com/apache/doris/blob/9bb9cbd13145a10d3cf8444bf545dba8e94e6e5b/fe/fe-core/src/main/java/org/apache/doris/nereids/spm/builder/SPMPlan2SQLBuilder.java#L379) Capture this supported materialized-CTE shape: ```sql WITH c AS (SELECT k+1 AS x,v+1 AS y FROM t) SELECT a.x FROM c a JOIN (SELECT y FROM c UNION ALL SELECT q FROM u) b ON a.x=b.y WHERE a.x IN (1,2) ``` The direct consumer records producer column x. UNION is nonprunable, so its y consumer receives `need=null`, but this branch records no obligation for it. The producer is then pruned to x alone, while `visitPhysicalCTEConsumer` still emits a y reference, producing invalid frozen SQL. Capture retains both physical consumers and x/y mappings; this is the decompiler's separate pruning. The IN-list marker also makes `validateFrozenTextReplannable` return before planning, so CREATE accepts the broken text and replay fails to bind y with default fallback disabled. Treat null need as a keep-all consumer obligation, or disable producer pruning if any consumer is untracked, and cover this mixed UNION/list-marker replay. ## Existing blockers retained by the original runs These already-posted findings are referenced, without duplicate comments. The union below records which issues were retained by each run; omission from one run is not evidence that an issue was fixed. - Run 38870748242: [4260129823](https://github.com/apache/doris/pull/68499#discussion_r4260129823), [4241328271](https://github.com/apache/doris/pull/68499#discussion_r4241328271), [4241328267](https://github.com/apache/doris/pull/68499#discussion_r4241328267), [4241809315](https://github.com/apache/doris/pull/68499#discussion_r4241809315), [4241809324](https://github.com/apache/doris/pull/68499#discussion_r4241809324). - Run 38860079169: [4123270541](https://github.com/apache/doris/pull/68499#discussion_r4123270541), [4235672929](https://github.com/apache/doris/pull/68499#discussion_r4235672929), [4237894950](https://github.com/apache/doris/pull/68499#discussion_r4237894950), [4260129823](https://github.com/apache/doris/pull/68499#discussion_r4260129823), [4241328267](https://github.com/apache/doris/pull/68499#discussion_r4241328267), [4241809315](https://github.com/apache/doris/pull/68499#discussion_r4241809315), [4241809324](https://github.com/apache/doris/pull/68499#discussion_r4241809324). ## Original coverage limits **Run 38870748242:** All three final-round reviewers returned NO_NEW_VALUABLE_FINDINGS, but required detailed coverage is incomplete: 29 planner SPM unit files (ManagerAndPlanner, MatchingSafety, Plan2SQLBuilder, ReviewRound3, and Safety rounds 9-15,18-21,24-29,32,35,39,40,44-46,48) received only targeted review; independent semantic equivalence of all 123 benchmark frozen-SQL goldens remains unproven, especially 102 text-only TPCDS goldens. Exact planner/type provenance of the q14/q17 precision-only golden changes was not established. No unresolved concrete finding candidate remains. Runtime validation was prohibited by the review contract. **Run 38860079169:** Three-round cap reached without convergence: the final planner and persistence reviewers returned FOUND_NEW_VALUABLE_FINDINGS, while the risk reviewer returned NO_NEW_VALUABLE_FINDINGS. U2 remains unresolved: status INSERT and identity DELETE can stall between beginRowMutation validation and dispatch, without a demonstrated upstream guarantee that lease age/leadership is rechecked. Exhaustive independent changed-body coverage remains incomplete for 32/52 planner unit/accessor files and 26/36 nonbenchmark regression suites; the 123 TPCH/TPCDS suites received template/representative analysis rather than every full SQL body, and all 147 goldens received structural inspection without exhaustive independent numerical/provenance proof. All 465 paths were assigned and inventoried, deep production ranges were traced, U1 was verified as M12, and every verified finding is included. The unresolved protocol question recorded in the earlier run is retained as an open question, not promoted into a finding. These findings and limits are the recovered reviewers' conclusions; this manual posting does not claim fresh full-code validation or convergence. <!-- manual-review-recovery:68499:38860079169:38870748242:9bb9cbd13145a10d3cf8444bf545dba8e94e6e5b --> -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
