[feature](plan)support sql plan management - #68499
starocean999 wants to merge 28 commits into
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
REQUEST CHANGES — static review incomplete at the mandatory three-round cap.
I found 20 actionable issues (18 P1, 2 P2). They include wrong-result frozen SQL, unsafe plan matching, capture loss, unmanageable cloud-created baselines, missing nested planner locks, and non-atomic persistence/failover behavior. Round 3 produced two new accepted findings, so the allowed review process did not converge; this review is explicitly incomplete rather than a claim that the remaining code is clean.
Critical checkpoint conclusions (code-review skill Part 1.3):
- Goal and proof: The change adds FE SQL plan management: baseline DDL, matching, frozen-plan replay, persistence, and automatic audit-log capture. The implementation does not safely accomplish that goal because the inline findings demonstrate wrong results, missed/incorrect capture, replay failures, and durability failures. The added tests do not cover the reported paths.
- Scope and focus: This is a broad new subsystem, not a small local change: 330 paths, including 61 production Java files, parser grammar, 9 Java test files, and 257 regression artifacts. The package organization is reasonably focused, but the correctness surface is necessarily large.
- Concurrency: DDL, periodic refresh, master-only capture, and FE role changes share BaselineManager state. The manager uses a read/write lock and I found no inconsistent nested-lock order or lock-held heavyweight SQL call that establishes a deadlock, but MF-015 misses the nested planner's metadata locks and MF-019 exposes stale state across promotion.
- Lifecycle: I traced internal-table bootstrap, all-FE refresh, follower-to-master promotion, daemon start/stop checks, session/global lifetime, and failure cleanup. MF-008, MF-016, and MF-019 show cloud, repair, restart, and promotion lifecycle gaps. No C++ static-initialization issue applies to this FE-only change.
- Configuration: Capture controls are intended to be dynamic and are reread by the daemon. MF-007 shows SQL variable assignment bypasses regex validation, and MF-008 shows the enable path can activate capture where all management DDL is prohibited.
- Compatibility: I found no FE/BE Thrift, native symbol, or storage-format change and no new FE-to-BE variable. The RuleType spelling change repairs an otherwise unreachable mixed-case name. The new internal-table persistence is not a rolling-format issue, but its refresh/failover semantics are unsafe (MF-016/MF-019).
- Parallel paths: I checked user DDL versus automatic capture, SESSION versus GLOBAL baselines, normal versus external catalogs, frozen-text versus in-memory fallback, and executor/EXPLAIN integration. Required behavior is not consistent across those paths; see MF-008, MF-009/MF-010, MF-015, and MF-017.
- Conditional checks: I reviewed cloud guards, scan restrictions, namespace/table-existence gates, matching levels, and fallback conditions. Several checks are incomplete or applied on only one parallel path (MF-008 through MF-011 and MF-017).
- Test coverage: The bundle adds 9 Java test files and 129 regression suites with 128 outputs, including 123 TPC-DS/TPCH suite-output pairs. Negative coverage is missing for the concrete inline cases: constant UNION rows, reordered derived LIMIT, capped audit scans, namespace collisions, quoted identifiers, external modifiers/catalog capture, SQL regex SET, failover, and injected persistence failures.
- Test results: Expected-result artifacts are present and were statically inspected as part of the changed-file sweep, but no test or build was run under this review mandate. I therefore cannot independently confirm runtime results.
- Observability: Capture counters and logs exist, but they do not make silent wrong-result replay, skipped audit rows, or nondeterministic durable status sufficiently detectable.
- Persistence and transactions: Baselines use the new internal table and periodic snapshots rather than an FE EditLog replay path. MF-016 shows a reachable duplicate-row partial failure, and MF-019 shows promotion plus DELETE-before-INSERT can lose an existing baseline.
- Data writes and crash safety: Durable status/create operations are separate statements without an atomic invariant. FE/store failures can leave ambiguous duplicates or erase an existing row, so atomicity and failover safety are not established.
- FE/BE propagation: No new value is passed to BE, so scattered Thrift-send-path updates and mixed-version BE handling are not applicable.
- Error handling: Capture intentionally logs and skips per-query failures, but persistence compensation can fail and later load an arbitrary row (MF-016); destructive create ordering can lose data when the insert fails (MF-019).
- Memory safety and BE nullability: The substantive change is Java FE code; no native allocation/ownership or BE ColumnNullable path is modified, so those checkpoints are not applicable.
- Data correctness: This checkpoint fails. MF-001, MF-002, MF-009 through MF-014, MF-018, and MF-020 provide concrete wrong-result, wrong-object, or failed-replay paths.
- Performance: The audit LIMIT bounds each scan but advances the whole window, creating loss rather than a safe paging optimization (MF-003). I found no separate supported CPU/memory hot-path blocker beyond the reported correctness issues.
- Other issues: The final changed-file/risk sweep found no unresolved candidate or invalid anchor, but late discoveries MF-019 and MF-020 require this review to remain explicitly incomplete at the cap.
Validation was static only against the authoritative bundle and exact head 3519d38042a1f35b4dba4d5ccefaf4a0b4d2e37d. No build or test was run.
|
/review |
There was a problem hiding this comment.
REQUEST CHANGES — static review completed after two convergence rounds.
Five new, non-duplicate issues are attached inline: upgraded clusters can miss the new SPM table (P1); failed capture rows are permanently marked processed (P1); PhysicalGenerate drops wrapped child clauses (P1); zero/NULL query_time cursors starve later audit pages (P1); and non-positive capture settings silently disable/skip capture (P2). Existing exact-head threads were treated as hard duplicate fences.
Checkpoint conclusions:
- Goal/proof and scope: This is a broad FE SPM feature spanning DDL, matching, frozen-plan replay, persistence, and audit capture. The five concrete paths show the PR is not yet safe to merge.
- Correctness: F4 can change rows/projections during replay. The investigated GROUP_CONCAT placeholder concern was dismissed because normal GroupConcat legality rejects the placeholder before physical decompilation and SPMPlanner falls back safely.
- Lifecycle/persistence: F1 leaves global baseline CRUD unavailable on upgraded installations whose legacy internal tables already exist.
- Capture/configuration/error handling: F2, F5, and F6 can silently lose audit candidates or disable capture; swallowed exceptions and watermark/cursor transitions prevent recovery.
- Concurrency/integration: I traced manager locks, FE startup/master transfer, command routing, session/global scope, and executor integration; no additional un-fenced issue was substantiated beyond the inline findings and existing threads. No FE/BE protocol or storage-format change is present.
- Tests/validation: No build or test was run because the review mandate is static-only. Existing tests do not cover the five reported paths; each inline comment requests focused regression coverage.
- Performance/security/focus: No separate performance, authorization, SSRF, or user-provided focus issue was found; the supplied focus file contains no additional focus.
All conclusions are based on the authoritative bundle and exact head 9ef0b43.
|
/review |
There was a problem hiding this comment.
Static review of exact head a42d7530ed0b85db2f0f4c7495ae380e6e9024af found blocking correctness, authorization, compatibility, and lifecycle issues. I reviewed all 342 changed paths, reconciled the existing inline threads as duplicate fences, and ran three convergence passes.
Checkpoint conclusions:
- Goal and scope: the SPM implementation is broad and internally coherent in many paths, but the accepted plan is not yet faithfully preserved for several supported query shapes.
- Data correctness: explicit scan pins, nested TopN semantics, MARK output identity, and some join/scan serialization paths can change results or make accepted baselines unusable.
- Authorization: the EXPLAIN retry can reuse privilege state from another tree, and physical freezing erases the view authorization boundary.
- Concurrency and lifecycle: synchronous durable I/O runs under the query lookup lock, and capture pagination does not retain a stable pending window.
- Configuration and compatibility:
enable_nereids_ruleschanges meaning for ordinary sessions, while embeddedSET_VARcan undo SPM's own safety settings. - Persistence, failover, cloud, and interfaces: startup/rebuild/refresh and cloud command paths were traced; no FE/BE protocol change is involved. Previously reported durable-dedup, upgrade, cloud-capture, and fallback issues were treated as existing fences and not reposted.
- Tests and validation: the PR adds broad FE and regression coverage, but the inline cases remain uncovered. This review is static only; per task constraints I did not build Doris or run tests.
|
/review |
There was a problem hiding this comment.
Request changes: static review of exact head c9b5a91 found six distinct correctness/lifecycle blockers, detailed inline. Existing inline threads were treated as hard duplicate fences.
Checkpoints:
- Semantic and replay correctness: set-operation qualifiers, nested view authorization traversal, and nested protected SET_VAR hints were traced through parser, plan, decompiler, and replay paths.
- Concurrency and persistence: status publication versus refresh snapshots was checked.
- Capture lifecycle: nullable keyset cursors and failed-row retry reachability were checked.
- Integration: parser/mark joins, Explain and StmtExecutor fallback, session/global paths, scan modifiers, and the full changed-file set were checked; no additional non-duplicate findings were substantiated.
- Validation: static evidence only; no builds or tests were run. Targeted regression, race, authorization, and retry tests are requested inline.
|
/review |
There was a problem hiding this comment.
Request changes: static review of exact head 5ec56070ecb4d0078629357f4f5610caca1bc0a6 found 16 distinct, non-duplicate correctness and lifecycle defects, detailed inline.
Coverage: all 344 changed paths were swept; three normal/risk-focused convergence rounds completed with final NO_NEW_VALUABLE_FINDINGS results; existing live threads were treated as hard duplicate fences. The user focus file had no additional focus points.
Checkpoint conclusions:
- Semantic correctness and matching: blocking wrong-result/write-redirection gaps remain in mixed IN handling, OUTFILE, ASOF USING, MATCH analyzers, aliases, conjunct ordering, and scan-parameter equality.
- Frozen SQL and compatibility: generated SQL can drop ESCAPE/function namespaces/types, alter typed empty branches and scan values, or emit syntax the parser cannot consume.
- Persistence, replay, and lifecycle: hint-bearing rows can disappear on refresh, failed promotion reloads expose stale baselines, and capture pagination state is lost across restart/failover.
- Concurrency/performance: lifecycle and failover paths were traced; no additional fresh performance issue survived duplicate fencing.
- Security: not assessed because no security review was requested under the repository threat-model instructions.
- Validation: static only. Runner instructions prohibited builds, tests, or source edits; the added tests/oracles do not exercise the reported replay, reload, and failover cases.
|
/review |
There was a problem hiding this comment.
REQUEST CHANGES — static review capped/incomplete after the mandatory third round.
I found 16 distinct, non-duplicate issues (8 P1, 8 P2), attached inline. They include wrong-result cross-session/cross-catalog replay, unusable MARK syntax, capture-checkpoint loss, startup/query stalls, and multiple persisted frozen-SQL failures. Round 3 produced two new accepted findings, so the process did not converge before its three-round cap; this review is explicitly incomplete rather than a claim that the remaining code is clean. Existing live threads were treated as hard duplicate fences, and the final refresh found no exact-head review or inline comment covering these findings.
Checkpoint conclusions:
- Goal and testability: The PR adds FE SQL plan management across DDL, matching, physical-plan freezing, persistence, and automatic capture. The reported paths show the implementation does not yet preserve query semantics or durable state reliably. Each inline comment identifies a focused regression or failure-injection case.
- Scope and focus: I reconciled all 356 authoritative changed paths and diff sections, including production code, parser grammar, FE tests, focused SPM regressions, and the repetitive TPC-DS/TPCH suite-output families. The supplied focus file had no additional focus.
- Concurrency and locks: Manager publication, refresh, master startup, and capture-daemon interactions were traced. No additional lock-order issue survived duplicate fencing, but synchronous uncoalesced snapshot I/O can block readiness and concurrent query rewrites (F9).
- Lifecycle and static initialization: Internal-schema creation, daemon startup, refresh/restart, and leader handoff were reviewed. F2, F3, F9, and F10 expose readiness, retry, crash, and storage-redundancy gaps. No C++ static-initialization checkpoint applies to this FE-only feature.
- Configuration and dynamic behavior: SET_VAR/session context, SQL mode, and creator/executor context were traced. F7, F12, and F13 show replay can retain or reconstruct the wrong runtime semantics.
- Compatibility and rolling upgrade: No FE/BE Thrift or native storage-format change is present. Persisted SQL is nevertheless a compatibility boundary: identifier, literal, SQL-mode, TVF, and grammar defects in F5, F11, F13, F14, and F16 break refresh/restart behavior.
- Parallel paths: Manual versus captured baselines, SESSION versus GLOBAL scope, creator-local fallback versus frozen-text replay, and startup versus periodic refresh were checked. F4 and F7 are fallback-only defects; F12 and F15 affect global replay; several persistence defects appear only after reload.
- Conditional checks: Matching levels, placeholder-residue guards, namespace qualification, hint stripping, and parser cardinality were reviewed. F1, F4, F6, F7, F14, and F15 demonstrate missing or misordered conditions.
- Tests and results: The added tests and expected outputs were statically inspected, but they do not cover the concrete inline cases. Per the review mandate, no build or test was run, so runtime results are not independently validated.
- Observability and error handling: Several failures degrade silently to a missed baseline after refresh (F5, F11, F13, F16), while checkpoint loss and long synchronous reads are only indirectly visible. Current logging does not make these semantic or availability failures reliably detectable.
- Persistence and failover: Durable baseline reload, internal-table creation, capture checkpoint loading, replacement, and leader handoff were traced. F2, F3, F5, F10, F11, F13, and F16 show retry, durability, redundancy, or reconstruction failures.
- Data writes, atomicity, and crash safety: F3 deletes the only capture checkpoint before its separately committed replacement is durable, leaving an explicit crash/handoff loss window.
- FE/BE propagation: This is FE-only planning and persistence work; no new FE-to-BE field, Thrift enum, or mixed-version BE send path applies.
- Performance: F9 can inherit a 43,200-second internal-query timeout on startup and rewrite paths and allows duplicate concurrent loads. No additional non-duplicate hot-path issue survived the final sweep.
- Other: Data correctness fails through F1, F7, F12, F14, and F15; replay availability fails through several P2 findings. No security review was requested under the repository threat-model instructions, so this review makes no vulnerability conclusion.
Validation was static only against exact head d4784bd3a9b5c2858811a916fca230074d06b3b2 and base b67f4d62b781b7c4f2a12f4a39f2c1c31cc4ad50. No build, test, or repository source edit was performed.
|
/review |
There was a problem hiding this comment.
REQUEST CHANGES — static review of exact head 9d7a7061540bd4584b507f1922fb188599ca5d2d completed after converged normal and risk-focused passes. Five distinct, non-duplicate defects are attached inline (2 P1, 3 P2). Existing live threads were treated as hard duplicate fences.
Checkpoint conclusions:
- Goal, scope, and proof: this is a broad FE SQL Plan Management feature spanning DDL, matching, physical-plan freezing, persistence, schema migration, and automatic audit capture. The organization is reasonably focused, but the wrong-result and lifecycle paths below mean the implementation is not yet safe to merge.
- Data correctness and error handling: false frozen-text classification can replay captured literals instead of user values (P1). Frozen expression-subquery replay and non-default-mode capture also fail silently instead of producing usable baselines (P2).
- Concurrency, lifecycle, persistence, and data writes: manager, refresh, initializer, leader/capture, and restart paths were traced. Ambiguous status-delete compensation can erase both durable versions (P1), and a transient schema ALTER failure has no in-process retry (P2). No additional unfenced lock-order or deadlock issue survived convergence.
- Configuration, compatibility, parallel paths, and conditionals: manual versus captured, session versus global, in-memory versus persisted frozen replay, and cloud/shared-nothing transaction outcomes were checked. No FE/BE protocol or native storage-format field is added, but parser mode is not propagated through the parallel auto-capture path.
- Tests and results: the PR adds broad FE and regression coverage, but the five concrete inline cases remain uncovered. Expected-result files were inspected statically; no build or test was run under this review mandate, so runtime results are not independently validated.
- Observability and performance: capture metrics/logging exist and no additional substantiated hot-path issue remained. The schema warning is misleading because its promised retry does not occur.
- Applicability: this is Java FE planning/persistence work; BE memory/nullability and FE-to-BE variable propagation checkpoints do not apply. The supplied focus file contains no additional user focus.
Validation was static only against the authoritative bundle and live exact head/base. No build, test, or repository source edit was performed.
|
/review |
|
Codex automated review failed and did not complete. Error: You've hit your usage limit. Visit https://chatgpt.com/codex/settings/usage to purchase more credits or try again at Sep 30th, 2026 2:34 AM. The selected account is excluded until 2026-09-30T02:34:00Z. Please trigger /review again; another configured account may be available. |
|
/review |
There was a problem hiding this comment.
REQUEST CHANGES — exact-head static review capped/incomplete after the mandatory third round.
Eleven distinct findings are attached inline (9 P1, 2 P2). Round three still produced three new accepted findings, so the review reached its allowed cap rather than convergence; this is not a claim that the remaining code is clean. All 81 existing inline threads were treated as hard duplicate fences.
Checkpoint conclusions:
- Goal/proof and result correctness: the FE SPM feature spans DDL, matching, frozen replay, persistence, and automatic capture, but the reported paths can replay the wrong table/UDF/columns/row limit, encode constraint-dependent semantics, or make persisted baselines unreachable.
- Scope/focus: the authoritative bundle contains 361 paths (77 production-source paths, 15 FE unit tests, and 269 regression artifacts); there was no additional user focus.
- Concurrency, locks, lifecycle, persistence, and crash safety: process-local id allocation can collide across a leadership overlap, promotion reload can publish an older snapshot, and lossy checkpoint serialization can consume unrecoverable capture work. No conflicting nested lock order was substantiated beyond those concrete races.
- Configuration, compatibility, and parallel paths: parser-affecting SET_VAR timing, mutable PK/FK metadata, session reset, raw fallback, SESSION/GLOBAL scope, CREATE/capture/reload, EXPLAIN/executor, and schema drift were traced. No FE/BE protocol or native storage-format change is involved.
- Conditions/comments and error handling: fail-closed guards were reviewed; the accepted raw/frozen classification and relation/schema checks are incomplete. Other concrete leads were existing-thread duplicates or were dismissed with evidence.
- Tests and negative coverage: changed suites/results reconcile, but the eleven failure modes lack focused race, handoff, reload, cross-session, UDF-collision, constraint-drift, and schema-drift coverage.
- Test results: static evidence only. No build, unit test, regression test, or runtime reproduction was run under the review constraints.
- Observability: metrics/logs exist, but they do not prevent or reliably expose the silent wrong-result and skipped-capture cases.
- FE/BE variables, native memory, and BE nullability: not applicable to this Java FE-only change.
- Performance: no separate supported performance blocker survived; the retry cap is reported for correctness/data loss, not throughput.
- Other issues: all accepted anchors were verified against the authoritative diff, but the third-round discoveries require the capped/incomplete label.
Reviewed exact head d0fa6c613d737cade01c1e09e5bd50f81938a7ee against base b67f4d62b781b7c4f2a12f4a39f2c1c31cc4ad50.
|
/review |
There was a problem hiding this comment.
Request changes: this exact-head static review found 10 new, non-duplicate issues (3 P1, 7 P2). The blocking correctness failures include stale observer metadata approving a frozen plan, a failed checkpoint read overwriting prior-leader progress, and distributed TopN replay returning too few rows.
Review scope and completion
- Reviewed all 366 authoritative changed paths (93
fe-corepaths, 2 parser grammar paths, and 271 regression suite/result paths), the full aggregate diff, existing inline threads, and the complete SPM create/freeze/persist/match/replay/capture call chains. The user focus file contained no additional focus points. - Existing threads were treated as hard duplicate fences; substantially similar context-expression, timeout, persistence, retry, identifier, UDF, SQL-mode, temporary-table, ASOF/MARK, and order-free LIMIT concerns were not reposted.
- Three normal/risk review rounds converged with no unresolved candidate after the final changed-file sweep.
- Validation was static only, as required by the runner prompt: no build, unit test, regression test, or runtime execution was performed. Changed
.outfiles were inspected but not independently regenerated.
Critical checkpoint conclusions
- Goal and proof: the PR implements broad FE SQL Plan Management with session/global baselines, frozen-plan SQL, persistence, refresh, capture, grammar, metrics, and tests. The goal is not yet achieved safely because accepted findings cover wrong results, missed/overwritten capture progress, invalid persisted SQL, and lifecycle-dependent replay behavior.
- Scope/minimality: this is a cohesive feature but not a small or locally focused change; its 366 paths and new decompiler/persistence protocols materially increase review and regression risk.
- Concurrency/thread safety: planner transforms are statement-local, but observer journal visibility and leader/daemon handoff are concurrent lifecycle boundaries. The pre-sync observer rewrite and failed-checkpoint-read continuation are unsafe; no new BE thread, bthread, atomic-memory-order, or lock-order issue applies.
- Lifecycle/static initialization: SESSION versus GLOBAL, immediate versus refreshed/restarted, FE promotion, checkpoint recovery, async audit publication, and schema/partition evolution were traced. Several accepted findings are lifecycle-only failures; no C++ cross-TU/static-initialization issue applies.
- Configuration: new SPM/capture controls are FE session/global variables and interval daemons reread dynamic values. The hardcoded five-minute capture overlap is not safe against the independently configurable audit-loader interval.
- Compatibility: no new FE/BE thrift field, BE storage format, or function-symbol compatibility path is introduced. Internal-table/grammar evolution and reload behavior were reviewed; already-raised schema-upgrade/replica concerns were not duplicated.
- Parallel paths: ordinary query, EXPLAIN, SESSION/GLOBAL, immediate/reload, creator/capture, master/observer, and frozen/fallback paths were compared. The ordinary query path orders observer synchronization incorrectly, while marker-free behavior differs between immediate and refreshed stores.
- Conditional checks: the distributed-TopN continuation predicate, temporary-partition identity, and purported multiset comparison are incorrect. Other reviewed fail-closed conditions either preserve availability or overlap existing comments.
- Test coverage: coverage is extensive, but it omits lagging-observer metadata, failed checkpoint reads through a full cycle, LIMIT-sized tied cursors, unkeyed retries, nonzero-offset captured TopN, set-operation parse/reload round trips, formal/temp namespace parity, marker-free immediate/SESSION replay, between-page publication, and equal-length redistributed duplicates.
- Test results: 135 changed regression result files pair with their changed suites; the remaining changed privilege suite is exception/assert based. Results were inspected statically only and are not claimed as executed validation.
- Observability: capture metrics and logs were added and baseline hits are exposed, but silent missed rows, marker-free non-enforcement, and parse fallback after reload are not adequately observable to an operator.
- Transactions/persistence: internal-table writes, reload publication, leadership handoff, and checkpoint UPSERT were traced. The failed-read path can still destroy durable progress before the atomic replacement protocol helps; existing ambiguous-write and allocator findings remain fenced by prior threads.
- Data writes/crash behavior: SPM writes baseline/checkpoint internal tables. MAIN-2, MAIN-4, and the late-publication finding show loss windows; no BE data mutation path is changed.
- FE/BE variables: all added SPM variables are FE-only (
needForward=false); no scattered thrift/BE propagation point is required. - Performance: decompiler complexity, cache refresh, pagination, and internal-query timeouts were reviewed. The inherited long internal-read timeout is already covered by an existing bounded-read thread; no separate performance comment is reposted.
- Error handling: checkpoint read failure is logged but the cycle continues destructively, and transient unkeyed failures are discarded despite a retry contract. Java exception/fallback paths were otherwise traced; BE
Status/THROW_IF_ERRORchecks are not applicable. - Memory safety: this is FE Java/parser/regression code; no BE allocator, COW, reservation, handle-ownership, or nullable-column memory-safety path is touched.
- Data correctness: three P1 findings demonstrate stale-schema/wrong-limit behavior or durable progress loss; the seven P2 findings cover reproducible wrong-result-under-supported-configuration, availability, capture, and persistence correctness gaps.
- BE null/nullable handling: not applicable; no BE
IColumn/ColumnNullablecode changed. FE audit SQL NULL handling was reviewed separately and is the basis of one pagination finding.
Because the P1 findings can produce incorrect query results or lose durable capture state, this review requests changes.
|
/review |
|
Codex automated review failed and did not complete. Error: {"type":"error","status":400,"error":{"type":"invalid_request_error","message":"The 'gpt-5.6-sol' model is not supported when using Codex with a ChatGPT account."}} Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
|
/review |
|
Codex automated review failed and did not complete. Error: Codex exited with status 0 without a terminal turn event; review is incomplete Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
|
/review |
|
Codex automated review failed and did not complete. Error: Your access token could not be refreshed because your refresh token was revoked. Please log out and sign in again. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
|
/review |
|
Codex automated review failed and did not complete. Error: Codex exited with status 0 without a terminal turn event; review is incomplete Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
|
/review |
77d9127 to
634ade5
Compare
|
/review |
There was a problem hiding this comment.
Static review of PR #68499 at 634ade59adeceb703ed9a3c405cbe2439c718811 is complete. I found four new P1 and six new P2 issues, each attached inline. Two earlier P1 threads remain applicable: 4134147076 (a final Project can still bury ORDER BY/LIMIT when a quoted or qualified sort key prevents hoisting) and 4134146996 (the runtime-context guard still permits now(), current_timestamp(), and current_date() to fold into creator-time literals). I did not repeat those comments. The supplied focus file had no additional focus beyond the full review.
Critical checkpoints:
- Goal and proof: The PR adds SQL plan baselines, capture, matching, frozen replay, management DDL, and tests. The paths are wired end to end, but the inline findings show cases where replay, fallback, and DDL do not meet the intended behavior. The tests cover ordinary flows; they do not prove the reported edge cases.
- Scope and clarity: The 388 changed files are concentrated in FE SPM code, FE tests, grammar, and regression fixtures. The feature is broad but coherent; no unrelated source change was identified.
- Concurrency and locks: Planner metadata is resolved before collection and table locking, enabling the stale-object race reported inline. Baseline status writes use a writer lock and short state-lock sections, but promotion can expose a stale loaded cache before reload completes. I found no separate lock-order or deadlock issue.
- Lifecycle: Master transfer, periodic reload, capture checkpointing, and fallback retries were traced. Promotion cache misses and no-op status confirmation still have the reported lifecycle defects; the proposed late-audit-publication loss was dismissed after checking the configured overlap.
- Configuration: SPM rewrite, timeout, and fallback are session settings, and the current head forwards the FE planning settings. Opt-in fallback can retain a point-query flag and an automatic runtime-filter wait from the failed replay.
- Compatibility: The internal schema and baseline loader carry defaults for older nullable fields and parser modes; no separate rolling-upgrade failure was substantiated. Fingerprint identity for different-database bind/plan tables remains wrong as reported inline. No BE protocol type was added.
- Parallel paths: Ordinary query, EXPLAIN, forwarded FE, GLOBAL, and SESSION paths were checked. The current EXPLAIN metadata revalidation and forwarded settings address earlier threads; the replay and fallback issues in this review remain.
- Conditions and guards: The view and namespace guards miss a scalar subquery hidden in
* REPLACE, and the view guard misclassifies a CTE alias that shadows a catalog view. The earlier runtime-context guard remains incomplete for current-time functions. - Test coverage: FE unit, SPM regression, and TPC suites cover major flows. Missing cases include concurrent DROP/CREATE before planner locking, CTE/view shadowing, hidden
* REPLACEsubqueries across databases or views, promotion/no-op ALTER failures, and failed point replay to non-point fallback. - Expected results: I inspected the changed regression suites and paired output artifacts; three changed suites use assertion/error checks without a changed
.out. No tests were run here, so expected result correctness is a static assessment only. - Observability: Capture/replay logging and metrics provide trace points. The false-success ALTER paths can still mislead operators despite those logs; no additional standalone observability issue was established.
- Persistence and failover: Baselines use the internal duplicate-key table with reload and winner selection. The promotion cache-miss and no-op winner/probe cases are the remaining persisted-state concerns, each reported inline.
- Writes and atomicity: Status changes persist INSERT then DELETE with compensation. Two surviving status rows can make a no-op probe accept the older version while reload chooses the newer one. No BE user-data write path changed in this PR.
- FE/BE variables: The existing runtime-filter wait is sent in query options. After a failed replay the original fallback can inherit the first plan's derived wait, as reported inline; no separate new FE/BE variable omission was found.
- Performance: Matching has a no-baseline fast path and capture scans are bounded. The inherited runtime-filter wait can add substantial avoidable query latency; no other substantiated performance regression was found.
- Other issues and limits: All accepted issues are inline. The empty-string audit-cursor predicate was excluded because ordinary eligible SELECT audits receive distinct query IDs before the tail keys, and no supported producer of the required tied eligible IDs was established. This review used static code and diff evidence only; builds, tests, and source edits were prohibited by the review contract.
Existing P0/P1 findings confirmed for this head: #68499 (comment), #68499 (comment)
|
/review |
There was a problem hiding this comment.
Review complete at head 33e8514e8632b837c51fc41418a602e4760d508f after three convergence rounds and a final sweep of all 391 changed paths. Four new P1 and two new P2 issues are inline. Eight previously reported P1 comments still apply and are carried by ID without reposting: 4110391955, 4111362454, 4104521044, 4122141934, 4134147076, 4141782554, 4106651232, 4106651274. The original pre-write races described in 4104521044 and 4122141934 were repaired, but their stale-cache outcomes remain possible when internal writes return OK while committed but unpublished. Historical 4111362468 and 4119817988 are fixed on this head.
Critical checkpoints:
- Goal and scope: The PR adds SPM commands, matching and replay, physical-plan SQL capture, durable/global and session baselines, forwarding, and audit capture. The grammar, FE implementation, FE tests, and regression suites/data form one feature. The review found correctness gaps in stored-SQL reparsing, table identity, status persistence, and capture continuity. No unrelated source modification was found in this review checkout.
- Concurrency and locks: Baseline writers and refresh use
writerLock, while matching reads usestateLock; table locking follows an earlier fingerprint lookup. I found a pre-lock table-cache race (inline) and a surviving stale-visible-refresh issue (existing 4104521044). Internal I/O is outsidestateLock; I found no additional proved lock-order deadlock. Audit and refresh daemons and promotion create the relevant concurrent paths. - Lifecycle and compatibility: Startup/periodic reload, promotion, forwarded session import, and post-forward DDL confirmation were traced. Context-free JOIN-hint parsing drops valid baselines (inline); a committed but unreadable checkpoint can lose a window on handoff (inline). Persisted-schema upgrade/provenance and fallback paths were inspected; no separate rolling-upgrade or static-initialization fault was established.
- Configuration and parallel paths: I checked GLOBAL versus SESSION, local versus forwarded SELECT/EXPLAIN, frozen versus fallback plans, first versus later checkpoint pages, and affected settings. Forced query forwarding exposes the EXPLAIN mismatch (inline). The default
insert_visible_timeout_return_mode=committedpermits the checkpoint and stale-cache visibility gaps. No separate missing FE-to-BE variable transmission was established. - Conditions, tests, and results: Special guards for replay metadata, rule masks, cursor retries, and write reconciliation were checked against their callers. FE and regression fixtures were inspected, including representative expected results, but the failure sequences above lack coverage; no independent mismatch was found in the inspected expected outputs. This review was static only: no build, test, or runtime validation was run under the review contract.
- Observability and performance: Logs, warnings, and capture metrics were inspected. The missing distinction between COMMITTED and VISIBLE is material to diagnosing the persistence findings. No separate substantiated logging or performance regression was found.
- Transactions and writes: The status INSERT/DELETE pair can delete the sole durable baseline after a false positive reconciliation (inline); the older ambiguous-delete and ID-collision blockers remain. Audit rows can arrive after their start-time window (inline). A first checkpoint reservation can be treated as durable before it is readable (inline). Existing 4122141934 still covers successful post-forward DDL returning with a stale observer cache.
User focus: no additional focus was provided. All new substantiated issues are inline; prior blockers are referenced without duplicate comments.
Existing P0/P1 findings confirmed for this head: #68499 (comment), #68499 (comment), #68499 (comment), #68499 (comment), #68499 (comment), #68499 (comment), #68499 (comment), #68499 (comment)
|
/review |
|
Codex automated review failed and did not complete. Error: Review recovery stopped: shared time budget or helper timeout exhausted Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
|
/review |
There was a problem hiding this comment.
Review result
Static review complete for PR #68499 at 7b93cfbcb779c2adec74d0adc97e50d7e04073d2 against the verified base cb39aec17d03afbebc12f32bf651d418a4587711. I request changes for one new P1 and the two still-applicable, previously reported P1 obligations. Four distinct P2/P3 findings are inline. Three review rounds completed; all final-round reviewers reported NO_NEW_VALUABLE_FINDINGS. The user focus file provided no additional focus.
The five new comments cover GLOBAL baseline write visibility (P1), forwarded SESSION baseline ID stability (P2), a case-variant capture table-count error (P3), quoted-column corruption in a second SQL-builder pass (P2), and checkpoint readback accepting another leader's row (P2). Each has a concrete trigger and an anchor verified against the authoritative changed-file diff.
Existing blockers carried, not reposted: 4093908079 still covers a matched query retaining a captured LIMIT at mergeLimitNode's class-mismatch return: its original derived-table example is fixed, but an independent plan SQL with a CTE root still leaves the user's larger top-level LIMIT unapplied. 4134146996 still covers the guard's obligation to reject all runtime-context expressions before freezing: its named identity/session cases are fixed, but now() and current_timestamp() still fold to the CREATE statement time. The old q02 relabel LIMIT example and the current late-audit-row example were independently checked and are fixed on this head.
Existing P2 issues not reposted: 4123270694 covers the remaining case-variant LATERAL VIEW name collision, 4119818019 covers partition variants collapsed by audit digest deduplication, and 4123270714 covers three same-name Project outputs left ambiguous. The new checkpoint comment is distinct from 4151718405: that earlier issue was scanning when no reservation row was readable; the new readback accepts the wrong readable row. All ten ledger candidates have a final accepted or duplicate disposition.
Review checkpoints
| Checkpoint | Conclusion |
|---|---|
| Goal and proof | The FE feature adds SPM CREATE/ALTER/DROP/SHOW, matching and frozen replay, automatic capture, and storage. Changed FE unit/regression coverage exercises many paths, but it does not prove the failure interleavings and query shapes reported here. |
| Scope and focus | All 393 changed paths were swept: 82 FE production, 30 FE tests, 2 grammar, 142 regression suites, and 137 expected-result files. They cluster around SPM and supporting Nereids behavior; no unrelated source change was identified. The change is broad and the accepted issues prevent a correctness sign-off. |
| Concurrency and locks | GLOBAL writes use a writer lock and a short state-lock publication; I/O is outside the state lock, and no independent lock-order deadlock was established. SQL OK before OLAP publication and a leader-handoff checkpoint readback remain unsafe (new P1/P2 comments). |
| Lifecycle | Startup/load, periodic refresh, promotion, SESSION reset and forwarding, capture checkpoint/retry, and fallback replan were traced. The GLOBAL publication and checkpoint handoff findings remain; no separate static-initialization or cleanup failure was substantiated. |
| Dynamic configuration | Capture filters rebuild from GLOBAL settings each cycle and relevant SPM session settings are forwarded before master planning. No separate dynamic-propagation bug was proved; forwarded SESSION IDs are unstable (inline P2). |
| Compatibility | Parser changes, internal SPM table creation/upgrade, stored-plan reparse, and raw fallback were inspected for rolling and persisted-state paths. No further compatibility defect was established beyond the reported replay/persistence issues. |
| Parallel paths | Local/forwarded SELECT and EXPLAIN, SESSION/GLOBAL baselines, cloud capture gating, and fallback were checked. The forwarded ID mismatch remains; previously reported fallback-state and forwarding-setting gaps are addressed on this head. |
| Conditional checks | Replay context, scan identity, table-count and SQL-output guards were followed through their callers. Two existing P1 obligations remain through new residual inputs; the case-variant table gate and quoted pass-through have distinct inline comments. |
| Tests and results | FE tests and changed regression suites/results were inspected, including TPCDS/TPCH fixtures. No build, unit test, regression test, or cluster run was performed because this review contract is static-only. Result correctness was not independently measured. |
| Observability | Capture/baseline logs and metrics were inspected; no separate logging or metrics defect was substantiated. Logs cannot recover an acknowledged but unpublished GLOBAL ID or a lost checkpoint window. |
| Persistence and failover | Internal tables, status reconciliation, visible reads, refresh and promotion were traced. GLOBAL INSERT success can precede visibility, and the new checkpoint probe can confirm another leader's row. |
| Writes and crashes | Multi-statement status transitions and single-row checkpoint UPSERT were reviewed for crash/handoff windows. The two persistence comments identify the remaining concrete loss paths; no additional atomicity failure was established. |
| FE–BE variables | No new FE-to-BE thrift variable path was identified. FE session forwarding fields were checked; their row identity is the distinct remaining forwarding issue. |
| Performance and other paths | Capture pagination, retries, query thresholds, and builder work were inspected. No independent high-cost regression was proved; the case-variant table gate can create unnecessary GLOBAL baselines. |
Review execution was static only. No Doris source file was edited and no build or test was run.
Existing P0/P1 findings confirmed for this head: #68499 (comment), #68499 (comment)
|
/review |
There was a problem hiding this comment.
Review of PR #68499 at 6638964220864f1ec088e65ad99588b0787dc7ab: request changes. The static review covered all 394 changed paths, the relevant callers and consumers, existing review threads, and three convergence rounds. I found 11 distinct new issues (five P1 and six P2), detailed inline. Three earlier P1 comments still apply and are carried without reposting: 4111362454 (leader handoff/ID collision), 4111362474 (SESSION baselines survive COM_CHANGE_USER), and 4154354925 (late visibility after CREATE confirmation failure). Other previously raised concerns were checked against the current head and were either addressed or already cover their remaining cases. No additional review focus was supplied.
Critical checkpoint conclusions:
- Goal and proof: The change implements SPM baseline DDL, matching, frozen/tree replay, durable FE state, and audit capture. The feature is incomplete in the cases identified inline, including wrong replay results, lost durable baselines, and missed capture candidates. Added FE and regression tests cover normal paths, but they do not prove the failure sequences in these findings.
- Scope: The 84 FE production paths, 30 FE test paths, 143 regression suites, and 137 expected-output paths all relate to this SPM work. The breadth is understandable for this feature; no unrelated change was identified in the authoritative changed-path list.
- Concurrency and locks: Baseline writes use a process-local writer lock, while master promotion, other FEs, audit publication, and refresh can interleave. That lock cannot fence a demoted FE or a promoted FE's stale snapshot; M-B1 and M-R2B1 give exact interleavings. I found no separate deadlock. The heavy internal-table reads/writes are already serialized in the manager, but publication and ownership still require durable checks.
- Lifecycle: I traced startup/upgrade, promotion, cache invalidation/reload, periodic refresh, SESSION reset/change-user, capture checkpoint/retry, and fallback. Promotion ordering, refresh comparison, and change-user state remain incorrect as cited; no separate lifecycle defect was substantiated.
- Configuration: SPM session/capture controls and their FE read sites were inspected. I found no distinct dynamic-configuration propagation defect in the changed paths; this was a static check.
- Compatibility: Internal-table schema upgrade, stored SQL/SQL mode, reload, and master/follower behavior were inspected. No separate rolling-upgrade or format incompatibility was proven; the durable identity and fingerprint failures remain blocking.
- Parallel paths and conditions: GLOBAL versus SESSION, frozen versus tree replay, CREATE/ALTER/DROP, cache hit/miss, and ordinary versus audit capture were traced. The comments identify guards that skip validation or compare too little: missing post-plan baseline, stale cache hits, unguarded identity DELETE, and incomplete schema fingerprint.
- Tests and results: I inspected all 30 changed FE test files and all 143 regression suites with 137 expected outputs, including the paired TPCH/TPCDS checks. The changed suite/output names and result markers align. Missing targeted cases include promotion/demotion races, schema nullability/constraint changes, repeated-placeholder OR pairing, and partition-specific audit dedup. No build, test, or cluster was run under this review's static-only instruction; expected outputs were inspected, not independently generated.
- Observability: Capture and baseline logs/metrics were checked. I found no separate logging or metric issue; the correctness failures can still be silent to callers where a stale cache reports success.
- Persistence and EditLog: SPM state is stored in internal tables rather than a new FE EditLog record. I traced insert/delete visibility probes, identity-based cleanup, winner selection, refresh, and handoff. M-B1, M-R2B1, M-R2B2 and the three carried P1 threads describe remaining persistence failures.
- Writes and crashes: The SQL write/probe sequence is not an atomic cross-FE ownership fence. A completed CREATE can be removed by a demoted writer; a late-published row can collide after a failed visibility confirmation. Normal crash/retry and checkpoint paths were inspected; no additional independent failure was proved.
- FE/BE variables: This PR adds no BE or Cloud code or new FE-to-BE thrift variable. I inspected FE session forwarding and SPM context propagation; the remaining SESSION isolation issue is the carried COM_CHANGE_USER comment.
- Performance and other semantics: The completion-aware audit predicate can force retained-history scans on each page (M-C2). Optimizer rule masking, nullable metadata, output labels, placeholder alignment, and OR matching have the additional semantic failures reported inline. No further distinct substantiated issue remained after the final changed-file and candidate sweep.
Review status: complete static review, with the new findings and continuing P1 blockers requiring changes. This is not a runtime validation claim.
Existing P0/P1 findings confirmed for this head: #68499 (comment), #68499 (comment), #68499 (comment)
|
/review |
|
Codex automated review failed and did not complete. Error: Review recovery stopped: shared time budget or helper timeout exhausted Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
|
/review |
There was a problem hiding this comment.
Request changes. This exact-head static review found six new issues: two P1 durable-state handoff races and four P2 replay, matching, performance, and regression-oracle defects. Five previously reported P1 comments still apply and are carried by ID without reposting. Three review rounds and the final changed-file sweep are complete. Two valuable findings first appeared in Round 3, so convergence remains incomplete under the prescribed three-round limit. The review focus file specified no additional focus.
Critical checkpoints:
- Goal and scope: The PR adds SQL plan management with manual and captured baselines, GLOBAL persistence, SESSION baselines, parameter matching, frozen/non-frozen replay, and EXPLAIN integration. Normal paths have broad tests, but the accepted findings and carried comments prevent a clean correctness conclusion. The 396 changed paths are related to this feature; no unrelated change was identified.
- Concurrency and locks: Query planning, the capture daemon, refresh, and management commands can overlap.
writerLock/stateLockserialize local manager state without an observed lock-order deadlock, but FE-local locks and a pre-dispatch leader check do not order writes across a master handoff. M1 and M4 show completed ALTER/DROP outcomes lost or undone; existing P1 #4155346584 covers a separate identity-delete handoff. Internal-table I/O under the writer lock also keeps local writers serialized. - Lifecycle: Initial load, promotion invalidation, generation-guarded refresh, SESSION reset/import, and retry/fallback cleanup were traced. No separate lifecycle defect survived beyond the accepted durable races and carried comments. This Java-only change introduces no cross-translation-unit static initialization concern.
- Configuration: Capture and SPM session settings, daemon interval refresh, and SQL-mode handling were checked. Dynamic daemon settings are read on later cycles; no distinct configuration propagation defect was substantiated.
- Compatibility: The internal SPM schema/initializer and replay metadata paths were checked; no separate rolling-upgrade, storage-format, or FE/BE wire-compatibility defect was substantiated in this static pass.
- Parallel paths and conditions: CREATE/ALTER/DROP/SHOW, GLOBAL/SESSION, SELECT/EXPLAIN, forwarded commands, frozen fallback, nested subqueries, and capture handoff were compared. M2 drops ORDERED/LEADING on non-frozen replay and M3 confuses explicit catalogs. Existing P1 #4106651232 still covers nested SET_VAR, while #4141782554 still covers a nested policy path.
- Tests and expected results: The 30 changed FE test files, 144 regression suites, and 137 expected-output files were inspected. All 123 changed TPCDS/TPCH suites assert baseline hits and compare SPM-on/off results; the expected-output pairs checked were consistent. M6 shows that the Round23 JDBC header assertion runs a different, nonmatching SQL and cannot prove replay behavior. Handoff and nested-expression cases above remain unproven by the focused tests. No build or test was run, and generated-output provenance was not independently established, as required by the review contract.
- Observability: Capture counters, logs, baseline metrics, and relevant error messages were checked; no separate missing diagnostic was substantiated.
- Persistence, transactions, and data correctness: GLOBAL rows use internal-table statements rather than one atomic status transition. M1's late status DELETE erases a newer completed ALTER; M4's late INSERT restores a baseline after completed DROP. Existing P1 #4110391955 covers ambiguous DELETE compensation, #4111362454 covers CREATE ID collision, and #4155346584 covers a late identity DELETE. The matching/replay findings and existing nested-policy comment also affect plan or result correctness. No BE visible-version or MoW code changed.
- FE/BE variables, memory, and performance: The reviewed settings remain FE-side; no new BE-bound variable path was identified. Capture paging/retry state is bounded in the inspected code, while M5 makes nested scalar-subquery inspection exponential and repeats uncached catalog resolution. No other concrete memory or performance issue survived review.
All six new comments are inline. Existing P1 comments are carried as IDs. Remaining suggestions were either duplicates of existing threads or lacked a concrete failing path.
Existing P0/P1 findings confirmed for this head: #68499 (comment), #68499 (comment), #68499 (comment), #68499 (comment), #68499 (comment)
|
/review |
There was a problem hiding this comment.
Complete static review of PR #68499 at 0fa85a5. The SPM feature has substantial FE and regression coverage, but six new correctness issues remain (four P1, two P2). Existing P1 comments 4161713833 (status INSERT can publish after a concurrent DROP) and 4151718386 (a query running longer than the bounded audit lookback can still be skipped) remain applicable and are carried without reposting. Round 2 normal and risk reviews all returned NO_NEW_VALUABLE_FINDINGS; the 398 changed paths and candidate ledger received a final sweep. No user focus was supplied.
Critical checkpoints:
- Goal and tests: CREATE/ALTER/DROP/SHOW, matching, frozen replay and automatic capture are implemented with FE unit and regression suites. Existing tests do not prove the six schedules and shapes reported inline; no build or test was run because this review invocation prohibits them.
- Scope: The new FE feature spans 85 production Java files, 31 FE tests, 145 regression suites and 137 expected-output files. The reviewed implementation follows existing Nereids, FE command, internal-table and daemon patterns; the remaining problems lie at their cross-component boundaries.
- Concurrency and locks: Master handoff, forwarded commands, follower reads, baseline refresh and the capture daemon can interleave. BaselineManager.writerLock and generation/state-version guards serialize local cache changes, but they do not provide a durable leadership/visibility barrier. SQL reads and writes under the lock can stall; no separate lock-order deadlock was substantiated.
- Lifecycle: Promotion invalidates the baseline cache before accepting writes, addressing the earlier literal stale-cache issue. Capture checkpoint state survives demotion in the same singleton and an old cycle can still write after demotion; see the checkpoint finding.
- Configuration: The daemon rereads capture settings and reschedules its interval. Configurable query_timeout can exceed the one-day completion lookback, so existing P1 4151718386 remains. SPM rewrite/fallback and SQL-mode configuration paths were inspected.
- Compatibility: Internal schema initialization and upgrade add nullable provenance/fingerprint columns and legacy-row handling. The new fingerprint column is too short for valid wide joins. No new FE-to-BE wire field or C++ change is in this diff.
- Parallel paths and conditions: GLOBAL versus SESSION, frozen versus tree fallback, forwarded versus local DDL, and normal versus retry capture paths were traced. LIMIT transfer and output-label alignment fail for the separate manual-plan shapes reported inline. Conditional status INSERT still lacks atomicity with a concurrent DROP (existing P1 4161713833).
- Test coverage and results: FE tests cover many parser, replay, schema and handoff cases; regression suites include TPCH/TPCDS comparisons and SPM DDL checks. The changed expected outputs and representative assertions were statically inspected; neither the newly found cases nor all live hit preconditions are covered. These are inspected artifacts, not independently executed test results.
- Observability: SPM logs, EXPLAIN diagnostics and metrics exist. The successful stale follower read and best-effort collision repair can nevertheless look like successful management operations, so their logging does not restore correctness.
- Persistence and writes: Baseline rows use DUPLICATE KEY and multi-statement status changes; capture uses a UNIQUE-key checkpoint UPSERT. Cross-epoch allocation, publication and checkpoint replacement lack the required atomicity/fencing; the inline findings give concrete failure schedules. Previous insert visibility and delete confirmation fixes address their documented cases.
- Variable transport: Forwarded SESSION baseline IDs and parser/session modes are carried through the FE forwarding path; no distinct missing FE/BE variable transport issue was found.
- Performance and other issues: The bounded audit scan preserves partition pruning, while existing inline performance concerns remain duplicate fences. The final sweep found no further distinct, substantiated issue.
Existing P0/P1 findings confirmed for this head: #68499 (comment), #68499 (comment)
|
/review |
There was a problem hiding this comment.
Complete static review of PR #68499 at aa11ae7. The intended feature is SQL Plan Management: manual and captured GLOBAL/SESSION baselines, plan replay, durable baseline/checkpoint storage, and FE forwarding. The ordinary creation, matching, replay, and DDL paths are represented by FE unit and regression cases, but the six new inline findings show correctness and scaling gaps. Two normal review rounds and separate risk passes converged with no unresolved candidate. No additional user review focus was supplied.
Critical checkpoints:
- Goal and proof: Manual baseline CRUD, matching, fallback, and basic capture have source-level tests. Those tests do not prove partition-topology replay, late audit publication, non-host-zone capture, high-count forwarding, loader bursts, or large durable-key lookup.
- Scope and clarity: The feature is modular across parser, planner, manager, capture, forwarding, and tests, but spans 400 changed paths. The scan, storage, and replay contracts interact across those modules; the inline findings identify the remaining mismatches.
- Concurrency and locks: Query matching uses the manager's state read lock; writers acquire writerLock before stateLock and keep internal-table I/O outside stateLock, so I found no lock-order inversion. The async loader reserves its slot only after thread creation (new P2). Six existing P1 threads still identify commit-time leader/ID/checkpoint races despite current pre-write guards.
- Lifecycle: GLOBAL rows load and refresh from internal tables; SESSION rows stay connection-local and are reconstructed on each forwarded request. Checkpoint state survives capture handoff. The forwarded context is fresh per request, explaining the repeated parse cost. There is no C++ cross-translation-unit static initializer in this FE-only change.
- Configuration: Rewrite and capture controls are mutable and read on their relevant paths. The audit producer's mutable global time_zone disagrees with capture's host-zone bounds (new P1); the mutable query audit wait can exceed capture's publication overlap (new P2).
- Compatibility: Nullable added columns, schema-upgrade retries, legacy row parsing, and fallback handling cover the inspected storage migration paths. I found no new FE-to-BE variable or BE dispatch contract here; FE-to-FE forwarding carries SESSION baselines, with the new import-cost issue.
- Parallel paths and guards: Local and forwarded SELECT, EXPLAIN, fallback planning, manual and captured baselines were traced. Retained matched-baseline metadata, prior ORDER-BY and nullability fixes, and fallback planner reset address their earlier threads. The current top-level LIMIT guard still permits an inner captured cap (existing P1); schema guards omit manual partition topology (new P1).
- Tests and expected results: I inspected changed FE tests, 146 regression suites, and 137 output files; the nine suites without outputs use assertions rather than snapshot results. I found no separate output mismatch by static inspection. Builds and tests were prohibited by this review contract, so this is not runtime validation.
- Observability: Baseline and capture paths have logs and capture counters. The audit-row omissions in the two capture findings can advance checkpoints without a failed-capture counter, so those scenarios need targeted tests or visibility when repaired.
- Persistence, writes, and failover: Writes affect the internal baseline and capture-checkpoint tables rather than user data. Read-back checks and promotion reloads cover several earlier cases, but six previously reported P1 handoff/limit findings remain applicable: 4164852732, 4164852722, 4164852710, 4161713833, 4161713819, and 4155346584. Their specific dispositions are recorded in the shared review ledger; they are not reposted inline. The ID-collision and inner-LIMIT threads remain applicable through distinct later orderings after the latest repairs.
- Performance and other issues: Forwarding reparses all SESSION baselines before the rewrite switch/deadline (new P2); query bursts can start redundant loader threads (new P2); each durable CREATE scans the non-keyed dedup columns under a ten-second timeout (new P2). The remaining new correctness issues are partition-topology replay and the two audit capture misses. No additional distinct candidate survived the final sweep.
This is a complete static review of the pinned head. Six new inline comments are attached; the six confirmed existing P1 comments remain blocking. Source files, builds, and tests were untouched.
Existing P0/P1 findings confirmed for this head: #68499 (comment), #68499 (comment), #68499 (comment), #68499 (comment), #68499 (comment), #68499 (comment)
|
/review |
There was a problem hiding this comment.
Complete static review of PR #68499 at head aa082d01615277f4aa316f264ecd5cfa7a505993.
I found four distinct new issues: two P1 replay failures and two P2 SHOW failures. Six previously posted P1 comments still apply; their IDs are carried in existing_blocking_comment_ids without reposting. The three round 2 reviewers found NO_NEW_VALUABLE_FINDINGS, and the final changed-file and candidate sweep is complete. The review focus file supplied no additional focus.
Critical checkpoints:
- Goal and proof: The PR adds SPM baseline creation, matching, frozen replay, capture, persistence, and SHOW. FE and regression tests exercise many paths, but the four inline cases show that the correctness goal is not yet met; no inspected test proves those cases safe.
- Scope and clarity: I inspected the 403 changed paths and traced the principal FE, parser, persistence, capture, and regression flows. The large cross-cutting scope follows the feature; I found no distinct scope-only issue.
- Concurrency and locks: The refresh/capture daemons, audit publication, forwarded DDL, and master handoff can overlap. Local cache locks protect in-memory maps, while separate durable SQL statements and leadership checks leave the six already reported P1 races. No additional lock-order deadlock or heavy locked operation was substantiated.
- Lifecycle: Startup load, promotion invalidation, periodic refresh, session reset, and capture checkpoint cleanup were traced. An already-loaded third FE can serve stale GLOBAL SHOW rows (inline P2). No static initialization issue applies to these Java paths.
- Configuration: Session SPM/fallback and MV rewrite settings affect replay; ordinary MV rewrite plus the default disabled SPM fallback causes inline P1. The live audit time-zone transition P1 remains. No new BE-side setting was identified.
- Compatibility: I checked the internal-table upgrade path, stored-plan rebuild, and changed persisted fields against refresh/replay. No additional rolling-upgrade or storage-format defect was substantiated beyond existing threads.
- Parallel paths: I checked manual and captured baselines, SESSION and GLOBAL scope, ordinary planning and EXPLAIN/fallback, leader and follower reads. The MV fingerprint, self-join selector, and cross-FE SHOW findings arise from differences between those paths.
- Conditional guards: The selector multiset loses relation occurrence (inline P1), and SHOW's equality parser treats an unsupported expression as LIKE (inline P2). Other examined guards either held or were covered by live comments.
- Test coverage: The changed FE suites and regression SQL/results cover common flows, but lack targeted MV-after-CREATE, partition-pin swap after data change, third-FE SHOW, and literal-equality SHOW cases. Expected result files and ordering/error conventions were inspected statically; runtime results cannot be certified.
- Tests run: None. The review prompt prohibits builds and tests.
- Observability: I inspected capture/refresh metrics and logs and found no separate logging-only blocker. The wrong-row and stale-result cases need behavior fixes.
- Persistence and failover: Baselines and capture checkpoints use internal tables; reload and failover were traced. Six independently confirmed live P1 comments cover remaining write/checkpoint epoch and LIMIT/time-zone failures.
- Writes and crashes: DDL and checkpoint writes are separate from cache publication. Existing P1 threads cover non-atomic leader transitions and status/identity mutations; no additional crash-only issue survived deduplication.
- FE/BE variables: The reviewed SPM selection controls are FE-side; no new FE-to-BE variable propagation gap was found.
- Performance and other issues: I reviewed audit pagination, refresh cadence, replay planning, and expected-result volume. No distinct performance or other correctness issue remained after candidate reconciliation.
Existing P0/P1 findings confirmed for this head: #68499 (comment), #68499 (comment), #68499 (comment), #68499 (comment), #68499 (comment), #68499 (comment)
| collectBindSideFingerprintEntries(ctx, bindPlan, | ||
| lockedTablesByQualifiedName(plannedPlan), entries); | ||
| } | ||
| collectPhysicalTableEntries(plannedPlan, entries); |
There was a problem hiding this comment.
[P1] Validate source metadata before replay-side MV substitution. Create a baseline for SELECT k, SUM(v) FROM t GROUP BY k, then create and refresh an eligible async MV. CREATE stored t in the fingerprint because its private rule mask excludes MV rewrites; an ordinary baseline hit can now plan an MTMV scan, and this call adds that new table identity to the replay fingerprint. schemaFingerprintEquivalent rejects it although t has not changed, and the default enable_spm_fallback=false makes the SELECT fail. Validate the frozen query's source tables independently of the optimizer's chosen MV, or apply a consistent replay rule mask.
| } | ||
| List<String> fromBind = new ArrayList<>(entry.getValue()); | ||
| List<String> fromPlan = new ArrayList<>(planSelectors.get(entry.getKey())); | ||
| Collections.sort(fromBind); |
There was a problem hiding this comment.
[P1] Keep each partition pin attached to its self-join occurrence. The guard sorts selectors for every occurrence of bare table t, so it accepts a bind over t PARTITION(p1) a CROSS JOIN t PARTITION(p2) b with a manual plan that swaps p1 and p2 between a and b. Let t(p,k) be range-partitioned on p, initially containing (1,1) in p1 and (11,1) in p2. Both SELECT a.k,b.k plans yield (1,1), but after adding (12,11) to p2 the bind also yields (1,11) while replay instead yields (11,1). The bind/caller match and table fingerprint still pass. Compare selectors per aligned relation/alias or reject ambiguous per-occurrence swaps.
| // promotion (which clears the map) SHOW reported ZERO rows although durable | ||
| // baselines existed, and a failed read never converged. Use the same confirmed | ||
| // read (with a retryable error) the mutating DDL relies on. | ||
| BaselineManager.getInstance().ensureLoadedConfirmed(); |
There was a problem hiding this comment.
[P2] Refresh GLOBAL rows for each authoritative SHOW. On FE B, load an empty baseline cache; on FE A, complete CREATE GLOBAL BASELINE PLAN; then run SHOW on B before its next refresh daemon cycle. ensureLoadedConfirmed() returns immediately for loaded=true, and getAllBaselines() copies B's old map, so SHOW reports no row despite the completed CREATE (and can retain a completed DROP for up to the refresh interval). This command says GLOBAL rows must be authoritative; perform a confirmed durable read for SHOW or route its GLOBAL portion to an authoritative FE.
| knownColumn = true; | ||
| break; | ||
| default: | ||
| knownColumn = false; |
There was a problem hiding this comment.
[P2] Reject equality predicates without a supported SHOW column. SHOW BASELINE PLANS WHERE 1 = 1 reaches this branch because its RHS is a literal and the LHS has no column name. It sets pattern = '1', bypasses the unsupported-predicate error below, and SHOW searches SQL/status/source text for 1 instead of handling the true predicate or rejecting it. WHERE id + 0 = 1 is misread the same way. Accept only a recognized column-equals-literal form here; raise the advertised analysis error for other expressions.
3 participants
What problem does this PR solve?
Issue Number: close #xxx
Related PR: #xxx
Problem Summary:
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)