[feature](plan)support sql plan management - #68499
starocean999 wants to merge 39 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 |
|
/review |
There was a problem hiding this comment.
Reviewed PR #68499 at f0106fc against the authoritative 407-path bundle. Request changes: six new inline issues are attached (one P1 and five P2). Nine earlier P1 findings still apply and are carried by ID below, without reposting their threads. There was no additional user-provided review focus.
Critical checkpoints:
- Goal and proof: The change adds SQL Plan Management creation, matching, replay, durable and session baselines, audit capture, and SHOW controls. The paths work together structurally, but wrong-row TABLESAMPLE replay and the capture, fallback, and SHOW defects mean the goal is not fully met. Added FE tests, regression suites, and output fixtures were inspected; none proves the colliding sample pair, a changing threshold during capture, or the other reported edge cases. No build or test was run in this review.
- Scope and clarity: The broad set of FE, grammar, and test edits follows the feature's end-to-end paths. The review found no separate unrelated change to report. The selector and capture eligibility contracts need a single shared representation rather than parallel textual or mutable interpretations.
- Concurrency, locks, and lifecycle: Capture runs in a daemon while SET GLOBAL may change thresholds; the SQL read and filter construction use different snapshots, and a pending cursor spans daemon cycles. Durable baseline manager locks protect local state, while internal SQL can outlive leadership and does not get an atomic leader fence. The existing handoff/status/checkpoint P1 threads below remain live. No additional lock-order or static-initialization issue was substantiated; no native static lifecycle was changed.
- Dynamic configuration: Capture thresholds are mutable and read promptly, but that is unsafe with the persisted pagination cursor and separately built in-memory filter (new M-B2). The audit time-zone setting also remains an existing P1 pending-window blocker.
- Compatibility: Durable internal tables and follower snapshot reload were inspected, as were SESSION and GLOBAL routes. No new FE-to-BE variable or protocol field was identified. Replay correctness and rolling leader changes are still limited by the selector and handoff findings. No independent storage-format incompatibility was substantiated.
- Parallel paths and guards: SELECT and EXPLAIN share fallback context, so both retain the plan-side MV hint (M-E1). TABLESAMPLE uses a hash-text fallback in replay and a related audit fingerprint (M-A3); audit selector detection omits supported syntax (M-A4). The SHOW LIKE predicate omits scope (M-C2). Other suspected LIMIT and scan-occurrence variants are already covered by existing P1 threads.
- Tests and results: FE unit/concurrency tests, SQL regression suites, and generated-looking output fixtures were inspected statically, including negative and pagination cases. The submitted tests do not exercise the concrete failures above; observed fixture contents are not independently executed results, and this review cannot attest to how they were generated. No tests or builds were run, as required by the review prompt.
- Observability and errors: Existing capture/checkpoint logging covers explicit skips and failures, but silently discarded audit rows and changed replay semantics lack direct diagnostic evidence. No separate exception-propagation or Java ownership defect was substantiated; BE memory and nullable-column rules are not touched by this FE-focused PR.
- Persistence, data writes, and crashes: Audit checkpoint, status INSERT/DELETE, CREATE collision, and follower/leader handoff were traced through their SQL and transaction paths. The nine carried P1 comments cover the live atomicity and visibility defects; local locks do not make those durable writes atomic across master changes. No further distinct crash or read-version issue was substantiated.
- Performance and remaining issues: One 500-row audit page per three-hour default cycle bounds capture throughput and can leave a growing backlog (M-B3). The six inline comments and carried IDs account for all accepted findings after the final changed-file and candidate sweep.
Carried P1 comment IDs: 4151718386 (late audit completion), 4161713819 (status DELETE handoff), 4161713833 (status INSERT after DROP), 4164852710 (CREATE ID collision handoff), 4164852722 (inner LIMIT contract), 4164852732 (checkpoint handoff), 4165823773 (time-zone change during a pending window), 4166755527 (scan-selector occurrence alignment), and 4171273314 (fall-back DST range). Each was independently checked against this head; their existing inline threads are the duplicate fence.
Review convergence is incomplete under the three-round cap: the third normal round found an additional same-cycle threshold race, which is verified and included in the M-B2 inline comment. All identified candidates were resolved and the final changed-file sweep found no unresolved point; another convergence round is prohibited by the review prompt.
Existing P0/P1 findings confirmed for this head: #68499 (comment), #68499 (comment), #68499 (comment), #68499 (comment), #68499 (comment), #68499 (comment), #68499 (comment), #68499 (comment), #68499 (comment)
|
/review |
There was a problem hiding this comment.
Review status: Three static review rounds are complete on head ce47a2afa039a699a5d32f34deee2339d5e45068. A new valuable finding arose in round three, so convergence was not reached within the required three-round cap; this review is incomplete by that criterion. All nine independently verified new issues are inline below. Four existing P1 findings still apply and are carried by ID without duplicate comments: 4164852732 (checkpoint leader epoch), 4164852722 (hidden inner LIMIT), 4165823773 (audit time-zone change), and 4164852710 (a stale old-master INSERT can still leave a competing same-ID row, although the old master now fails its own CREATE). No additional user focus was supplied.
Critical checkpoint conclusions:
- Goal and proof: The PR adds SQL plan management across CREATE/ALTER/DROP/SHOW, frozen replay, and automatic capture. The nine inline failures show the goal is not yet met for all valid states. Existing tests cover basic use and selected handoffs, but do not prove the reported publication, clock, and filter transitions.
- Scope and clarity: The 411 changed paths are feature-related: 84 FE main Java files, 37 FE Java tests, two grammar files, 150 regression suites, and 138 expected-result files. The cross-module size warrants the full replay, baseline, capture, and test passes completed here.
- Concurrency and locks: DDL/daemon work, follower refresh, and FE promotion can overlap. Local writerLock serializes baseline writes and stateLock protects brief cache publication; no additional lock-order or deadlock defect was substantiated. M2/M3/M7 and carried 4164852710/4164852732 show gaps across commit publication and leader changes.
- Lifecycle and persistence: Startup/load, refresh, promotion, checkpoint restore, retries, and multi-statement status changes were traced. M2/M3/M7 affect durable row/cache agreement; M5/M6 affect the capture filter across handoff and windows. The previously reported checkpoint epoch issue remains.
- Dynamic configuration: Capture interval rescheduling and numeric window pinning are present. M5 loses the include/exclude pattern snapshot at handoff, M6 re-filters old retries with new globals, and the existing audit-zone thread still applies.
- Compatibility and parallel paths: Legacy SPM schema/provenance reads, cloud-mode gates, GLOBAL/SESSION and forwarded/local DDL, SELECT/EXPLAIN, frozen/non-frozen replay, and parser modes were reviewed. Cloud SPM management and capture are gated, dismissing the cloud false-absence hypothesis. M8 is a frozen replay clock path; the existing LIMIT issue is carried. No separate migration or FE/BE protocol defect was substantiated.
- Conditions and error handling: The code has explicit visibility probes, but SQL OK can mean COMMITTED before publication; bounded probes then drive unsafe compensation or stale-cache behavior in M2/M3/M7. Other suspicious conditions were either dismissed by reachable gates or covered by existing threads.
- Tests and results: I inspected FE tests, all 150 changed regression suites and their expected-result labels, and the 123 TPCH/TPCDS benchmark suites. The tests do not exercise the nine reported failure paths. Per the review contract, no build or test was run; expected output correctness is based on static inspection only.
- Observability and performance: Capture metrics and failure logs exist, but M9 has no retry byte/work budget after the 50-page drain. Existing snapshot/dedup performance threads were not reposted. No separate observability defect was substantiated.
- Data writes and FE/BE state: SPM durable writes and checkpoint UPSERT were checked against committed visibility and failover. This change adds no BE-side variable transport; BE visible-version, MoW, and memory-tracker checkpoints do not apply to the FE-only diff. No further distinct issue remained after the final changed-file and candidate sweeps.
Existing P0/P1 findings confirmed for this head: #68499 (comment), #68499 (comment), #68499 (comment), #68499 (comment)
|
/review |
There was a problem hiding this comment.
Review outcome: changes requested. I reviewed PR #68499 at b27825a3c814e643cf22942fe51a43aa456983a8 using the verified 414-path diff. Six distinct issues are inline below (two P1, four P2). Eight earlier P1 findings still apply and are carried by ID without duplicate comments: 4171273314, 4172437852, 4111362454, 4165823773, 4166755527, 4105319734, 4172437835, and 4164852706. The review converged after three rounds and a final changed-file sweep.
Critical checkpoint conclusions
- Goal, data correctness, and tests: The change adds SQL plan baselines, replay, durable GLOBAL management, and automatic audit capture. The integration is broad, but the new name-based decompiler paths can freeze wrong aggregate results or omit a legal output column; the paging and capture paths can publish stale state or lose eligible work. I inspected 151 changed regression suites, 139 output files, and 38 FE test files. Existing suites cover ordinary hits, 64/65 retry rewinds, and same-window filter restoration, but not the combined failure schedules or legal name collisions reported inline. These are static coverage conclusions; no build or test was run under the review contract.
- Scope, parallel paths, and conditions: FE planner/parser, GLOBAL and SESSION paths, forwarding, replay fallback, manual selectors, schema upgrade, audit scanning, and representative regression outputs were inspected. The feature is integrated but large; internal slot and function names are inferred from text in several parallel paths. The new findings isolate distinct cases. Older NULL-aware alias, result-label, marker-name, and enabled SESSION-forwarding concerns already have inline comments and were not reposted.
- Concurrency, locks, lifecycle, and writes:
writerLock/stateLockserialize local baseline state with no additional lock-order failure substantiated, but they cannot pin another FE's table version across snapshot pages (inline P2). Leader handoff, committed-but-unpublished CREATE/DROP, retry-anchor restoration, and post-forward visibility retain the carried P1 issues. A journal replay wait does not establish internal-table row publication. Local state is invalidated on promotion, but the listed residual paths can still replay stale or wrong baselines. - Configuration, persistence, and compatibility: Dynamic capture regexes can exceed the durable checkpoint column width (inline P2); a changed global
time_zoneand DST fall-back still have earlier P1 comments. I inspected fresh and upgraded internal schemas, checkpoint serialization, SQL mode pinning, and follower reads. This is FE-only SPM work; there is no new FE-to-BE field, BE data-version/delete-bitmap path, or C++ static-initialization path in the diff. - Error handling, observability, and performance: Failed checkpoint writes and baseline reads have logging/retry paths, yet an accepted oversized regex can stop capture before its first scan and an ambiguous DROP can leave a cached entry, as described inline or in carried comments. Bounded page reads reduce single-query timeout exposure but lack a cross-page version fence. The enabled SESSION forwarding payload/work concern is already reported as P2 4165823768. I found no additional distinct logging, metrics, or CPU/memory issue with a concrete failure path.
The focus file specifies no additional review focus. This was a static review only; I did not run builds, tests, or modify product source.
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 |
There was a problem hiding this comment.
Static review of PR #68499 at head 0ec72b260acdf74702e56f65d1dbb7537c5faaad completed through three normal rounds, separate risk-focused scans, and a final changed-file sweep. The review is incomplete under the prompt's convergence rule: two valuable findings appeared in the third and last permitted round. This submission records all 15 substantiated new findings (7 P1, 7 P2, 1 P3). No build, test, or product-source edit was performed.
Existing P1 blockers carried without duplicate inline comments: r4172437856 still applies to bare clock keywords parsed as bound Now/CurrentDate/CurrentTime, outside the unbound-function guard; r4172437852 still applies when a committed but unreadable CREATE loses its process-local pending reservation on leader transfer; r4171273314 still applies to the fall-back first-hour segment cut by the top-level scan upper bound. For the last thread, the originally cited second-hour row is fixed by range splitting, but an eligible first-hour row can be lost if the capture filter changes before the next window recovers its ambiguous local time. Existing P2 snapshot-page and upgraded-checkpoint-width issues remain in their live threads.
Critical checkpoints:
- Goal, scope, tests: The PR adds SPM DDL, frozen-plan matching/replay, session forwarding, and automatic audit capture across 416 changed files. Focused FE and regression fixtures cover many ordinary and negative paths, but the inline cases show remaining wrong results, missed matches, capture loss, and test gaps. All fixtures were inspected statically; none was independently executed. No user-specific review focus was supplied.
- Concurrency and lifecycle: Local baseline writes use
writerLockand map publication usesstateLock; capture runs in a daemon while AuditLoader and FE promotion have separate lifecycles. No additional lock-order deadlock was established. Leader handoff, async load, hidden COMMITTED publication, initial checkpoint reads, and connection-local SESSION lifetime expose the recorded failures. - Configuration and compatibility: Dynamic capture interval, filters, and global
time_zonewere traced through new and pending windows. SQL modes and baseline/checkpoint row decoding have explicit paths; the existing upgraded-column-width thread remains. No new BE protocol or FE-to-BE variable was found. FE-to-FE SESSION forwarding carries the unbounded payload reported inline. - Parallel paths and conditions: GLOBAL/SESSION, master/follower, SELECT/EXPLAIN, fallback, nested expressions, manual plans, and audit pagination were checked. Existing guards address several old threads, while the accepted LIMIT, placeholder, aggregate, UDAF, clock-keyword, and audit-window cases survive their conditions.
- Observability, persistence, and writes: Replay/capture logs and metrics expose normal decisions, but some accepted capture misses advance the watermark without an error. Baseline IDs and checkpoint cursors require durable ordering across crashes and takeover; the accepted comments identify the gaps. The SESSION payload and bounded-retry wakeup are the remaining performance/liveness concerns. The final inventory and candidate sweep found no unresolved candidate without an inline, an existing-thread disposition, or concrete dismissal.
Existing P0/P1 findings confirmed for this head: #68499 (comment), #68499 (comment), #68499 (comment)
|
/review |
There was a problem hiding this comment.
Static review of PR #68499 at ffbf943. Four distinct new findings are inline: one P1 and three P2. Convergence is incomplete: the fourth finding emerged in the third and final permitted review round. All 418 changed paths were covered by the normal and risk passes, and every candidate was validated or deduplicated. No builds or tests were run, as required by the review prompt. No additional user review focus was supplied.
Existing P1 blockers still applicable to this head are carried by ID rather than reposted: baseline CREATE/id/status handoff (4172437852, 4173514179, 4161713819, 4161713833); capture fall-back/zone/reservation/handoff (4171273314, 4173514182, 4173514206, 4164852732); and a hidden LIMIT in a retained CTE (4173514197). Existing P2 comments already cover the generator/export alias, wide fingerprint, and unavailable external-metadata residuals, so this review does not duplicate them.
Critical checkpoint conclusions:
- Goal and proof: The PR implements FE SQL Plan Management creation, persistence, replay, and audit capture. The reviewed code does not yet meet its correctness goal in the four new paths and the carried P1 paths. FE tests and regression fixtures cover many ordinary flows but do not prove these failure interleavings.
- Scope and clarity: This is a broad cross-layer feature. The SPM packages separate major responsibilities, but the large baseline and capture managers make state transitions and retries difficult to audit.
- Concurrency and locks: Query matching, baseline refresh, capture, internal-table writes, and leadership changes can overlap. Local writer/state locks and generation checks cover some same-FE races; the carried handoff comments show that a leadership check does not fence a later write on another FE.
- Lock work and ordering: Baseline internal I/O is generally outside the state lock but serialized by its writer lock. No new independent lock-order deadlock was established by static inspection.
- Lifecycle: Startup, promotion, reload, refresh, and daemon stop/restart paths were traced. Pending CREATE adoption and retry checkpoint restore still have the issues described inline.
- Dynamic configuration: Capture thresholds and patterns are read dynamically and pinned for a pending window. Time-zone changes and an older retry anchor can still produce the wrong scan zone; the existing zone-change blockers also remain.
- Compatibility and formats: Fresh and upgrade paths add nullable SPM columns and legacy checkpoint decoding. The review found no new distinct rolling-upgrade format defect; the prior fingerprint-size issue is already commented on.
- Parallel paths: GLOBAL and SESSION baseline paths, CREATE/ALTER/DROP, forwarded DDL, EXPLAIN, and replay fallback were checked. The new findings affect the GLOBAL durable path and capture handoff; no separate SESSION or FE/BE transport issue was substantiated.
- Conditional checks: Fingerprint, replay-limit, and leader guards are present, but pending CREATE equality omits the fingerprint and the oversized-id pagination guard assumes a two-row maximum that status failures do not guarantee.
- Test coverage: The changed FE tests cover ordinary and selected fault paths. They do not cover delayed status-INSERT publication with query matching, a zone change with truncated old retries, a schema change during pending CREATE, or a status-id group larger than one snapshot page.
- Test results: All 152 changed regression suites were statically paired with their expected-output files and labels. These files and FE tests were inspected, not executed; their results are not independent runtime validation.
- Observability: Capture counters and logging for retries, checkpoint failures, and baseline reconciliation exist. They aid diagnosis but do not prevent the confirmed stale or skipped state.
- Transactions and persistence: Baseline status changes use separate INSERT and DELETE statements, while capture uses checkpoint UPSERT and retry JSON. Ambiguous visibility, per-page snapshots, and leader handoff remain material correctness risks.
- Writes and crash recovery: The new status-cache and oversized-id findings can publish a state different from the durable winner. Existing P1 comments cover additional CREATE, status, and checkpoint interleavings.
- FE/BE variables and performance: No new BE variable propagation defect was established. Page and queue budgets bound routine work, but pagination must preserve all rows of a valid baseline ID group.
- Other issues: No further distinct candidate survived the 277-comment raw inline deduplication audit. The three-round cap prevents claiming full convergence after the new final-round finding.
Existing P0/P1 findings confirmed for this head: #68499 (comment), #68499 (comment), #68499 (comment), #68499 (comment), #68499 (comment), #68499 (comment), #68499 (comment), #68499 (comment), #68499 (comment)
|
/review |
There was a problem hiding this comment.
Review status: COMPLETE after three bounded, static review rounds. The third full pass and separate risk pass found no new valuable issue. The feature does not yet meet its correctness goal: two new P1 findings are inline below, and seven previously reported P1 findings still apply. review_focus.txt specified no additional focus.
The PR introduces SESSION and GLOBAL SQL plan baselines, frozen-plan replay, and leader-only audit capture across 418 changed paths. I reviewed the authoritative diff, related FE call chains, changed tests/results, and the full raw inline-comment history. Nine distinct new findings are inline: 2 P1, 5 P2, and 2 P3. Existing P1 comments 4111362454 (cross-leader ID allocation), 4123270541 (create-time schema fingerprint race), 4171273314 (fall-back audit window loss), 4173514206 (hidden checkpoint reservation), 4173514197 (hidden CTE row cap), 4173514182 (first scan after zone change), and 4172437852 (unpublished CREATE after pending-record eviction/promotion) are carried by ID instead of reposted. Prior P2 4164852736 already covers the unbounded schema_fingerprint in VARCHAR(4096); the remaining snapshot, output-label, and audit-publication examples covered by existing threads were likewise deduplicated.
Code-review checkpoints
- Goal and proof: The ordinary CREATE, SHOW, match, replay, and capture paths have FE and regression coverage, including 123 TPC-DS/TPC-H suites with EXPLAIN baseline-hit assertions. The accepted and carried defects show that the end-to-end goal is not yet met. This was static inspection: no build, test, cluster, or fixture generation ran here.
- Scope and focus: The production changes are FE/parser-facing; the large diff is mostly regression suites and expected-result fixtures. The implementation spans several necessary layers, though its size increases the need for end-to-end tests. No unrelated changed path remained outside the assigned review slices.
- Concurrency, locks, and work placement:
writerLockserializes one FE's baseline writes andstateLockguards its cache, but neither fences another leader. Physical planning releases metadata locks before fingerprinting; audit worker dequeue/batch transfer races with capture's horizon read. Heavy internal-table I/O is generally outsidestateLock; no separate lock-order/deadlock issue was substantiated. The cited races and older P1 threads remain. - Lifecycle: I traced cache load/refresh, promotion invalidation, pending CREATE records, per-statement SESSION import, audit queue/batch publication, and checkpoint resume. Bounded pending records and observer-local audit state still leave the already reported handoff/publication gaps; the new inline findings cover additional status, horizon, and forwarding cases.
- Configuration: Rewrite/session variables are forwarded to the planning FE, and capture thresholds/intervals and SQL mode are read at their relevant cycle/statement boundaries. The first scan after an unrecorded global time-zone change remains covered by
4173514182; no additional distinct dynamic-configuration defect was established. - Compatibility and parallel paths: SPM persists through new internal tables with nullable provenance fields; no BE source or new FE-to-BE protocol field is in this diff, and no new EditLog payload is introduced. I checked SESSION/GLOBAL, local/forwarded, manual/automatic, frozen/fallback, and CTE/subquery paths. The earlier P2 internal-schema issues remain in their threads; no separate rolling-version failure was established from this PR base.
- Conditions and errors: Conditional INSERT, duplicate-row winner, schema, limit, qualifier, time-zone, and table-identity guards were traced through their callers. M9/M10/M13 expose status-result/cache disagreement; the existing hidden-limit and fingerprint threads cover other wrong-result paths. No other silent failure was substantiated after the final recheck.
- Tests and expected results: Changed FE tests and regression suites cover many success, error, handoff, and replay cases. The nine new comments identify concrete missing boundary cases. I inspected representative
.outfiles and all benchmark hit-check presence, but did not execute tests or regenerate expected results, so their runtime correctness is unverified here. - Observability: Capture counters and contextual logs exist, and EXPLAIN surfaces baseline hits. They do not establish publication visibility for an audit row or warn the SQL client when an enabled SESSION baseline is omitted from a forwarded payload.
- Durability, writes, and failover: GLOBAL state is stored through internal-table DML, not new EditLog records. Multi-statement status changes and leader handoff are not atomic; a crash or delayed publication can leave cache and durable winners apart or lose capture progress as detailed in the inline and carried P1 findings.
- Performance and remaining issues: The replay candidate fast path and bounded capture work were checked. The 8 Mi character forwarding cap avoids transport overflow but silently changes baseline selection (inline P2). No other concrete performance issue or unresolved candidate survived the final changed-file and raw-comment sweep.
Existing P0/P1 findings confirmed for this head: #68499 (comment), #68499 (comment), #68499 (comment), #68499 (comment), #68499 (comment), #68499 (comment), #68499 (comment)
|
/review |
There was a problem hiding this comment.
Review outcome
Request changes on head a7d0302. This static review found six new issues in the inline comments (five P1, one P2). Three previously reported P1 issues still apply and are carried by ID: 4173514179 (ID reservation visibility after promotion), 4173514206 (hidden checkpoint reservation across takeover), and 4173514182 (audit rows after a global time-zone change). I did not repost those threads. Four other residual concerns were suppressed because earlier same-path comments already cover them: paginated snapshot consistency, schema fingerprint width, oversized same-ID paging, and fall-back DST scan ranges. The user supplied no additional review focus.
Critical checkpoints
- Goal and proof: The PR adds SPM baseline creation, mutation, capture, and replay. It has unit and regression coverage, but the six inline counterexamples remain uncovered; the feature is not correct for those cases.
- Scope and clarity: The authoritative diff contains 418 changed paths: 86 FE production files, 40 FE unit tests, 152 regression suites, and 140 outputs. The paths are feature-related; no separate scope defect was substantiated.
- Concurrency and locks: Local baseline mutations and refresh use writerLock, and cache/index changes use stateLock. I found no separate lock-order deadlock. The accepted DROP and ALTER cases show cache publication can still disagree with durable rows after failures.
- Lifecycle and initialization: Leader handoff, baseline refresh, capture checkpoint restoration, and session import were traced. The three carried P1 threads remain failover or zone-lifecycle blockers; no additional static-initialization issue was substantiated.
- Dynamic configuration: Capture interval and filters are reread by the daemon. A global time-zone change can still exclude older zone-free audit rows, as described by existing P1 4173514182.
- Compatibility and storage: Fresh and upgraded internal SPM table schemas, durable row selection, and the FE forwarding payload were checked. No separate rolling-upgrade or format incompatibility was established.
- Parallel paths and conditions: Frozen replay, fallback, CTE bodies, derived tables, and manual plan SQL were compared. LIMIT checks use values without placement and omit CTE definitions; the clock guard omits bound bare-keyword forms. The status exception probe identifies a status, not the attempted transition.
- Test coverage: Added tests exercise many happy and failure paths, but not the post-DROP cleanup read failure, stale same-status INSERT proof, failed first-reservation wakeup, shifted LIMIT cap, CTE-body cap, or bare clock syntax. No build or test was run under the static-only review contract.
- Test results: I inspected the changed test artifacts; static comparison of all 123 changed TPCDS/TPCH with-SPM and without-SPM expected-result pairs found no mismatch. This does not establish runtime success.
- Observability: The changed write and capture paths log baseline IDs and checkpoint failures. No independent logging or metrics defect was substantiated.
- Persistence and failover: SPM state uses internal tables and the initializer rather than a new EditLog record. The carried ID and checkpoint visibility blockers, plus the new status/cache findings, show durable-versus-live state remains unsafe in the stated races.
- Writes and atomicity: Status changes use an INSERT/DELETE pair on a duplicate-key table. The accepted stale confirmation and post-delete cleanup findings show failure handling is not yet atomic from the reader's perspective.
- FE/BE transport: No BE implementation or FE-to-BE variable change is in this diff. Forwarded SESSION baselines are bounded at creation and imported before matching; no separate transport issue survived review.
- Performance and other issues: The hot-path baseline gate, bounded capture work, and sequence MAX query were examined. No additional performance finding had a demonstrated failure path. BE memory, version, and delete-bitmap checkpoints do not apply to this FE-only change.
The third review round converged: all normal and risk-focused passes reported no new valuable findings. All six new anchors are added lines in the authoritative diff. Review was static only; no product source was changed.
Existing P0/P1 findings confirmed for this head: #68499 (comment), #68499 (comment), #68499 (comment)
|
/review |
There was a problem hiding this comment.
Static review of PR #68499 at head a2ae3b675e7ab6336c2cf96f5acc9153f27b1fa0 is complete after three normal and risk-focused rounds and a final changed-file sweep. I found three new P2 capture gaps and one P3 regression-fixture issue, reported inline. Six earlier P1 comments still apply on this head and are carried by ID below, without reposting. The unresolved P1s prevent a positive correctness verdict.
Critical checkpoint conclusions
- Goal, scope, and proof: The PR adds SQL Plan Management baseline DDL, matching, frozen-plan replay, persistence, and audit-driven capture. The 421 changed paths include the two grammars, 84 FE production Java files, 41 FE test files, and 153 regression suites with 141 output files. This is a broad cross-layer feature change. Tests cover ordinary create/replay and many failure cases, but the concrete residuals below mean the implementation does not yet meet the full replay/capture contract.
- Concurrency and locks:
BaselineManageruses process-local writer/state locks and keeps internal-table I/O outside the state lock; the local lock order showed no separate deadlock. Those locks cannot fence old-master writes after a handoff. The audit processor, audit loader, and capture daemon use separate workers and queues, and the new horizon observes only part of that pipeline. New P2 comments cover the follower, upstream processor, and committed-but-unpublished states. - Lifecycle and persistence: Startup, promotion, refresh, schema creation/upgrade, checkpoint restore, and plugin start/stop paths were traced. GLOBAL baselines and capture progress persist in internal tables rather than a new EditLog record. Existing P1s still cover cross-master ID allocation, status DELETE, and checkpoint UPSERT races; the split fall-back audit window also still loses its first segment. Existing P2s 4173024998 and 4175894967 remain on paginated snapshot consistency and equal-ID page boundaries.
- Configuration and compatibility: Dynamic capture settings are reread per daemon cycle; SESSION forwarding, SQL mode, cloud-mode gating, and nullable internal-schema upgrade paths were inspected. No new FE-to-BE wire variable or changed BE storage layout appears in the PR. The saved-variable semantics of an alias UDF can still be lost on frozen replay (existing P1 4119817988); mixed-version behavior was inspected statically, not exercised.
- Parallel and conditional paths: GLOBAL/SESSION CREATE, ALTER, DROP, SHOW, local/forwarded execution, ordinary SELECT, EXPLAIN, and fallback were checked against their guards. The retained-LIMIT input key still conflates self-join sides (existing P1 4176748160). The new local audit horizon does not finish a same-FE successful-load publication cycle (existing P2 4173514211); the three new capture comments identify separate boundaries that also need coverage.
- Tests and expected results: I checked all 421 changed paths, the 141 generated-output headers and suite labels, and the 123 TPC suite/output pairs statically. The new join and recursive-CTE suites and grammar were included in the final sweep. Four fixed SQL results in
test_spm_review_round27.groovybypass runner-generated output, as the P3 inline comment explains. Added tests do not discriminate the cross-FE, upstream-queue, Publish Timeout, after-check handoff, or first fall-back-segment cases. No build, unit test, or regression test was run, as this review environment prohibits them; inspected fixtures are not executed validation. - Error handling, observability, and performance: Capture and lifecycle code add logs and metrics, but a skipped audit row can leave no explicit failure signal. Stream-load
Publish Timeoutis committed but may remain unreadable; the loader clears its horizon without accounting for that response. I found no separate substantiated memory, lock-order, or performance issue in the changed FE code, and made no runtime performance claim. BE transaction status was inspected only to validate that response contract.
Existing P1 disposition: 4111362454 (ID allocation across handoff), 4119817988 (alias-UDF saved variables), 4161713819 (status DELETE across handoff), 4164852732 (checkpoint UPSERT across handoff), 4171273314 (fall-back scan segments), and 4176748160 (retained LIMIT placement) remain applicable. They are not duplicated in this review. Older P2s 4173514211, 4173024998, and 4175894967 also remain relevant and were not reposted.
The user supplied no additional review focus. All round-3 agents returned NO_NEW_VALUABLE_FINDINGS; every ledger candidate is accepted, deduplicated, or dismissed. This review is static-only and complete for the pinned head.
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.
Review result
Completed a static review of PR #68499 at head 2006701ed23079ea929605a4bdd862108ded53ae in two convergence rounds. The final sweep covered all 427 changed paths and resolved every candidate. This review adds eight P2 and one P3 inline findings. Ten previously reported P1 findings remain applicable; I carried their IDs instead of reposting them.
Existing P1 blockers carried: baseline CREATE/ID and status handoff or cache consistency: #4172437852, #4173514179, #4164852710, #4175894961, #4176216441, #4161713819; checkpoint reservation/final write and audit writer-zone reachability: #4173514206, #4164852732, #4173514182; retained LIMIT input identity: #4176748160. The newer checks narrow some original race windows, but residual handoff and visibility paths still produce the underlying correctness failures. Historical P2 snapshot and Generate alias threads (#4173024998 and #4123270694) cover other residual cases and receive no duplicate inline.
Critical checkpoints
- Goal, scope, and size: The change adds SQL plan management across baseline DDL, structural matching, frozen SQL replay, durable refresh, and audit-driven capture. The 89 FE main, 43 FE test, 153 regression suite, and 142 regression data paths are functionally connected but broad. Main feature paths exist; the listed correctness defects prevent a clean review.
- Concurrency and locks: Per-manager writer/state locks protect local maps and keep heavy table I/O outside the state lock, but they do not serialize separate FE leaders. Audit runtime, processor, loader, and reporter threads have observable handoff gaps; pre-write leadership checks do not fence later forwarded SQL. No distinct lock-order deadlock was established.
- Lifecycle: Startup, promotion, follower refresh, forwarded DDL, session reset, checkpoint resume, and audit publication were traced. The carried P1 IDs cover remaining promotion/late-publication faults. No separate Java static initialization problem was found.
- Configuration and compatibility: Capture interval and filter settings are reread for daemon cycles, while global time-zone changes can invalidate zone-less audit horizon freshness or leave an intermediate audit writer zone unreachable. Persisted baseline/checkpoint schemas and old-row parsing include compatibility paths; the cross-FE temporal cases above remain. No separate rolling-upgrade protocol defect was substantiated.
- Parallel paths and conditions: GLOBAL/SESSION baselines, local/forwarded queries, CTE and subquery limits, planner fallback, loader and pre-loader audit stages were compared. Affected-row, stored-second, and conditional status checks fix several prior exact cases, but retained LIMIT alias identity and several ambiguous-write branches remain unsafe as cited.
- Tests and results: FE unit and regression cases, fixtures, and expected results were inspected; builds and tests were prohibited and were not run. Coverage misses the concrete audit handoffs, report failures/visibility, idle self-audit, scanner timeout, and fractional completion boundary in the new inline comments. No runtime pass result is claimed.
- Observability and error handling: Baseline/capture warnings usually identify failed paths, but the horizon reporter acknowledges absent rows and a scan exception leaves the pending window on the normal three-hour schedule. These are addressed inline; no separate logging or metric defect was established.
- Persistence, writes, and atomicity: The shared horizon DELETE/INSERT is not one atomic refresh; checkpoint and baseline operations use separately published internal-table transactions. The new comments and carried P1 IDs identify concrete loss, stale-cache, or delayed-resume windows. FE crashes and handoff were considered in those sequences.
- FE/BE variables, performance, and remaining issues: This FE-centered change adds no new FE-to-BE wire field in the reviewed paths. The reporter's own audited writes cause idle internal write/load churn (P3), and post-reservation scan failures defer capture for the configured interval (P2). No other distinct performance defect survived the final sweep.
User focus: No additional review focus was provided. This is static inspection only; round 2 normal and focused passes found no new distinct candidate, so the review is complete.
Existing P0/P1 findings confirmed for this head: #68499 (comment), #68499 (comment), #68499 (comment), #68499 (comment), #68499 (comment), #68499 (comment), #68499 (comment), #68499 (comment), #68499 (comment), #68499 (comment)
|
/review |
There was a problem hiding this comment.
Review result
Complete static review of PR #68499 at head a16dfc6300af4d92eba8a8e8bba2d85842a137e7: three distinct new inline findings (one P1, two P2). Seven existing P1 findings still apply and are carried by ID rather than reposted. The two review rounds converged with all final normal and risk-focused passes reporting NO_NEW_VALUABLE_FINDINGS; the final changed-file and candidate sweep found no unresolved point. No additional user focus was supplied. No builds or tests were run under the review prompt.
Critical checkpoints
- Goal and proof: The change adds durable SQL plan management, replay, and audit-driven capture. The code and focused test cases exercise many ordinary flows, but the attached resource, paging, and audit-fence failures mean the goal is not fully met. Inspected tests and expected outputs are static evidence, not executed proof.
- Scope and clarity: All 427 changed paths were covered (89 FE main, 43 FE test, 295 regression). The parser, optimizer, decompiler, durable manager, audit pipeline, and integration changes form one feature; the remaining failure contracts are identified in the inline findings and carried threads.
- Concurrency and locks: FE audit workers, loader, capture daemon, baseline refresh, forwarded DDL, and master handoff introduce concurrent execution. Local locks and cache publication were traced alongside their critical variables; database writes and remote visibility probes occur outside those local locks. No new lock-order deadlock was substantiated. Leadership checks and separate writes remain vulnerable to handoff, as described by existing P1 comments 4111362454, 4161713819, and 4164852732.
- Lifecycle: The nested planner statement context is restored without closing its connector scope. The P1 inline finding traces a reachable Iceberg lease from bind to statement teardown. No C++ cross-translation-unit static initialization is involved in this FE-only change.
- Configuration: SPM and capture settings and their read sites were inspected, including the finite default scan overlap and reporting cadence. Dynamic setting changes do not cure the expired positive horizon; no separate configuration propagation defect was substantiated.
- Compatibility: Internal-table initialization, schema upgrade, cache reload, and promotion paths were reviewed for mixed-version and restart behavior. No separate wire or storage-format incompatibility was substantiated; the outstanding handoff and unreadable-write cases are covered below.
- Parallel paths: Direct and forwarded DDL, success and error returns, raw-plan fallback, master promotion, global/session baselines, and replay variants were traced. The nested-context issue reaches successful and failing plans; historical forwarded DISABLE visibility P1 4164852706 remains applicable.
- Conditional checks: Positive-row freshness, snapshot count/fence, status and checkpoint leadership guards, and replay LIMIT/label guards were checked against their caller conditions. The two P2 inline findings show why freshness and count checks are insufficient. Existing self-join LIMIT identity P1 4176748160 still applies.
- Coverage and negative cases: FE tests and regression cases cover ordinary capture, replay, pagination, and many error paths; their static assertions and outputs were inspected. Missing discriminating cases include a live backlogged FE whose positive report expires, reordered equal-ID rows between page SELECTs, and nested connector scope cleanup on successful and failing plans. Historical handoff and delayed-visibility tests also leave the carried P1 windows open.
- Test results: Changed
.outfiles and test assertions were reviewed for consistency, including generated SPM suites. They were not regenerated or executed, so no runtime result or CI success is claimed. - Observability: Capture and baseline logs/metrics were reviewed in their error paths. The reported missed events, stale published status, and pinned leases need the correctness fixes; no additional independently actionable logging or metric issue was substantiated.
- Persistence and failover: Baselines and capture checkpoints use internal-table writes with process-local caches, not a single atomic FE EditLog transaction. Existing P1 comments 4111362454, 4161713819, 4164852706, 4164852732, 4172437852, and 4173514206 remain applicable to leadership handoff, unreadable committed rows, or cross-FE retry; they are not duplicated here.
- Writes, atomicity, and crash paths: Conditional status writes, visibility readback, duplicate-key winner selection, checkpoint UPSERT, and follower refresh were traced. The seven carried P1s and the equal-ID paging finding describe remaining ways durable state and the published cache or capture cursor can diverge. No further distinct crash-only failure was substantiated.
- FE/BE variables: No new FE-to-BE variable-passing defect was found in the changed paths. The reviewed SPM setting transport and query paths yielded no separate protocol finding.
- Performance: Bounded snapshot pages and capture windows were checked with their continuation and retry logic. The paging fix needs stable row identity; no other actionable complexity or hot-path performance regression was substantiated.
- Other issues: Existing P2 threads 4173024998, 4176998367, and 4164852712 still cover the separate stable-snapshot mutation, multi-batch publication fence, and output-label cases. They were not reposted. Other historical points were checked against the current head and either fixed or covered by the IDs above.
This review is complete for the supplied head and is based on static inspection only.
Existing P0/P1 findings confirmed for this head: #68499 (comment), #68499 (comment), #68499 (comment), #68499 (comment), #68499 (comment), #68499 (comment), #68499 (comment)
| sessionVar.enableCTEMaterialize = originalCteMaterialize; | ||
| sessionVar.inlineCTEReferencedThreshold = originalInlineCteThreshold; | ||
| sessionVar.cteInlineMode = originalCteInlineMode; | ||
| ctx.setStatementContext(originalCtx); |
There was a problem hiding this comment.
[P1] Close the nested statement context after baseline planning. An Iceberg bind can load its snapshot through the fresh context installed here; that context's connector scope owns a table-cache lease or tracked table, released only by StatementContext.close(). This finally restores the outer context without closing the nested one, so normal statement teardown closes only the outer context and every such CREATE/refresh can leave an Iceberg resource pinned. Close the nested context in this finally and cover successful and failing nested plans with a scoped closeable.
| if (horizon <= 0) { | ||
| continue; | ||
| } | ||
| if (updatedAt <= 0 || now - updatedAt > ROW_STALE_MILLIS) { |
There was a problem hiding this comment.
[P2] Keep a live follower's positive audit fence when its report expires. A follower can still hold a completed 10:00 event while keepalive upserts fail for five minutes; this branch then drops its previously confirmed positive row, and the leader checkpoints 12:00 with no fence. If the follower loads at 12:01, the next five-minute overlap starts at 11:55 and the event's 10:01 completion also misses the scanner predicate, so capture never sees it. Check FE liveness before discarding a positive row, or fail the capture cycle closed while a live reporter is overdue.
| * so the group can outgrow {@link #SNAPSHOT_PAGE_SIZE} rows - a jump past it would | ||
| * omit the rows behind the first page, possibly the newest durable status. | ||
| */ | ||
| private static final String SELECT_PAGE_SQL = SNAPSHOT_COLUMNS + SPM_BASELINES_TABLE |
There was a problem hiding this comment.
[P2] Give equal-id rows a stable order across snapshot pages. This OFFSET is applied to a separate SELECT ordered only by id. With 1,999 lower-id rows plus old and newer status rows for id 2000, page one can end on the old row; if the next SELECT reverses those tied rows, OFFSET 1 reads the old row again and skips the newer one. The read count still equals the unchanged table's COUNT(*), so refresh can publish the wrong status. Order by a unique row identity or page from a stable snapshot, and test tie reordering between page queries.
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)