Repository navigation
[feature](plan)support sql plan management - #68499
starocean999 wants to merge 15 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 |
|
Codex automated review failed and did not complete. Error: Review recovery stopped: Could not verify exactly one final review from this invocation; first recorded API failure: GitHub API POST repos/apache/doris/pulls/68499/reviews failed (CalledProcessError, exit=1, timeout=None); stderr="gh: Unprocessable Entity (HTTP 422)\n"; stdout="{"message":"Unprocessable Entity","errors":["Position could not be resolved, Position could not be resolved, Position could not be resolved, and Position could not be resolved"],"documentation_url":"https://docs.github.com/rest/pulls/reviews#create-a-review-for-a-pull-request\",\"status\":\"422\"}" (see review-api-errors.jsonl) Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
|
/review |
There was a problem hiding this comment.
Request changes. Review did not converge after 3 rounds; verified findings are delivered at the required round cap.
Reviewed #68499 at 2508e18 using the supplied aggregate diff and 495-path list. Static analysis only; no builds, tests, or source modifications were performed. Six new verified findings are inline: qualified UDFs in star REPLACE, nested-field output headers, NULLIF marker survival, capture-time function identity, residual anti-join column binding, and schema-fingerprint field boundaries. Existing P1s are carried by numeric ID rather than reposted.
The feature does not yet preserve every admitted query's semantics. The supplied user-focus file contains no additional concerns.
| Required checkpoint | Conclusion |
|---|---|
| Goal and proof | GLOBAL/SESSION baseline creation, matching, frozen replay and fallback are implemented; the verified cases show correctness gaps. Author tests are evidence of intended coverage, not executed validation. |
| Scope and clarity | The change spans 94 production paths, 89 Java test paths, 162 regression suites and 150 result fixtures. The manager and plan-tree/decompiler classes are large; detailed residual coverage is disclosed below. |
| Concurrency and locks | Writer serialization, cache epochs, durable reservations, shared mutation windows and sorted table locks were traced. Function identity is read after capture; known DROP retry and expired-window failures remain. |
| Lifecycle and initialization | Nested capture contexts close in finally; fallback resets planning/session state; session stores clear on connection cleanup; startup/periodic global loading is asynchronous. No C++ static initialization changes. |
| Configuration | Rewrite/fallback/deadline settings are forwarded. Cache size is validated; refresh interval is reread dynamically with a positive clamp. |
| Compatibility | Evolved provenance/identity/clock columns are nullable and have migration/retry paths; legacy provenance defaults exist. Mixed-version operational behavior was not executed. |
| Thrift/protobuf IDs | No gensrc/thrift or gensrc/proto changes. |
| Parallel paths | Query/EXPLAIN, GLOBAL/SESSION, forwarded/local, frozen/tree replay and cloud-aware internal contexts were checked to the stated coverage depth. |
| Special conditions | Placeholder, scan-selector, output alignment, row-cap and metadata guards were traced; verified misses are inline. |
| Test coverage | Extensive positive/negative and concurrency tests exist, but the new cases are not covered by the inspected tests. Remaining test-body depth is disclosed below. |
| Changed expected results | All 585 literal qt tags in changed regression suites match output blocks; 42 paired with/without-SPM blocks agree textually. This does not establish all SQL-result semantics or generation provenance. |
| Observability | DDL correlation, baseline-hit EXPLAIN output, warning/error context and refresh logging exist. No separately verified observability bug. |
| Persistence and failover | Internal tables carry baseline rows, sequence identities and the mutation clock; cache reload/handoff and failure-retry schedules were traced. The carried P1s show outstanding durability/order failures. |
| Writes and atomicity | Multi-statement ALTER/DROP uses confirmation and fencing; a known partial twin cleanup and aged status-publication fence remain incorrect. This PR does not change user-data transaction or MoW/visible-version protocols. |
| FE/BE propagation | No new FE/BE protocol fields; new manual scan selectors are propagated through logical-to-physical construction/copies. |
| Performance | Cache capacity and batched identity reads are bounded. Historical cold-read deadline and held-window settlement concerns were deduplicated; detailed cost/runtime behavior was not measured. |
| Other issues | New findings were independently checked against current code and historical review comments; unresolved questions are kept out of inline findings. |
Existing blocking findings independently confirmed on this head (not reposted):
- 4312196383: absent-row DROP retry compares epoch milliseconds with a seconds bound and skips unfinished twins.
- 4306654208: a committed no-op status intent can outlive the 120-second shared fence and reverse a successor's successful ALTER when stored seconds have advanced ahead of wall time.
- 4306654206: prune publication beyond the same fence allows the inclusive-ID/OFFSET snapshot reader to retain an old status and skip the surviving newer version.
- 4240415594: equal random FE salts and same-tick opens still let one visible marker certify another writer's unpublished marker; closing the shared tick removes both. The new salt/count check reduces the probability but does not prove ownership.
Known P2 cold-read deadline and held-window settlement concerns were deduplicated against historical comments rather than reposted. The older direct NULL/CASE/IFNULL, ordinary qualified-UDF, cast/AVG and table-fingerprint examples were rechecked against their current repairs; the new inlines describe separate surviving instances.
Remaining questions and coverage limits: the stale non-dropped compact identity versus newer history DROP predicate is suspicious for supported preexisting twins, but a complete healthy-head twin/status-write genesis has not been established. It remains a question, not an inline finding. Static coverage remains incomplete for some planner/decompiler shape and fingerprint/walker interactions, substantial FE/regression test bodies, and the semantic correctness of all 150 expected-result files. Structural tag/block checks do not resolve those limits. No runtime validation was attempted, as required by this invocation.
Final convergence result: converged=false, rounds=3. The final round added the deterministic schema-encoding finding after the five-comment set was rechecked; the final six-comment set therefore lacks unanimous unchanged-set NO_NEW_VALUABLE_FINDINGS. The coverage and reachability limits above also remain.
Review did not converge after 3 rounds. This automated review cannot pass the review check.
The final round added the schema-fingerprint encoding finding, so the six-finding set did not receive unanimous unchanged-set NO_NEW_VALUABLE_FINDINGS. Required detailed semantic coverage remains incomplete for some decompiler/plan-tree fingerprint-walker interactions, substantial FE/regression test bodies and all expected-result semantics. R3 also retains an unresolved reachability question: whether current leadership/recovery can produce an unmarked lower-id late twin after a larger-id DROP while a stale non-dropped compact identity masks the newer history tombstone. This question is excluded from inline findings; no fourth round was started.
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; first recorded API failure: GitHub API POST repos/apache/doris/pulls/68499/reviews failed (CalledProcessError, exit=1, timeout=None); stderr="gh: Unprocessable Entity (HTTP 422)\n"; stdout="{"message":"Unprocessable Entity","errors":["Position could not be resolved and Position could not be resolved"],"documentation_url":"https://docs.github.com/rest/pulls/reviews#create-a-review-for-a-pull-request\",\"status\":\"422\"}" (see review-api-errors.jsonl) Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
|
/review |
There was a problem hiding this comment.
Static review of #68499 at d5d0580. Four new verified findings are inline: two P1s (wrong-value replay and reversal of a successful status ALTER), one P2 result-header regression, and one P2 unnecessary refresh parsing. Six existing P1 comments remain applicable and are retained without duplicate comments. Nested conditional producers still erase live markers: projected CASE 4307645264, IFNULL/NVL 4322240470, and NULLIF 4325302219 bypass the literal-only/direct-parent repairs when composed with other conditionals, allowing TRUE variants to replay as NULL. Resumed DROP cleanup can skip a future-dated preexisting twin 4312196383, or delete a later same-second CREATE after a failed key read 4322240477. A function redefinition between successful snapshot comparison and the subsequent live fingerprint read still certifies stale frozen output 4325302236.
There was no additional user-provided focus. Builds, tests and source edits were prohibited and were not performed. Static artifact checks verified all 497 authoritative path hunks against HEAD, all 42 paired with/without-SPM expected-output blocks, and all 492 extracted benchmark SQL/EXPLAIN pairs. These checks establish artifact consistency, not executed query correctness or proof that golden files were generated by test scripts.
Critical checkpoint conclusions:
- Goal and proof: the feature creates, persists, matches and replays session/global SQL plan baselines. Capture -> parameterization -> isolated optimization -> SQL decompilation/probe -> fingerprint -> storage -> matching/substitution -> replay/metadata validation was traced. Tests exercise this flow, but the new P1s and retained P1s show correctness is incomplete.
- Scope and clarity: this is a broad, cohesive subsystem addition across 94 production paths, 91 FE-test paths, 162 regression-suite paths and 150 expected-result paths. Persistence, cluster access, matching and serialization have separate interfaces; the large helpers require the detailed coverage stated in the convergence result.
- Concurrency: connection DDL threads, cache misses, async loading, all-FE refresh, concurrent metadata DDL and demoted-master writes introduce concurrency. Local writerLock serialization and stateLock/cache monitor ordering were traced; persistence I/O is outside query-side state/cache locks. Cold parsing still holds writerLock (new P2); real status flips bypass the stored-time lead bound, so their unreadable writes can outlive the shared fence and defeat a later successful intent (new P1). Retained DROP/function races remain as described above.
- Lifecycle and static initialization: nested capture StatementContext is closed in finally, caller context/session overrides are restored, per-session stores clear on reset, and daemon/loader retries plus retained mutation obligations were traced. The DROP obligation discriminator remains incorrect. No C++ cross-translation-unit initialization is changed.
- Configuration: rewrite/fallback/timeout settings are read per statement and forwarded; global refresh interval is re-read each cycle and cache capacity consults current defaults. No runtime configuration verification was performed.
- Compatibility: internal baseline/sequence/HWM tables add nullable provenance fields with type-aware upgrade loops and explicit timestamp handling. Stored raw/frozen provenance and legacy-null branches were inspected. Rolling upgrades were not executed.
- Thrift/protobuf definitions: none changed; no field-id or enum-value comparison is applicable.
- Parallel paths: session/global storage, cached/durable misses, frozen/raw fallback, query/EXPLAIN, and master/follower refresh/forwarding were inspected. Both replay paths share the output-label logic implicated by the new P2.
- Conditional guards: placeholder preservation, scan selectors, view/context refusal, cap/order contracts, qualified names, CTE/star payloads and metadata checks were traced. GREATEST/LEAST is an admitted producer omitted by the marker classifier; nesting bypasses the named conditional repairs; statement-wide qualifier discovery is unsound for an inner-only alias.
- Test coverage: extensive FE unit and regression fixtures cover capture/replay, joins/CTEs, coercions, delayed visibility and retry schedules. The identified triggers need actual capture-to-replay or production-precision schedules; existing narrow marker/ancient-timestamp tests do not prove them safe. Runtime validation was prohibited.
- Expected results: changed output artifacts and benchmark pair consistency were inspected statically as described above. Their runtime accuracy and generation provenance were not independently verified.
- Observability: EXPLAIN reports baseline id/scope/digest and logs identify replay failures and mutation recovery. The wrong-value and header regressions can successfully plan, so failure logging does not detect them.
- Persistence and failover: baseline metadata uses internal SQL tables rather than new EditLog records; reservations, HWM mutation windows, status intents, tombstones, cache publication and promotion reload were traced. Individual statement durability does not establish correctness of the retained multi-step DROP/function capture scenarios.
- Writes and atomicity: this changes baseline metadata DML, with explicit confirmation and reconciliation for ambiguous outcomes. The new status-ordering P1 and retained retry findings show that the overall multi-step operation can still violate the intended completed-DDL outcome. User-data transaction/visible-version/delete-bitmap logic is not changed.
- FE-to-BE variables: no new protocol fields or transmitted execution variables were introduced. FE-to-FE forwarding uses the existing session-variable map with bounded session baseline payloads.
- Performance and memory: the new cold-row parsing P2 rebuilds and discards uncached ASTs on each refresh, increasing CPU/peak Java memory and DDL lock duration. The historical cold-cache deadline limitation remains already-known context; no duplicate finding is posted. BE allocator/MemTracker rules are not implicated.
- Other verified concerns: known P2 nested-field trailing-star and qualified-EXCEPT provenance residuals were kept behind their existing comment fences (4325302204 and 4307645271); no duplicate inline is added. Remaining review coverage and convergence are stated separately below.
Static coverage is complete across the 497 supplied changed paths, using the coordinated per-path ledger and artifact audits. No unresolved candidate or risk-focus question remains. Runtime execution, rolling-upgrade behavior and golden-file generation provenance remain outside this static-only validation.
Review did not converge after 3 rounds. The final round identified and independently verified the additional real-status-flip P1; the four-comment set therefore has no subsequent unchanged-set convergence round within the allowed cap. All verified findings are delivered. Static path coverage is complete, all candidate/risk dispositions are settled, and no unverified candidate is included or withheld.
Review did not converge after 3 rounds. This automated review cannot pass the review check.
The final third-round sweep added and independently verified L2: real status flips bypass the stored-time lead bound and can reverse a successful successor ALTER. The resulting four-comment set has no subsequent unchanged-set convergence round within the three-round cap. Static coverage of all 497 paths is complete; all candidates and risk focuses are settled, with no unresolved or withheld unverified candidate.
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.
Static review of head 8a5e245. Changes are needed: the verified inline findings cover status-intent ordering, manual HAVING binding, erased array-element markers, UDF nullability fingerprints, result headers, qualified EXCEPT and scoped CTE cap identity.
Two existing P1 findings remain applicable and are not reposted: 4119817996 (a qualified alias-UDF body change can retain the same lossy dependency fingerprint) and 4325302236 (equal before/after function snapshots can conceal an intervening definition used during expansion). Their simpler original examples have partial fixes, but the current dependency encoding and capture interval still permit the documented variants. Existing P2 concerns about the unwired rewrite-deadline overload and qualified nested-field headers remain duplicate-fenced.
Critical checkpoint conclusions:
- Goal and proof: SESSION/GLOBAL manual capture, persistent baselines and literal-variant replay are implemented, but the verified semantic and lifecycle findings prevent correctness approval. Existing tests do not cover these exact failing compositions.
- Scope and reuse: this is a large feature with many independent traversals and SQL reconstruction paths; source and test ownership cover all 499 changed paths. Shared helpers exist, but their scope/type/identity assumptions need the fixes described inline.
- Concurrency and locks: local GLOBAL writers serialize on writerLock; stateLock and the LRU monitor protect publication. I/O is generally outside the publication lock. M1 shows preserved local/shared pending fences are bypassed by a real status flip; local serialization alone does not order unreadable committed versions.
- Lifecycle and initialization: startup loading, daemon refresh, promotion invalidation, SESSION reset, nested planning-context cleanup and prepared EXECUTE context renewal were traced. No C++ static-initialization change is present. The status lifecycle and function-capture interval remain incorrect as reported.
- Configuration: rewrite/fallback default off; cache/refresh values are read dynamically and refresh is clamped. The manager has a deadline-aware cold reader, but its production caller still invokes the unlimited overload, as already reported in 4260129920.
- Compatibility: internal-table schemas add nullable provenance/reservation/clock fields, with upgrade retries and legacy parsing. Function dependency compatibility is insufficient for changed optimizer-relevant metadata. No exported BE function-symbol or data-file-format change was identified.
- Thrift/protobuf IDs: no gensrc/thrift or gensrc/proto changes; cross-release field-ID comparison is not applicable.
- Parallel paths: frozen and raw replay, SELECT and EXPLAIN, SESSION and GLOBAL, forwarding, promotion and refresh paths were examined. The affected common guards can reach both SELECT/EXPLAIN; cloud execution was not validated.
- Special conditions: conservative refusal paths protect unsupported freezes, but the NULL-producer classifier, relaxed output aliases, partly open EXCEPT side and CTE-name cap map have concrete incorrect conditions.
- Test coverage: FE helper/planner/concurrency tests, focused regressions and benchmark harnesses were statically inspected. No builds, unit tests, regression tests or SQL executions ran; validation was static.
- Changed expected results: all 150 output files received structural audit; all 42 stored SPM/direct result pairs agree. All 123 benchmark harnesses compare matching executed/EXPLAIN texts and include own-baseline hit checks. These facts do not establish every stored result value, frozen SQL's semantics or generation provenance.
- Observability: EXPLAIN exposes baseline id/scope/digest and persistence/replay paths log failures. No new observability defect was independently established.
- Persistence and failover: baselines use internal-table SQL transactions, durable reservations/tombstones and mutation windows, rather than a new EditLog opcode. Schema DDL uses the existing Env paths. M1 shows status intents are not correctly composed; E1/E2/M6 show metadata certification gaps.
- Writes and atomicity: each internal SQL statement has the existing transaction behavior, but the multi-statement baseline protocol does not guarantee the advertised final status under the verified delayed-publication schedule.
- FE/BE propagation: no new wire variable is introduced. Manual scan-pin information is preserved into physical scans; UDF calls rebind current catalog declarations, which makes M6 observable.
- Performance: bounded caches, coalesced loads and stable traversal order are present. The existing cold-read deadline issue remains; no additional unverified performance claim is raised.
- Other concerns and user focus: no additional user-provided focus. Every accepted candidate is inline; duplicates, repaired reports and speculative dismissals are recorded in the shared ledger. Remaining coverage limits and convergence are stated separately below.
Convergence: true after 3 review rounds. All three reviewers returned NO_NEW_VALUABLE_FINDINGS on the same final seven-comment set (4 P1, 3 P2), including the corrected executable CTE anchor. All 499 assigned changed paths and required related mechanisms have static review coverage; there are no unresolved candidates or unreviewed assigned ranges. Existing blockers 4119817996 and 4325302236 are retained without duplicate inline comments. Runtime execution, cloud behavior, generated-output provenance, performance measurements and exhaustive equivalence of arbitrary frozen SQL remain unvalidated; those are explicit static-review confidence limits. No additional user focus was supplied.
Review converged after 3 round(s).
Existing P0/P1 findings confirmed for this head: #68499 (comment), #68499 (comment)
|
/review |
There was a problem hiding this comment.
Static review of #68499 at 2a46dc6. Changes requested: two new P1 findings are inline (MAP NULL-marker loss and computed ORDER BY alias binding). Three independently confirmed existing P1s are retained without duplicate comments:
- 4333965568: the local real-flip fence repair still bypasses unresolved shared intents after leader handoff. With a hidden old-leader no-op DISABLE and an overlapping successor ENABLE whose timestamp is no later, both can succeed and the durable winner reverts ENABLE.
- 4333965579: the function-form ARRAY repair does not cover bracket ArrayLiteral leaves. Capturing element_at([NULL],1) and replaying [TRUE] still loses the marker during Boolean binding.
- 4333965583: class-level flags still omit ordinary UDF instance NullableMode, so the existing post-CREATE nullability/COALESCE counterexample remains.
The known P2 cold-lookup deadline issue 4260129920 also remains: the production caller still invokes the untimed manager overload. Other recent fixes were revalidated in their applicable paths, including typed-constructor/arithmetic casts, locked metadata capture, dotted anti-join references, HAVING aliases, output/CTE scopes, and qualified function payloads. A tentative same-name nested alias-UDF dependency concern was dismissed because production CREATE expands the inner alias before persistence.
Critical checkpoint conclusions:
- Goal and correctness: SESSION/GLOBAL baseline DDL and parameterized freeze/replay are integrated, but the two new semantic failures and retained P1s prevent accepting the implementation as correct. Existing helper/unit tests do not cover these complete capture/replay counterexamples.
- Scope and clarity: this is a large feature across 503 changed paths, including broad planner support, durable metadata, forwarding, audit bookkeeping, and extensive tests. The small ordinary-planner changes were inspected; full semantic coverage of every large helper/test body is incomplete.
- Concurrency and locks: query-side publication uses cache/state synchronization and durable writers use a separate writer lock. The shared-intent status-ordering failure above remains; complete concurrency-test and historical-race coverage was not achieved.
- Lifecycle: private capture contexts, fallback state resets, SESSION forwarding, startup loading, promotion invalidation, refresh and cleanup were traced. Remaining limits include less-traveled manager/history and planner combinations; no blanket lifecycle-safety claim is made.
- Configuration: rewrite, timeout and fallback switches are forwarded; rewrite and failed-replay fallback default off. The deadline-aware cold lookup is not wired into the production caller, as already reported.
- Compatibility: internal schemas, provenance, legacy decoding, microsecond timestamps and upgrade initialization were inspected. Mixed-version deployment behavior was not executed or exhaustively proven.
- Thrift/protobuf: no definitions changed, so cross-branch field-ID comparison is not applicable.
- Parallel paths: frozen/raw, GLOBAL/SESSION, execution/EXPLAIN, forwarding, join/aggregate/window/set/CTE and scan-selection paths were considered. Full cross-product coverage remains incomplete.
- Conditional checks: enumerated NULL-producer classification misses MAP and bracket ARRAY shapes; root alias relaxation fails to preserve a computed ORDER BY consumer. These are concrete failures of the relevant guards.
- Test coverage: extensive unit/regression tests were inspected statically, including targeted later fixes. No builds, tests, cluster runs or source modifications were performed, as required by this review contract.
- Expected results: static comparison of all 123 benchmark suites found matching stored bind text, matching paired query inputs, and equal/nonempty recorded with-SPM/without-SPM result blocks where present. This does not prove every frozen SQL equivalent or establish output-generation provenance; other expected-output/test bodies retain semantic depth gaps.
- Observability: baseline-hit EXPLAIN metadata and downgrade/failure logs exist in the inspected paths. No additional observability defect was substantiated.
- Persistence and failover: baseline metadata uses internal-table SQL with reservations, tombstones and mutation windows rather than a new EditLog opcode. The confirmed shared-intent handoff failure remains.
- Writes and atomicity: changes write baseline metadata and internal bookkeeping, not user-table rowsets. Multi-statement baseline mutation ordering remains subject to the reported status race; general crash/failover safety is not proven by static inspection alone.
- FE/BE propagation: no new protocol field was added; session forwarding and existing planner translation paths were inspected. No additional propagation omission was substantiated.
- Performance: cold-cache coalescing, lazy parsing, batched reservation reads and scoped locking were checked. The existing rewrite-deadline issue remains; the full large-query/recursive-walker complexity matrix was not completed.
- Additional concerns: the user supplied no extra focus. Existing threads were used as duplicate fences; only independently verified new issues are inline.
Convergence: false after 3 rounds. Reviewers rechecked the final two-comment set, but required exhaustive semantic coverage remains incomplete for the full PlanTreeSupport/manual/NULL-producer matrix, portions of manager/history/failover combinations, many unit/regression/expected-output bodies, and individual revalidation of every historical P0/P1 comment. All 503 paths were inventoried and assigned; inventory and structural scanning are not full semantic verification. No unresolved candidate is presented as an inline finding. This capped delivery publishes the verified findings and leaves the review check blocked for both the confirmed P1s and incomplete convergence.
Review did not converge after 3 rounds. This automated review cannot pass the review check.
Final reviewers rechecked two verified P1 comments, but full semantic coverage is incomplete across PlanTreeSupport/manual and NULL-producer combinations, some manager/history/failover combinations, many unit/regression/expected-output bodies, and individual historical P0/P1 revalidation. All 503 paths are inventoried/assigned; structural scans do not prove complete semantic review. No unresolved candidate is published as a finding.
Existing P0/P1 findings confirmed for this head: #68499 (comment), #68499 (comment), #68499 (comment)
|
/review |
|
Codex automated review failed and did not complete. Error: Review recovery stopped: Could not verify exactly one final review from this invocation; first recorded API failure: GitHub API POST repos/apache/doris/pulls/68499/reviews failed (CalledProcessError, exit=1, timeout=None); stderr="gh: Unprocessable Entity (HTTP 422)\nGitHub request failed: POST /repos/apache/doris/pulls/68499/reviews; HTTP=422; request_id=9C01:77DF9:1EA9EE5:62C8EFB:6AD99763; CalledProcessError; exit=1; timeout=None"; stdout="{"message":"Unprocessable Entity","errors":["Position could not be resolved and Position could not be resolved"],"documentation_url":"https://docs.github.com/rest/pulls/reviews#create-a-review-for-a-pull-request\",\"status\":\"422\"}" (see review-api-errors.jsonl) Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
### What problem does this PR solve? Related PR: apache#68499 Problem Summary: Bulk REST review creation can reject a valid set of inline findings with HTTP 422 on a large PR. Create one pending review, add each finding with a separate GraphQL thread mutation, verify the complete draft, and publish once. Preserve the existing input schema, frozen target guards, exclusive submission intent, exact final readback, and summary-only path. Record staged progress and GraphQL errors; clean up only owned pending drafts before publication, without retrying uncertain writes. ### Release note None ### Check List (For Author) - Test: Unit Test: 275 tests OK, 19 platform skips; real gh HTTP tests use loopback only. Manual test: all five original apache#68499 findings accepted in one draft via separate GraphQL mutations, then draft deleted. Frozen diff mapping replay matched all five anchors. Actionlint and git diff --check. - Behavior changed: Yes, inline review submission is staged before publishing. - Does this need documentation: Yes, operational guidance updated in this PR. Live formal publication and the GitHub Actions identity are not yet validated.
|
/review |
|
Codex automated review failed and did not complete. Error: Review recovery stopped: Could not verify exactly one final review from this invocation; first recorded API failure: GitHub API POST repos/apache/doris/pulls/68499/reviews failed (CalledProcessError, exit=1, timeout=None); stderr="gh: Unprocessable Entity (HTTP 422)\nGitHub request failed: POST /repos/apache/doris/pulls/68499/reviews; HTTP=422; request_id=F400:133F18:3E95CF5:C906B5F:6AD9D6D6; CalledProcessError; exit=1; timeout=None"; stdout="{"message":"Unprocessable Entity","errors":["Position could not be resolved and Position could not be resolved"],"documentation_url":"https://docs.github.com/rest/pulls/reviews#create-a-review-for-a-pull-request\",\"status\":\"422\"}" (see review-api-errors.jsonl) Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
|
/review |
There was a problem hiding this comment.
Request changes at 31b8f59382ece9bd0ae9bf65c93b2643f4b9045e.
Three new findings are inline: QUALIFY alias remapping can change selected rows (P1), ordinary UDF return-type changes can leave stale frozen expressions (P1), and case-sensitive table-name collisions prevent identical-query baseline matches (P2).
Four previously reported P1 blockers were independently revalidated without duplicate comments:
-
4176748157: after a confirmed identity DELETE,
retainDropKeyCleanupcan throw while reading the HWM before cache eviction and before the cleanup recovery boundary; the cached ENABLED baseline remains matchable. -
4202301265: the same pre-retention failure loses the deleted identity's durable tombstone/retry obligation, permitting revival by a delayed status row after cache loss or handoff.
-
4333965573: the HAVING repair compares consumed alias names, but an alias permutation preserves that set while binding the predicate to a different expression. For example,
SELECT k AS x,v AS y FROM t HAVING x>0versus the manual planSELECT k AS y,v AS x FROM t HAVING x>0filters k versus v. -
4333965579: the direct ARRAY(NULL) repair does not cover an ARRAY_CONCAT wrapper. Capture
SELECT k>0 OR element_at(array_concat(array(NULL),array(NULL)),1) AS c FROM t, then execute itsarray(TRUE),array(TRUE)variant. The wrapper is not recognized as an all-NULL container, indexed ANY retains ARRAY<NULL_TYPE>, andExpressionAnalyzer.visitOrerases ElementAt and its markers. For k=-1, frozen replay returns NULL instead of TRUE. The probe only replans the reduced SQL; no missing-bind-marker check repairs this residual of the existing report.
The original retained-twin cleanup, later re-CREATE cleanup, status lead-bound and unresolved opposite-intent schedules are repaired by retained cleanup bounds/removal and the shared timestamp/fence gates. Capture lock-time schema snapshots, registry-version checks, ordinary UDF nullable mode, and root ORDER BY consumer checks repair their corresponding reports; the inline findings describe different remaining cases. The known cold-lookup timeout and qualified EXCEPT provenance concerns remain covered by earlier P2 comments.
Critical checkpoint conclusions:
- Goal and proof: GLOBAL durable and SESSION connection-local plan management is wired through CREATE/SHOW/ALTER/DROP, SELECT and EXPLAIN. The new correctness findings and existing DROP/HAVING blockers prevent considering the goal complete. Added tests provide broad static evidence; none were executed for this review.
- Focus and size: parser, capture, replay, persistence, forwarding and tests form one feature, but the implementation is unusually large. Shared audit, time and internal-query changes were checked against ordinary callers; no additional concrete regression was established there.
- Concurrency and protection: DDL/query threads, initial loading, refresh and forwarding callbacks share manager generation/version/readiness, pending fences, hash loads and cache state.
writerLockserializes mutations; state read/write locking and the cache monitor protect publication; SESSION stores synchronize their operations. - Work inside locks: distributed SQL is outside the state/cache locks. The writer lock intentionally spans management I/O; refresh prepares changed trees before publication. No additional substantiated heavy-work correctness issue was found.
- Lock order: inspected production nesting is writer lock -> state lock -> cache monitor; no inverse acquisition or concrete deadlock was found. Loader notification is in finally paths.
- Lifecycle and initialization: promotion invalidates before readiness and rejects stale loads by generation/version. Connection reset clears SESSION and forwarded state; abandoned replay resets planner/session state. No new initialization cycle was identified; C++ cross-translation-unit static order is inapplicable.
- Configuration: enable/match/forwarding and replay controls are session variables. Refresh interval and cache capacity observe current defaults without restart; forwarded SESSION payloads are bounded. The known rewrite-budget issue still drops the planner deadline on cold lookup.
- Compatibility: stored parser mode, provenance and legacy fallback are present. The three new internal tables use explicit timestamp precision; mismatched pre-release schemas are deliberately rebuilt. Static inspection cannot certify mixed-version deployment or migration behavior.
- Thrift/protobuf: no changed definitions under gensrc/thrift or gensrc/proto; branch field-ID comparisons are inapplicable.
- Parallel paths: SELECT/EXPLAIN, GLOBAL/SESSION, frozen/raw replay, cached/cold DROP, real/no-op status changes and local/forwarded operations were traced. Cloud DDL explicitly rejects unsupported operations. Shared guards do not prevent the inline cases.
- Special checks and errors: capture/replay masks, parser isolation, dependency snapshots, mutation windows, reservation readback and tombstones have concrete purposes. The confirmed alias/dependency omissions and DROP recovery gap are exceptions to their intended guarantees; no additional speculative defensive-check finding is included.
- Test coverage: units cover matching/decompilation, metadata drift, negative inputs, forwarding, cache and latch-based mutation races; regression suites cover DDL and SPM on/off queries. The accepted alias/return-type/case-collision schedules, wrapped-array marker erasure and post-delete HWM failure lack effective coverage. Mocked persistence tests do not establish real distributed publication/failover behavior.
- Changed results: all 151 result paths were inspected, with repeated benchmark structures factored and all 21 TPCH SPM on/off output pairs compared. No separate incorrect expected row was substantiated. The ordinary TPCH q14/q17 changes reduce printed precision; numerical correctness and automatic-generation provenance remain unverified without running the suites.
- Observability: replay decisions and management/load/refresh errors include contextual logs and query/baseline identifiers. No additional necessary metric or excessive hot-path INFO log was established during static inspection.
- Persistence and failover: GLOBAL state uses transactional internal-table SQL, reservations, mutation windows and durable tombstones rather than new EditLog records. Promotion/late-writer paths were traced; the confirmed post-delete obligation gap remains an existing blocker.
- Data writes and crashes: each SQL statement is transactional; multi-statement baseline DDL relies on reconciliation and fencing, rather than one atomic multi-table transaction. The DROP gap can leave replayable or revivable metadata. No BE rowset, visible-version or delete-bitmap implementation is changed.
- FE/BE variables: no new FE-BE wire fields or scattered serialization sites are introduced. SESSION forwarding and internal-query timeout propagation were checked through their FE paths.
- Performance: bounded pages, per-hash load coalescing, shallow snapshots and whole-hash LRU limit common costs. Repeated metadata/internal SQL work remains significant; the existing cold-cache timeout report is the concrete unresolved performance defect identified here.
- Other issues and final recheck: revisited alias-consuming wrappers, out-of-band star/generator/inline payloads, table/function capture, fallback context cleanup, mutation obligations and ordinary audit/statistics callers. All recorded hypotheses have a verified, duplicate or evidence-based dismissed disposition; no additional unresolved candidate remains.
Coverage: all 514 authoritative changed paths were covered by the main scan and two disjoint normal-review assignments, with a separate reviewer for all seven initial risk mechanisms. Repeated benchmark and result text was examined through factored inventories plus targeted full bodies. There was no extra user focus in the supplied focus file. This was a static-only review: no builds, tests, source changes or result regeneration.
Convergence: not converged after 3 rounds. All three reviewers accepted the earlier frozen set, but the final sweep confirmed the wrapped-array residual above and expanded the retained blocker list from three to four. The revised full set did not receive another unchanged-set pass within the cap; no fourth round was started. All verified findings are delivered. No additional unresolved concrete candidate or static path gap remains; the execution limits above still apply.
Review did not converge after 3 rounds. This automated review cannot pass the review check.
Final sweep independently confirmed the ARRAY_CONCAT residual of existing P1 4333965579 after the round-3 frozen-set pass. The existing blocker set and summary therefore expanded after that pass and were not reconfirmed by every reviewer as one unchanged set within the three-round cap. All 514 changed paths have static coverage and all candidates/risks have explicit dispositions; no additional unresolved concrete candidate is identified. Builds/tests, distributed runtime and mixed-version behavior, and generated-result provenance remain unexecuted validation limits.
Existing P0/P1 findings confirmed for this head: #68499 (comment), #68499 (comment), #68499 (comment), #68499 (comment)
|
/review |
There was a problem hiding this comment.
Static review of head 8038431. Changes are needed: the three new inline findings contain two P1 replay-correctness defects and one P2 identical-query matching defect. Existing P1 4333965568 is independently reconfirmed and retained without reposting. No additional user focus was supplied.
Critical checkpoint conclusions:
- Goal and correctness: GLOBAL/SESSION capture, matching and frozen/raw replay are implemented, but the verified cases change results or prevent a baseline hit. Existing tests do not establish these cases.
- Scope, clarity and reuse: traversal, persistence and replay helpers are shared; this is a substantial feature with large new manager/decompiler helpers. Exhaustive minimality review is incomplete.
- Concurrency and locks: query, load/refresh and DDL paths were traced. Local writerLock separates I/O from short stateLock/cache publication. Foreign pending-status ordering still has the existing P1 described below; no blanket failover-safety conclusion.
- Lifecycle: nested statement contexts close and restore, thread-local capture/digest windows use finally, SESSION stores clear on reset and import replaces forwarded content. Inspected paths revealed no further verified lifecycle defect.
- Configuration: rewrite/fallback/timeout variables forward; cache capacity and refresh interval read dynamic defaults. The known cold-read deadline limitation remains.
- Compatibility: new SQL-mode/frozen-plan provenance and first-publication internal schemas are explicit. Rolling-upgrade behavior was not exhaustively proven.
- Thrift/protobuf: no definition changes; field-ID branch comparison is not applicable.
- Parallel paths: inspected frozen/raw replay, query/EXPLAIN, local/forwarded execution, GLOBAL/SESSION routing and cached/durable-only management. Historical P2 4307645271 remains partly applicable: qualified EXCEPT over two known same-named prefix columns plus an open star tail drops both labels and can rename a correct [x,z,k,q] header to [z,z,k,q]. It is already covered and not reposted. Coverage limits remain in the large helpers.
- Conditions and errors: ambiguous writes retain fences/obligations; conditional inserts distinguish zero rows; unusable replay normally declines or follows the fallback setting. The new classifier/mirror/fingerprint cases bypass intended semantic guards.
- Tests: extensive FE/regression coverage exists. Test bodies were sampled by mechanism; no builds, tests or runtime SQL were executed under this static-only contract.
- Expected results: static inventory found 151 changed output files, 42 SPM-on/off pairs identical, and hit assertions in all 123 added benchmark suites. This is not proof every expected result is correct or generated; exhaustive validation remains incomplete.
- Observability: capture/matching/fallback/mutation paths log identities and failure context; no additional independently established observability issue.
- Persistence/failover: internal OLAP rows, reservations, tombstones and mutation clocks carry durable state rather than new EditLog payloads. Existing DROP cleanup and lead-bound reports have inspected repairs; foreign-intent ordering retains existing P1 4333965568.
- Writes and atomicity: multi-statement management relies on deterministic winners and confirmed/ambiguous-write obligations. All crash/handoff interleavings have not been established.
- FE/BE variable transport: no new backend execution variable/protocol is introduced; session forwarding was inspected. BE visibility/delete bitmap/MemTracker changes are not applicable.
- Performance: shallow loads, complete-bucket LRU, coalescing and batched identity reads address earlier reports. Historical P2 4260129920 remains partly applicable: identity-state reads restore fixed ten-second budgets inside a shorter rewrite deadline. It is not reposted.
- Additional issues and duplicate control: all new candidates were checked against supplied threads and searched across 613 historical inline bodies. Selected prior P1s are repaired as recorded in the ledger; the full historical P0/P1 audit is incomplete.
Existing blocker 4333965568: the local same-FE intent fence is repaired, but foreign ordinary flips still bypass the shared quiet-clock read. A supported configured schedule sets max_bdbje_clock_delta_ms=60000 (Config.java, wired to JE by BDBEnvironment) with A's clock B+30s. At B-time0 visible DISABLED has T=29s; A computes no-op DISABLED S=30s, passes its leadership check and pauses before opening a mutation window. B promotes and completes quiet reload by B-time10. A resumes its internal DML after demotion, opens a visible shared window and commits S while its publication stays hidden. B's ENABLE at time12 sees cached and durable DISABLED T, takes the ordinary branch and chooses E=T+1us<S (a permitted 17s lead). Its conditional INSERT can read T before S publishes; both versions then publish before presence confirmation. ENABLE presence succeeds, old-status cleanup removes only versions <=T, and DISABLED S remains the durable winner. A later-time opposite status satisfies the local fence, so refresh reverses the successful ENABLE. This revalidation relies on explicit supported finite clock skew and the traced handoff/visibility schedule; it does not assert a measured election duration or failure under globally identical monotonic clocks. The existing comment is carried, not duplicated.
Coverage limits: the main agent did not read every line of the 145122-line patch before delegation. Exhaustive depth remains in output-star/USING label alignment, cap/CTE identity, projection/order hoists, needed-column and set/repeat rendering, optimizer rules admitted by the mask, all test/output correctness and all historical blocking-thread dispositions. All named candidates and risk focuses have dispositions in the ledger, but required coverage is incomplete. At the three-round cap the verified findings are delivered with converged=false; the review check must remain blocked. No fourth round is requested or implied.
Review did not converge after 3 rounds. This automated review cannot pass the review check.
Required coverage is incomplete after three rounds. The main agent did not fully read the entire authoritative diff before delegation. Remaining depth includes exhaustive output-star/USING/CTE/cap and physical-rewrite combinations, recursive CTE generated scopes, expression/coercion marker survival across every admitted optimizer rule, all aggregate stage layouts, all 151 expected-output files and 100+ semantic plus remaining manager/concurrency test bodies, and every historical P0/P1 disposition. Static output tag/pair checks and sampled tests do not prove all expected results or generated provenance. The three verified new findings and reconfirmed existing P1 4333965568 are deliverable, but these coverage conditions prevent convergence. No builds or runtime tests were performed, as required by the review-only contract.
Existing P0/P1 findings confirmed for this head: #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; first recorded API failure: GitHub API POST repos/apache/doris/pulls/68499/reviews failed (CalledProcessError, exit=1, timeout=None); stderr="gh: Unprocessable Entity (HTTP 422)\nGitHub request failed: POST /repos/apache/doris/pulls/68499/reviews; HTTP=422; request_id=C002:2C3769:9CF989:201BF9A:6ADE0B24; CalledProcessError; exit=1; timeout=None"; stdout="{"message":"Unprocessable Entity","errors":["Position could not be resolved and Position could not be resolved"],"documentation_url":"https://docs.github.com/rest/pulls/reviews#create-a-review-for-a-pull-request\",\"status\":\"422\"}" (see review-api-errors.jsonl) Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
|
/review |
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)