Repository navigation
[feature](plan)support sql plan management - #68499
starocean999 wants to merge 49 commits into
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
/review |
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.
Static review of PR #68499
Reviewed exact head b9a97ee7b42c3331352e0b13dbd8ccf2027d64a9 against the supplied authoritative diff. I found 13 new issues (3 P1, 10 P2), all attached inline. Existing P1 comments 4180088111, 4180088064, and 4180088051 still apply to this head and are carried as blockers without reposting. Existing P2 and historical threads were used as duplicate fences. The user supplied no additional review focus.
Convergence: The final permitted round found two distinct issues (stale cross-FE fingerprint adoption and mutable-UNIQUE aggregate elimination). All candidates found within the three rounds have a disposition, but discovery in round three means convergence is incomplete under the required cap. This is the final review submission under that cap. Inspection was static; no build, test, cluster run, or expected-result generation was performed.
Critical checkpoints
- Goal and proof: The feature aims to create and replay safe SQL plan baselines, persist GLOBAL identities through FE handoff, and capture eligible audit queries after publication. The inline wrong-result and publication findings show those guarantees are not yet met. Added FE and regression tests cover ordinary create/replay, command, retry, and capture flows, but do not prove the reported changing-constraint, partition-growth, handoff, and delayed-publication schedules.
- Scope and clarity: Parser/commands, matcher/optimizer/decompiler, baseline manager, and audit producer/loader/scanner/checkpoint roles are separated. The CREATE exclusion mask and the parallel local versus durable pending-create resolvers have inconsistent safety checks. The large fixture update was checked alongside the code paths; no separate clarity-only defect warranted a comment.
- Concurrency and locks: Statement/runtime-manager, audit processor/loader/reporter, capture daemon, and FE leader transitions overlap. Local queue transfers and manager maps have locks/monitors, with internal-table I/O outside short state locks; no new lock-order deadlock was established. Those local locks do not atomically cover audit publication versus checkpoint writes or leadership changes versus shared-table SQL. The three retained P1 threads and the audit inline findings identify concrete schedules.
- Lifecycle and initialization: Nested statement contexts close on success and failure. Pending CREATE markers, committed audit loads, reporter rows, and capture checkpoints can outlive their initiating FE; M7, M10, and M12 identify incorrect retirement/adoption. No cross-translation-unit static initialization applies to these Java changes.
- Dynamic configuration: Session rule masks, capture thresholds, and global
time_zoneare read during relevant operations; pending capture windows pin their scan context. M8 shows two reads of mutabletime_zonecan register and render the same audit row under different zones. No other independently supported dynamic-change defect survived review. - Compatibility: Fresh and upgrade paths for the internal sequence and capture tables were inspected. M6 identifies a 4096-character sequence digest column that cannot hold a valid long bind digest. No additional rolling-upgrade or function-symbol issue was substantiated.
- Parallel paths and conditions: GLOBAL/SESSION CREATE, normal and ambiguous retry, ALTER/DROP, forwarding, frozen/raw replay, EXPLAIN/fallback, and audit success/timeout/close/crash were compared. M12 is the cross-FE retry branch missing the local fingerprint check; M9 is an incomplete HTTP-200 body incorrectly treated as publication. The guards involved have comments in several paths, but these differing conditions remain incorrect.
- Tests and expected results: FE unit tests, SPM regression suites, and changed benchmark SQL/output fixtures were inspected for positive, negative, and end-to-end cases. Several new findings need discriminating tests described inline. Static inspection did not validate runtime results, and no result file was regenerated.
- Observability: Existing logs/metrics carry baseline IDs, capture windows, retries, and release events. False publication confirmation, stale-plan adoption, and wrong-result replay can still appear successful; no separate logging-only finding was established. The inline issues identify the missing correctness checks.
- Persistence and replay: GLOBAL baselines, sequence reservations/markers, audit horizon/zone rows, and capture checkpoints use separate internal-table writes. Follower refresh and promotion reload were traced. M5-M7/M12 and the retained handoff P1 threads show unresolved identity/visibility cases; M2-M4/M9-M10 show audit coverage can be lost before a durable checkpoint. No separate EditLog replay bug was substantiated.
- Data writes and crashes: Baseline rows, sequence markers, audit loads, horizon reports, and checkpoints are not one atomic transaction. Commit-versus-publication delay, FE death, and leader handoff produce the concrete failures in the inline set and existing blockers. No BE visible-version or MoW delete-bitmap path is changed here.
- FE/BE variables, memory, and performance: No new FE-to-BE variable transport or BE memory/nullable-column path was found in this Java-centered change. Queue, page, and retry budgets bound most work, but M4 documents the correctness cost of zone eviction. No additional actionable CPU, memory, or lock-duration regression was established.
- Other issues: The final changed-file and candidate sweep found no unresolved candidate. Related ideas that were already in live threads were deduplicated; the nonempty partition-predicate case is additional evidence for the same rule omission as M11.
Existing P0/P1 findings confirmed for this head: #68499 (comment), #68499 (comment), #68499 (comment)
|
/review |
There was a problem hiding this comment.
Review status
Static review of all 435 changed files at head 28070c3f5c6d59f102b1dbde3c0e28aa87656790: 16 new inline findings (3 P1, 13 P2). Three earlier P1 threads still apply: #4180088111 (unfenced old-status DELETE after leader handoff), #4180088064 (CREATE leader check separated from INSERT), and #4180088051 (unconfirmed CREATE marker lost or unreadable across handoff). They are carried by ID without duplicate comments. Other existing threads were rechecked against this head and either remain as already reported P2 context or are fixed/not substantiated here.
Three normal and risk-focused review rounds covered planner/replay, persistence/DDL/schema, and audit/capture, including unit and regression fixtures. The final round produced four new findings in persistence, planner replay, and audit capture, so convergence is incomplete under the three-round cap. Every identified candidate has nevertheless been validated, deduplicated, and either included inline or dismissed with code evidence. The user focus file specifies no additional focus.
Critical checkpoint conclusions
- Goal and proof: The PR adds SPM baseline creation, audit capture, durable storage, and frozen replay. The accepted wrong-row, stale-state, and missed-capture cases show that those contracts are not yet met. Existing tests cover positive and several failure paths, but do not prove the boundary schedules in the inline findings.
- Scope: The 435 changed paths are FE implementation/tests and SPM regression suites/results; the feature is broad but no unrelated product change was found.
- Concurrency and locks:
writerLockserializes local baseline writes with refresh,stateLockprotects the cache, and audit loader/reporter/capture threads use their own state guards. Internal-table I/O avoids the short cache lock; no separate lock-order deadlock was substantiated. Locks do not fence delayed table publication or leader handoff, as described below. - Lifecycle: Load, refresh, cache invalidation, writer-zone reporting/retirement, checkpoint continuation, and leader takeover were traced. The post-DDL refresh and zone-retirement findings identify remaining lifecycle errors; no separate resource-release defect was substantiated.
- Configuration: Global capture thresholds, interval, time zone, and refresh interval are sampled by their daemons; pending capture windows pin their filter. No additional configuration propagation failure was substantiated.
- Compatibility: Fresh and upgrade internal-table schema paths, persisted frozen SQL, checkpoint legacy fields, and mixed FE operation were checked. The schema fingerprint width and frozen SQL parsing findings remain. No new FE-BE variable or protocol field is introduced.
- Parallel paths and conditions: CREATE/ALTER/DROP/SHOW, master/follower and forwarded DDL, manual/captured plans, and UTC/DST audit scans were followed. The LIMIT early return, sort-key scope, and marker-recursion conditions have the reported counterexamples.
- Tests and expected results: The changed FE tests, Groovy suites, and
.outfiles were statically inspected; their execution and generated-result provenance were not verified. Add discriminating fixtures for the concrete inline examples, including delayed publication and zone changes. - Observability: Retry/horizon/refresh logging and capture metrics were reviewed; no stand-alone observability defect was substantiated. Several accepted wrong-result or missed-row paths can otherwise look like successful operations.
- Persistence, EditLog, and failover: Durable SQL writes, snapshot reload, journal sync, and leader transition were traced. Three existing P1 handoff findings still apply; new comments cover stale readback, ambiguous writes, and post-success refresh rollback.
- Data writes and crashes: Baseline INSERT/DELETE/status transitions and audit publication have separate commit and visibility phases; the accepted findings show where cache/checkpoint state can diverge. No additional crash-only failure was substantiated.
- FE-BE and Cloud: This diff changes no BE/Cloud source or FE-BE transmitted variable; the reviewed behavior is FE-side.
- Performance and other issues: Cache indexes, bounded audit pages/lookback, and snapshot pagination were inspected. No independent performance regression or other supported issue remained after deduplication.
No build or test was run, as required by the review prompt.
Existing P0/P1 findings confirmed for this head: #68499 (comment), #68499 (comment), #68499 (comment)
|
/review |
There was a problem hiding this comment.
Review status: INCOMPLETE convergence under the three-round limit. The final round produced two new valuable findings. This submission reports every substantiated issue found in the permitted rounds: 12 new inline findings (5 P1, 7 P2). Existing P1 comments 4180088111 (late status cleanup after leader handoff) and 4180559967 (same-table occurrence identity in the LIMIT guard) still apply to this head and are carried as blockers without reposting.
Scope and user focus. Static inspection covered all 435 changed paths at head f511d3a: 92 FE production files, 46 FE test files, and 297 regression assets. The supplied focus file has no additional focus point. I compared all proposed issues with the existing live inline threads and checked their anchors against added lines in the authoritative diff.
Critical checkpoint conclusions (code-review skill Part 1.3):
- Goal and proof: The change adds SQL Plan Management creation, matching, frozen-plan replay, persistence, forwarding, and audit-driven capture. Existing FE and regression tests exercise many paths, but the inline findings show that current tests do not prove limit equivalence, durable leader fencing, publication completeness, or precise payload admission.
- Scope and clarity: The changed code is organized around the feature and its test assets, but correctness depends on multiple FE and OLAP-table boundaries. I found no separate unrelated change to report.
- Concurrency, locks, and heavy work: BaselineManager writerLock/stateLock protect local cache publication, and audit loader batch transfer uses its monitor; SQL and stream-load work is generally outside the short state-lock sections. These local locks cannot serialize mutations from different leaders or make an INSERT SELECT source predicate hold through commit. The captured transaction and audit timelines produce data loss or stale state. I found no separate lock-order or deadlock issue.
- Lifecycle and initialization: I traced SESSION reset/forwarding, baseline reload and promotion, capture pending windows, audit reporter startup/close, and committed-batch fences. The accepted comments identify follower refresh, first-report, window-completion, and fence-capacity gaps. Cross-translation-unit C++ initialization is not involved.
- Configuration: FE global capture and audit settings are read by the daemon/loader; supported short load timeout and small batch settings make the fence-capacity loss reachable. I found no additional dynamic-setting propagation defect.
- Compatibility and storage format: I checked fresh and upgraded internal-table schemas, duplicate-key baseline rows, unique-key checkpoint rows, frozen SQL provenance, and forwarded SESSION payloads. No new FE-to-BE transmitted variable or separate mixed-version break was substantiated. Journal synchronization does not establish OLAP-row publication for the forwarded CREATE path.
- Parallel paths and conditional checks: I traced all LIMIT guard branches, frozen and fallback replay, CREATE/ALTER/DROP local and forwarded paths, immediate and periodic horizon writes, remote reads, and scanner completion predicates. The inline comments describe the conditions that accept wrong results or allow a checkpoint to pass unpublished rows.
- Tests and expected results: The PR changes 46 FE test files, 154 regression suites, and 143 output assets. I inspected test logic and representative fixtures but did not run tests, build Doris, or regenerate outputs, as the review prompt prohibits execution. Missing discriminating cases include reversed source-scan/commit order, same-second tied identities, missing/moved LIMITs, mixed audit horizons, first report failure, 257 pending fences, and the SESSION payload boundary. No test result is claimed as independently validated.
- Observability: The new code logs failed writes, pending windows, and early fence removal and adds FE metrics. Those signals aid diagnosis but do not prevent the reported loss paths. I found no separate actionable logging or metrics defect.
- Transactions, persistence, writes, and crashes: I traced the FE insert executor into commit and OLAP partition-version assignment. A source SELECT can qualify before another leader's write while its own transaction commits afterward; neither the epoch comparison nor the status-row presence test is a commit-time fence. Duplicate-key pagination also lacks a total order for tied identities. These are the primary durability findings.
- Memory, performance, and other correctness: The PR changes FE logic and no BE allocator or nullable-column implementation. I did not substantiate a separate memory or hot-path performance regression. All remaining suspected points were either covered by live threads, folded into one of the inline findings, or dismissed after tracing upstream and downstream guards.
The review execution is complete through the mandated final sweep, while convergence remains INCOMPLETE because two new valuable findings appeared in round three.
Existing P0/P1 findings confirmed for this head: #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 complete for apache/doris #68499 at b2b9dc4. Request changes: seven distinct new inline findings (one P1, six P2) are attached. Existing P1 threads 4182767937, 4182767933, and 4181409981 remain applicable and are carried as blockers without reposting. The prior checkpoint-overwrite P1 is fixed by append-only rows; the new reservation gap is separate. Existing P2 writer-zone registration feedback was deduplicated. No additional focus was specified in review_focus.txt.
Critical checkpoints:
- Goal and proof: the PR adds SQL Plan Management, including baseline DDL, frozen-plan replay, automatic audit capture, and tests. The feature has broad static test coverage, but its durable and capture contracts are not yet fully met in the seven cases below; no test was executed in this review.
- Scope and clarity: all 435 changed paths were swept (90 FE runtime, 46 FE tests, 2 parser, 297 regression). The cross-layer feature is necessarily broad; no separately actionable size or abstraction defect was established.
- Concurrency and locks: baseline writer/cache locks and local audit fence synchronization were traced. Local locking does not fence FE handoff, publication delay, or heartbeat state; M-D2, M-A, M-B, and M-F cover concrete races. No independent lock-order or deadlock defect was established.
- Lifecycle and initialization: promotion reload, audit producer/reporter/loader, fence retirement, and checkpoint reservation were traced. M-C, M-E, and M-F identify lifecycle gaps; no separate static-initialization issue was established.
- Configuration: session SPM variables and forwarding were checked, along with changing time zones and capture filters. M-D shows the visibility probe does not retain the writer zone. No separate dynamic-configuration propagation failure was established.
- Compatibility: new internal tables and append-only checkpoint columns, legacy-row parsing, and FE forwarding were inspected. No supported pre-existing SPM table migration failure was established from this base-to-head diff.
- Parallel paths: GLOBAL/SESSION baseline, SELECT/EXPLAIN, forwarded DDL, fallback replanning, audit load and capture paths were checked. No separate unreported result bug survived the second pass.
- Conditions and errors: row-cap, parser, policy, and publication checks were traced. The existing row-cap P1 remains; zero-report and failed-heartbeat conditions admit the new audit gaps. No additional silent-error path was substantiated.
- Test coverage: FE and regression cases exercise many ordinary, negative, and replay paths. They do not discriminate the delayed DELETE/by-key CREATE, fresh-zero acceptance, two-age overflow, late transaction publication, or two-reservation handoff scenarios attached inline.
- Expected results: changed regression .out files were statically inspected; no specific incorrect expected row was established. Their generation provenance and runtime result could not be verified without executing tests.
- Observability: new logging and metrics were inspected; no separate missing diagnostic was established. The disputed publication/retirement decisions need stronger correctness gates regardless of logging.
- Persistence and transactions: durable baseline rows, tombstones, audit stream-load COMMITTED-to-VISIBLE retry, and capture checkpoint writes were traced. M-D2, M-E, and M-F show that publication and failover remain correctness boundaries; no BE visible-version change is in this PR.
- Writes and crashes: DDL, stream-load, and checkpoint recovery paths were checked for ambiguous success and leader change. The inline timelines cover concrete incorrect outcomes; no independent leak or deadlock was established.
- FE/BE transport: no new FE-to-BE variable requiring a matching BE send path was identified. Session forwarding was checked separately; no additional gap was established.
- Performance and remaining risk: bounded scans, fence caps, and retry queues were reviewed. M-C shows the 256-entry cap aggregates deadlines incorrectly. No separate measured performance claim is made.
This review used the authoritative bundled diff, existing threads, two normal full-review rounds, separate risk-focused rounds, and a final changed-file/anchor sweep. The second round produced no new distinct findings. Validation is static only: no builds, tests, or product-source edits were performed under the review contract.
Existing P0/P1 findings confirmed for this head: #68499 (comment), #68499 (comment), #68499 (comment)
|
/review |
There was a problem hiding this comment.
Review opinion: Request changes. Static review of PR #68499 at head 99558b9083df001bacc839b06b935c05fa5d4c93 found 15 new issues (6 P1, 9 P2), all attached inline. Four existing P1 comments remain applicable and are carried by ID without reposting: 4190197774 (earlier capture reservation can be pruned), 4182767937 (LIMIT slice identity), 4182767933 (status INSERT can race DROP), and 4181409981 (a successful DISABLE can be undone by refresh after its five-minute fence expires). The supplied review focus had no additional points.
Convergence status: incomplete. The third and final permitted review round found two new valuable issues, the adopted-window zone credit and the unbounded CREATE watermark read. All discovered candidates have been verified, deduplicated, accepted, or dismissed, and the changed-file sweep is complete, but another convergence round was prohibited by the review contract.
Critical checkpoint conclusions
- Goal and proof: The PR adds SQL Plan Management capture, baseline storage, matching, and replay. Manual plan replay can change the caller's filters, projected values, scan source, order, or result headers; the present tests do not prove those contracts for the inline counterexamples.
- Scope and clarity: The 435 changed paths span parser/planner rules, commands, persistence, audit capture, FE integration, unit tests, and regression cases. The cross-layer changes serve the feature, but their interdependent state makes the unresolved cases material.
- Concurrency and locks: I traced the baseline writer/state locks, audit loader monitor, queue handoffs, daemon refreshes, and FE handoff. No separate lock-order defect was substantiated. Separate internal-table commits and cross-FE publication remain unfenced in the cited findings and existing P1 threads.
- Lifecycle: Baselines can outlive sessions or restart on a new leader; audit events pass through queues, stream load, shared horizon reports, and capture checkpoints. The dead-FE fence, missing pre-commit obligation, and adopted-window zone state have unresolved lifecycle gaps (see the inline audit and capture comments).
- Configuration: SPM session switches, forwarding flags, capture settings, and the refresh interval were checked at their read sites; no separate dynamic-setting defect was substantiated.
- Compatibility: New internal tables, appended checkpoint columns, and older-row parsing were inspected. No distinct rolling-version incompatibility was substantiated; schema identity checks still do not establish manual plan equivalence (see the inline scan-source comment).
- Parallel paths: Both frozen and parameterized replay retain the manual-plan semantic gaps. GLOBAL and SESSION operations, live and dead audit producers, and refresh/adoption paths were compared; route-specific failures are called out inline.
- Special conditions: Placeholder, scan-selector, cap, tombstone, publication-age, and pending-window guards were followed through their callers. Several guards accept a semantically different plan or treat elapsed time as a terminal write outcome (see the inline guard and publication comments).
- Test coverage: Changed unit and SQL suites cover many ordinary, fallback, and failover cases, but lack the concrete negative and delayed-publication schedules in the comments. No new test was run as part of this static review.
- Expected-result files: Regression output changes were inspected with their suites. Their presence is not independent execution evidence, and no separate result-file inconsistency was substantiated.
- Observability: Existing logs, IDs, and audit labels help trace decisions. They cannot recover an audit commitment that dies before its separate durable horizon report (see the inline crash-gap comment); no additional logging-only finding was substantiated.
- Transactions and persistence: Internal SQL rows and append-only checkpoints carry durable state, while visibility can lag commit. False DROP tombstones, finite CREATE/status fences, checkpoint/zone rollback, and existing status/DROP races remain unresolved (see the inline persistence and capture comments and the carried P1 threads).
- Writes and crashes: A failed DELETE can later remove a live row, and FE death or delayed audit publication can let capture checkpoint past an event (see the inline DELETE and audit comments). These are independent of successful local cache updates.
- FE/BE variables: SPM planning flags and enabled SESSION baselines were traced through forwarding; no missing FE-to-BE variable path was substantiated. Audit stream-load status and the separate FE horizon write form the audit crash gap.
- Performance: Historical DROP-tombstone reads on every refresh and the reservation-history MAX on every GLOBAL CREATE grow without a bounded lookup (see the two inline scalability comments). Their exact timeout threshold is workload dependent.
- Other paths: The final changed-file and unresolved-candidate sweep found no further independently substantiated issue. The cloud-mode CREATE candidate was dismissed because CREATE overrides the same rejection hook as ALTER/DROP; another audit idle-zero candidate was dismissed because the internal report itself records a writer zone.
Validation: Read-only/static inspection only. No build, test, or source edit was performed under the supplied review contract.
Existing P0/P1 findings confirmed for this head: #68499 (comment), #68499 (comment), #68499 (comment), #68499 (comment)
|
/review |
|
Codex automated review failed and did not complete. Error: Review recovery stopped: Could not verify exactly one final review from this invocation Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
|
/review |
There was a problem hiding this comment.
Review of PR #68499 at head 2b85ae1. I reviewed all 438 changed paths and their authoritative diff, related production flows, changed tests and golden outputs, and existing inline threads. This was static inspection only; the review instructions prohibited builds and test execution. No additional user focus was supplied. I request changes for the P1 correctness and durability findings below.
Review status: three of three permitted rounds are complete, but convergence is incomplete. A distinct same-node audit-restart failure surfaced in the final focused round. The round cap prevents another convergence pass; all candidates found so far were validated, deduplicated, and either included inline or carried from an existing thread.
Checkpoint conclusions:
- SQL behavior: manual baseline CREATE still accepts plans that change JOIN/source topology, result-forming operators, or filter attachment; replay can return different rows. Root output, source-name, and predicate-text checks do not establish relational equivalence. Existing LIMIT input/order and fully qualified source-identity concerns also remain.
- Identity and persistence: the compact id watermark can lag a visible reservation; same-second tombstone ordering can hide a newer CREATE. Forwarded duplicate CREATE confirmation, DROP/status interleaving, and stale checkpoint writers remain covered by existing P1 threads. Checkpoint pruning can remove a readable pending window before its replacement is visible.
- Audit and lifecycle: concurrent reports and same-node restart can erase confirmed load labels; overflow drops a contributing label; clean close drops historical rendering zones. Each can let capture advance past an audit row. The checkpoint table upgrade can lose pending state or leave an old model after a transient DROP failure.
- Compatibility, performance, and operational paths: I traced upgrade/restart, leader handoff, follower refresh, FE-to-BE audit load outcomes, retry and cleanup, and the compact-watermark performance change. The findings above describe the substantive remaining failures in these paths; I found no separate new performance issue. Existing P2 threads were treated as duplicate fences, not reposted.
- Test evidence: the changed FE tests and regression suites were inspected for relevant scenarios, but none were run. The reported publication, restart, migration, and manual-equivalence counterexamples need discriminating tests; inspected tests and author or CI claims are not independent execution evidence.
Still-applicable existing P1 inline comments, carried without duplication: 4190857510 (forwarded duplicate CREATE identity), 4190857552 (fully qualified source mismatch), 4182767937 (LIMIT input/order equivalence), 4182767933 (status INSERT after DROP), and 4182767927 (stale checkpoint writer). Earlier P1 examples 4190857564, 4190857549, 4190857545, 4190857503, 4190197774, 4182767947, and 4182767914 were addressed in their original form at this head and are not carried.
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.
Review opinion: Request changes. At d774220, I found six new P1 and two new P2 issues in manual plan replay, idle audit capture, and forwarded duplicate CREATE. The six already reported P1 issues listed below still apply; I have carried their comment IDs instead of reposting them. This is a complete static review after two convergence rounds and a final sweep of all 439 changed paths. The focus file supplied no additional focus points.
Critical checkpoint conclusions
- Goal and proof: The PR wires manual and captured SPM plans through FE planning, persistence, audit capture, and SQL syntax. The new tests exercise many standard and prior failure paths, but the result-changing manual-plan cases and idle capture liveness path in the inline comments prevent the feature from meeting its correctness goal.
- Size and clarity: The change is broad across 439 paths, including generated regression outputs. The principal risk is in the new cross-layer contracts and incomplete comparison branches; I found no separate smallness or readability blocker.
- Concurrency and locks: I traced the audit worker/reporter, baseline refresh/writer, and leader checkpoint interactions, including lock and I/O boundaries. The carried status DELETE/INSERT and checkpoint races remain across handoff. I found no additional lock-order deadlock or heavy-I/O-under-lock issue to report.
- Lifecycle: Startup registration, restart restoration, close, promotion, pending windows, and cleanup were checked. A confirmed idle zero report goes stale without keepalive; the prior FE-row restoration and checkpoint-prune P1s also remain.
- Configuration: Global capture interval and filter settings are read for subsequent capture cycles, and SPM session settings were traced through normal and fallback planning. I found no separate dynamic-setting defect.
- Compatibility: The checkpoint table upgrade has a remaining failed-restore path covered by existing P2 4193831863. Frozen versus raw SQL mode and rolling leader handoff were included in the review; the handoff blockers below remain.
- Parallel paths and conditions: Manual versus captured baselines, frozen versus parameterized replay, local versus forwarded DDL, and active versus idle reporters were checked. The inline findings identify distinct payload/alias checks missing from manual-plan validation and duplicate-CREATE confirmation.
- Test coverage: FE unit tests and regression suites cover many earlier cases, but lack the manual TVF, mark/ASOF, nested alias, generated header, forwarded duplicate-CREATE, and full idle-capture scenarios described inline.
- Test results: I inspected changed test code and expected outputs statically. No build, test, or runtime validation was run under the review contract, so these are code-path conclusions rather than executed results.
- Observability: Capture counters and failure logs, plus baseline/audit retry logs, were inspected. I found no distinct missing metric or logging issue.
- Persistence and failover: The compact ID watermark can still hide an unreadable reservation (4193831790); a completed DISABLE/DROP can lose its mutation fence (4181409981); status cleanup can delete a later leader's row (4180088111); and a delayed status INSERT can outlive DROP when its best-effort tombstone is unavailable (4182767933).
- Data writes and crashes: Prior FE audit obligations can be overwritten after restart (4193831833), and pruning can erase an earlier unconsumed checkpoint after delayed publication (4193831820). Those existing threads cover the write/crash risks without duplicate inline comments.
- FE/BE state: This diff adds no distinct BE-side variable transport to pair with the FE session changes; I found no separate propagation issue.
- Performance and remaining issues: I reviewed the allocator, refresh, capture paging, and regression breadth without establishing another distinct issue. The eight inline comments and six carried P1 comments are the substantiated blockers.
Completion: Round 1 used three full-coverage and three additional risk-focused reviews. Round 2 repeated full planner, persistence, and audit coverage plus an independent risk check; every round-2 agent returned NO_NEW_VALUABLE_FINDINGS. The final sweep found no unresolved candidate. Source files were not modified.
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 status
Static review of PR #68499 at 37a8b831cbb9b13b9eeccee832352bcf43c01595. I found 17 distinct new issues (12 P1, 5 P2), all attached inline. Three previously reported P1 issues remain applicable and are carried by ID below without reposting. Review incomplete under the three-round limit: the final round found new durable-state and capture issues, so the required no-new-finding convergence was not reached. The P1 issues block approval.
Critical checkpoints
- Goal and proof: The change adds SQL Plan Management baseline DDL, matching and frozen replay, plus audit-driven capture. The ordinary path and many edge cases have FE unit and regression coverage, but the concrete cases in the inline findings are not proved safe by those tests. I inspected tests and expected results only; no build, test, or live cluster execution was permitted or performed.
- Scope and focus: The 440 changed paths comprise 89 production Java files, 51 FE test files, 154 regression suites, 143 expected-output files, two grammar files and one README. The changes are feature-related across planner, persistent state and audit capture, though the cross-layer scope is large. The user supplied no additional focus points.
- Concurrency and locks: Global DDL serializes writer I/O with
writerLockand mutates the cache understateLock; audit loading/reporting and the leader capture daemon add concurrent publication and checkpoint transitions. I found no independent lock-order deadlock. Completed-but-unreadable writes, old mutation fences and leader turnover still produce the status, DROP and capture failures reported inline and in existing threads. - Lifecycle and cleanup: I traced CREATE/ALTER/DROP, follower forwarding, cache invalidation/reload, leader promotion, checkpoint reservation/adoption/prune, and audit fence retirement. M1, M2, M5, M12, M14, M17 and existing blockers show that several state obligations do not survive or resolve those transitions correctly. No BE or C++ static-initialization/ownership change is in this diff.
- Configuration and dynamic changes: Capture rereads global filter and interval settings; M19 shows a failed checkpoint reread can still leave the configured three-hour sleep. The global SQL mode affects internal SQL parsing; M8 and M18 identify writes whose escaped text is not pinned to the expected mode.
- Compatibility, transactions, writes and crashes: I inspected the new append-only baseline sequence/HWM and capture checkpoint schemas, migration, edit-log/visible-version ordering, and successor reloads. M8, M18 and the carried P1 checkpoint/identity threads show persistence or failover gaps. The confirmed-VISIBLE ordering was also used to dismiss an unsupported stale-empty DROP hypothesis. No further distinct rolling-upgrade or FE-to-BE variable-passing failure was established.
- Parallel paths and conditions: SESSION/GLOBAL storage, local/forwarded DDL, manual/automatic baseline creation, EXPLAIN and failed-replay fallback were inspected. M3, M4, M7, M13 and M16 give concrete parsed-plan pairs that still pass guards and change rows, values or output order. Several other apparent gaps were rejected after tracing cloud gating, placeholder checks, digest fields and buffered HTTP behavior.
- Tests and results: The changed FE tests and regression suites cover ordinary DDL, matching, capture and several handoff cases. They do not discriminate the accepted cases, including VALUES shape, follower fence supersession, tombstone SQL-mode round trip and failed checkpoint-reread cadence. Expected-output files were inspected, not independently generated or executed in this review.
- Errors and observability: The new paths log and expose some retries and capture counters, but M5 swallows a required tombstone write error after a successful DROP, M17 reports a later forwarded success while a stale fence masks its row, and M19 logs a prompt retry without scheduling it. No additional standalone metric or logging finding survived the sweep.
- Performance and other issues: M6 leaves a per-CREATE scan of append-only sequence history with a fixed timeout. Existing P2 threads already report separate unbounded tombstone and watermark reads; they were not reposted. No other distinct issue survived the final changed-file and candidate sweep.
Existing blocking comments carried forward
4193831833: a restarted FE can overwrite unresolved prior audit labels/zones with an empty report.4193831820: checkpoint pruning can delete an earlier pending reservation before a successor adopts it.4193831790: separate HWM, sequence and baseline writes can permit ID reuse at handoff.
These three P1 threads still apply at this head and appear in existing_blocking_comment_ids. Other still-applicable P2 threads remain in the existing review and were not duplicated.
Existing P0/P1 findings confirmed for this head: #68499 (comment), #68499 (comment), #68499 (comment)
|
/review |
There was a problem hiding this comment.
Review opinion
The PR adds SQL Plan Management (SPM) baseline DDL, persisted and session baselines, plan matching and frozen replay, plus audit-driven automatic capture. I found 13 new actionable issues (9 P1, 4 P2) across replay semantics, baseline handoff, and audit/checkpoint publication. The new P1s can return another column's value, discard eligible audit rows, or let completed baseline operations diverge from durable state. I also independently confirmed that existing P1 comments 4197109708 (a successful DROP can lose a failed tombstone append) and 4193831833 (a restart report can erase a carried publication obligation after its read fails) still apply; I have not reposted them.
Review execution reached the requested three-round limit. A distinct root set-operation header issue was found in Round 3 and is included inline. Convergence is incomplete under the prompt's iteration rule, although all 13 identified candidates were independently validated, deduplicated, and included in this review. The authoritative changed-file list and diff agree on 445 paths; the final sweep found no unresolved specific candidate. The focus file specifies no additional user review focus.
Critical checkpoint conclusions
- Goal and proof: The intended result is caller-equivalent replay and loss-free, durable automatic capture across FE handoff. The code meets many ordinary-path requirements, but the inline row-value, result-header, publication, and baseline identity cases show that goal is not fully met. The added tests cover many normal and earlier failure paths, but none proves the newly identified interleavings or replay variants end to end.
- Scope and clarity: This is a broad FE feature spanning parser, optimizer, DDL, internal tables, audit ingestion, capture, and 350 test/regression paths. The module boundaries are understandable and generally use adjacent conventions; the independently committed state transitions still require stronger common reconciliation. I found no separate refactor-only blocker.
- Concurrency and locks: Leader DDL, demoted in-flight writes, background load/refresh, query matching, audit workers/reporters, and the capture daemon can overlap. The baseline
writerLockserializes local mutations andstateLockprotects published indexes; audit queue/in-flight and loader monitors cover normal handoffs. One accepted P2 performs up to ten seconds of tombstone SQL understateLock.writeLock, blocking matcher reads. I found no additional lock-order deadlock from the inspected paths. - Lifecycle and ownership: FE promotion, restart, loader close, pending stream-load publication, and checkpoint migration must carry obligations until resolved. The accepted findings and existing threads show several obligations lost at these boundaries. Session baseline state follows
ConnectContextreset/import; no separate ownership or C++ cross-translation-unit static-initialization issue applies to these FE-only changes. - Configuration: The SPM session switches are read during planning; capture settings are refreshed per cycle and active windows pin their filter. No separate dynamic-setting propagation defect was substantiated. The accepted
SET_VARissue is a semantic caller hint removed before its planner application, not a setting refresh delay. - Compatibility: The new internal-table columns, old checkpoint model, SQL-mode handling, and frozen-plan fallback have compatibility paths. The remaining old-model checkpoint retry defect is already raised in live P2 thread 4193831863. I found no new FE-to-BE wire-format incompatibility in this diff.
- Parallel paths: I traced frozen and parameterized fallback replay, ordinary SELECT/EXPLAIN replanning, GLOBAL and SESSION baselines, cached and cache-miss DDL, audit queue/stream-load publication, and first/resumed capture windows. The accepted issues identify the parallel paths that still differ; the other inspected paths yielded no distinct issue beyond existing threads.
- Conditional checks: Namespace/schema, view privilege, scan selector, output, limit/order, and unsupported-decompiler guards have explicit fallback behavior. The cited failures instead arise where a nonempty stale compact row suppresses history, a best-effort or failed report permits progress, age alone expires a committed fence, and a status/cleanup read omits another identity. These branches need the guarantees described inline.
- Test coverage: Static unit and regression cases cover matching shapes, ordinary DDL, paging, retries, and some handoff and upgrade paths. Missing discriminating cases include quoted dotted manual-plan columns, caller SET_VAR replay, root/join-star headers, failed marker/compact/report writes, late same-ID rows, a late old checkpoint reservation, and committed overflow publication. More failure-interleaving tests are needed for those contracts.
- Test results: This was static inspection only, as required by the review prompt. I ran no build or test, generated no regression
.outfile, and cannot claim the changed expected results were executed or independently validated by a running cluster. - Observability: Baseline IDs, retry/fallback reasons, audit fences, checkpoint failures, and capture metrics have log or metric coverage. Those signals do not restore a missing durable obligation. I found no additional observability-only issue worth a separate inline comment.
- Persistence and replay: SPM global state uses internal duplicate-key baseline rows, sequence history/compact rows, tombstones, publication-horizon rows, and append-only checkpoints rather than an SPM EditLog record. The accepted CREATE/DROP/ALTER and capture findings show that separate writes and readbacks do not yet make every handoff or ambiguous-commit outcome equivalent across FEs.
- Writes, crashes, and atomicity: Baseline identity/row/marker operations and audit fence/load/checkpoint operations are not one atomic transaction. A crash or delayed publication can therefore expose the specific duplicate identity, revived DROP, or skipped audit window cases documented inline. No additional BE rowset, visible-version, delete-bitmap, or memory-allocation path changed in this review scope.
- FE-to-BE state: Stream-load labels and transaction-state probes are the FE/BE publication bridge here; the pre-send shared fence must be confirmed before a request can outlive its FE. I found no separate newly transmitted planner variable requiring a missing thrift send path. The caller's
SET_VARloss happens earlier, when the FE swaps the parsed tree. - Performance and remaining risks: The accepted cache-lock SQL is a direct query-planning stall. The unbounded pending-identity lookup concern is already in live P2 thread 4197109722 and was not reposted. Apart from the accepted and already-reported points, no other concrete correctness, lifecycle, compatibility, or performance failure survived the final changed-file and candidate sweep.
Existing P0/P1 findings confirmed for this head: #68499 (comment), #68499 (comment)
| } else if (item instanceof UnboundAlias) { | ||
| expression = ((UnboundAlias) item).child(); | ||
| } | ||
| return expression.toSql(); |
There was a problem hiding this comment.
[P1] Preserve identifier component boundaries in manual-plan output comparison. For a table with columns a.b and b, bind SELECT `a.b` FROM t a and manual plan SELECT a.b FROM t a read different columns. Both parsed slots render as a.b here, so the output and projection checks accept the pair; matching then uses the bind tree and frozen replay reads b for callers selecting a.b. Compare the parsed slot structure or a boundary-preserving digest before accepting the plan.
| // with a different limit is rewritten with the USER limit | ||
| // the frozen sink pinned the CAPTURED output labels: expose the caller's | ||
| // own ones (a value variant must not report the captured header) | ||
| LogicalPlan replay = SPMPlanTreeSupport.mergeLimits( |
There was a problem hiding this comment.
[P1] Apply the caller's SET_VAR before replay planning. tryRewritePlan replaces the parsed statement before EliminateLogicalSelectHint applies its hint; the frozen SQL has no hint, and the fallback strips SET_VAR too. A caller in a UTC session using SET_VAR(time_zone='+08:00') can therefore match an identical baseline but run from_unixtime(epoch_col) under UTC, changing result values. Carry the caller hint into the replay context, including nested blocks, or skip rewrite for semantic SET_VAR hints.
| try { | ||
| SqlModeHelper.withSqlMode(SqlModeHelper.MODE_DEFAULT, () -> { | ||
| try { | ||
| checkpointWriter.write(CHECKPOINT_PRUNE_SQL, params); |
There was a problem hiding this comment.
[P1] Preserve late pending reservations during checkpoint pruning. A previous leader can start an earlier-window INSERT that commits only after this leader's reservation is visible. If that old row publishes between writtenCheckpointVisible and this DELETE, the lower-epoch predicate removes it before adoptEarlierPendingCheckpoint can read it. The new leader then scans its later window and permanently skips the earlier unconsumed prefix. Reconcile pending rows before pruning and retain any unconsumed earlier reservation.
| // had already deleted; the failure still propagates (retryable), and the | ||
| // retry takes the cache-miss path which deletes the lingering rows. | ||
| removeCachedBaseline(id); | ||
| recordPendingMutationFence(id, null, 0); |
There was a problem hiding this comment.
[P1] Retain the deleted identity when cleanup fails. persistDeleteByIdentity has already confirmed this row absent, but if wipeDurableRowsById throws, this catch records only a null-identity fence and skips noteDroppedSeqState. A retry sees no row, so it cannot append the tombstone; a delayed status INSERT from the old master can revive the baseline after handoff. Keep the confirmed identity as a pending tombstone obligation across this error and retry.
| if (idAllocatorStoreForTest == null && !persistenceEnabled()) { | ||
| return; | ||
| } | ||
| List<BaselinePlan> rows = readPersistedById(id); |
There was a problem hiding this comment.
[P1] Do not finish DROP while a competing CREATE of this ID can still publish. After handoff, B can cache its ID N row while old leader A's different ID N INSERT remains unreadable. This by-ID cleanup read sees only B's confirmed deletion, so DROP tombstones B and returns success. A can publish later under N; its different bind/plan identity does not match B's tombstone, and refresh revives the dropped ID. Fence the entire consumed ID or make reservation/collision resolution exclude late foreign rows before confirming DROP.
| if (droppedPublishFenceTime == 0) { | ||
| return 0; | ||
| } | ||
| if (publishFenceNow() > droppedPublishFenceUntil) { |
There was a problem hiding this comment.
[P1] Keep overflowed committed batches fenced after the age deadline. When more than 256 loads are unresolved, this aggregate retains their labels, but liveDroppedPublishFence clears the whole aggregate after 30 minutes without checking whether one label is still COMMITTED. The live reporter can then write a zero horizon, letting capture checkpoint past an unreadable batch; a later publish falls outside overlap. Check transaction status for aggregate labels and retain any committed obligation before clearing it.
| rewrittenNode = rewrittenNode.child(0); | ||
| userNode = userNode.child(0); | ||
| } | ||
| if (rewrittenNode.getClass() != userNode.getClass() |
There was a problem hiding this comment.
[P2] Restore caller headers for root set operations. A baseline on SELECT k + 1 FROM t UNION ALL SELECT k + 1 FROM u matches the k + 2 caller, but frozen decompilation wraps the set in a LogicalProject carrying the captured first-branch label. The caller root is LogicalUnion, so this class-mismatch return skips alignment; fallback set roots also lack an output-list case. The result header remains k + 1 even though the caller requested k + 2. Align the set's first-operand labels onto the replay output or skip this rewrite.
| if (i != children.size() - 1) { | ||
| // an underivable side before a derivable one: the derivable labels | ||
| // cannot be positioned | ||
| return new StarLabels(List.of(), true); |
There was a problem hiding this comment.
[P2] Preserve caller column labels when an unknown-width join input comes first. For a baseline on SELECT * FROM u CROSS JOIN (SELECT k + 1 FROM t) s, the matching k + 2 caller reaches this empty open-tail return because u has unknown parse-time width. alignRootOutputLabels then leaves the frozen k + 1 label on the derived column, and the result metadata reports that captured header instead of k + 2. Resolve the leading width from metadata or skip replay when a later caller label cannot be aligned.
| // the durable row already carries the requested status (the cache | ||
| // missed an earlier flip): repair the live object and report success | ||
| // without rewriting the row | ||
| if (previousStatus != status) { |
There was a problem hiding this comment.
[P2] Reconcile the durable winner's identity before completing ALTER. A handoff can publish two different CREATE rows under ID N: B caches its row after an empty collision probe, then A's delayed row appears and wins the same-second digest tie. probeDurableStatus returns A, but a matching-status ALTER keeps B cached here and reports success; a differing-status ALTER can write a newer B row because the conditional INSERT checks B's presence, switching the durable winner. Compare probe.winner with the cached bind/plan identity and adopt or fail on a foreign winner.
| // gone it only guards the DELAYED-commit window left - a demoted master's | ||
| // in-flight status INSERT committing after the delete. | ||
| if (entry.getValue().droppedIdentity != null) { | ||
| noteDroppedSeqState(entry.getValue().droppedIdentity); |
There was a problem hiding this comment.
[P2] Move tombstone I/O out of the baseline cache write lock. When a prior DROP's DELETE was unconfirmed and a snapshot now shows absence, this call writes its sequence tombstone while stateLock.writeLock is held by load or refresh. The internal INSERT can wait up to 10 seconds, blocking every concurrent SPM candidate lookup on stateLock.readLock. Resolve the durable obligation outside the cache lock, then publish against a checked state version.
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)