Repository navigation
Share one execute query with plan capture idiom - #2395
Conversation
Firebolt, Presto/Trino, PostgreSQL, SingleStore, and Doris each wrapped the shared cursor execution with the same delegate-then-merge lines. Add execute_query_with_plan_capture to the result capture mixin that already owns the merge step, and point all five thin overrides at it. Each adapter keeps its override and platform docstring; adapters with different semantics (Redshift's failure guard, pg_mooncake's retry, QuestDB's rewriter) keep their bespoke paths.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
github-actions
Bot
added
the
awaiting-owner
|
Status (Sep 27): no bot review threads on this PR; CI CLEAN. Note: touches benchbox/platforms/base/result_capture.py, a soundness path — withholding auto-enqueue for owner review and manual merge per the soundness gate. |
|
Maintainer review: verified the diff directly. The shared execute_query_with_plan_capture owns the delegate-then-SUCCESS-guarded-merge idiom; all five call-site conversions are mechanical (same args, comments preserved, bespoke overrides like Redshift/QuestDB untouched). New unit tests cover the shared helper. Zero open threads, required checks green. Enqueuing. |
|
Independent external Muse review of a29ec31: SHIP. No Critical or Required findings. The five adapters retain their parent execution and platform contracts. The shared helper forwards validation and stream arguments, returns the same result, and runs the existing plan merge afterward. Strict PlanCaptureError propagation and isolated post-measurement capture remain intact. Redshift, pg_mooncake, and QuestDB keep their distinct paths. Optional suggestions were a helper-level strict-error test and a more detailed bound-callable annotation. |
Firebolt, Presto/Trino, PostgreSQL, SingleStore, and Doris duplicate the same parent-execution and plan-capture sequence. Move that sequence into the existing result capture mixin and delegate the five thin overrides to it. Preserve strict plan-capture errors, validation and stream arguments, isolated capture, and result identity. Adapters with different execution semantics retain their own paths.
Validation: 728 focused adapter and plan-capture tests passed. The required local preflight passed, including 2,125 medium-tier tests. Ruff passed; the type checker exited successfully with existing optional-SDK import warnings.
Soundness review:
External reviewer: muse
Review output: #2395 (comment)
All Critical/High findings resolved.