Repository navigation
fix(tuning): record tuning statements issued through .sql() and .query() - #2855
Conversation
![benchbox-oracle[bot]](https://avatars.githubusercontent.com/u/319264189?s=60&v=4){kind=small}
There was a problem hiding this comment.
oracle-verdict: failure for 81676e0bb2fe95867528109fecb2e2586dda495b
This result supersedes those for earlier heads; their open review threads still apply.
Tier: low-medium (data-only or small single-area change).
Decision: SHIP WITH FIXES.
Reviewer: luna (codex gpt-6-luna, effort high).
Two issues block merge: the schema detector can mistake ordinary column names for tuning, and DataFusion's successful CTAS sort path still bypasses ledger capture.
There was a problem hiding this comment.
oracle-verdict: failure for 90f091688b79d94ed925ccb533ae3d01953708cb
This result supersedes those for earlier heads; their open review threads still apply.
Tier: medium-high (167 code lines exceed 150).
Decision: SHIP WITH FIXES.
Reviewer: sol (codex gpt-6.1-sol, effort medium).
Two fixes are required: preserve Spark table formats when column lists contain comments, and prevent comment-only fragments from becoming tuning evidence. Reviewed the supplied diff and relevant head-commit code without running tests or modifying files.
There was a problem hiding this comment.
oracle-verdict: failure for 14c27885d655b3b1f6d12ab5744574458b513b4b
This result supersedes those for earlier heads; their open review threads still apply.
Tier: medium-high (189 code lines exceed 150).
Decision: SHIP WITH FIXES.
Reviewer: sol (codex gpt-6.1-sol, effort medium).
Both earlier defects are fixed. Two additional capture defects must be corrected because they omit applied tuning evidence or produce incorrect corroboration verdicts. Review used static reads only; no tests were run.
Verified fixed: D1, D2. Resolve their review threads.
Scope: files changed since head 90f091688b79d94ed925ccb533ae3d01953708cb was reviewed.
There was a problem hiding this comment.
oracle-verdict: failure for d48d18dede38965114d98373ea8c3f7992574f97
This result supersedes those for earlier heads; their open review threads still apply.
Tier: medium-high (194 code lines exceed 150).
Decision: SHIP WITH FIXES.
Reviewer: muse (muse muse-spark-1.3-contributor, effort medium).
New verb/jobs plumbing is well tested for splitting, comments, readbacks, dry runs and BigQuery final-state, and D3/D4 are fixed. Must fix before merge: lazy Spark/DataFusion .sql() frames are marked EXECUTED at submission, executescript failures record nothing, and the new fakes mask the lazy bug.
Verified fixed: D3, D4. Resolve their review threads.
Defects without a review thread
- High
benchbox/core/tuning/applied_ledger.py:476-481: D6: executescript failure records nothing instead of STATEMENT_FAILED
RecordingConnection.executescript calls the wrapped executescript first and only records EXECUTED afterwards, with no except path. If the script raises, control leaves before the recording loop, so no STATEMENT_FAILED entry is written for any split statement. A failed tuning script then leaves the ledger empty or partial, letting overall_status return noop instead of failed and hiding a tuning failure from the receipt and published outcome.
Defects outside the diff count like the others: fix them before merge.
Scope: files changed since head 14c27885d655b3b1f6d12ab5744574458b513b4b was reviewed.
Reviewers not available
- sol: absent (error)
- luna: absent (error)
There was a problem hiding this comment.
oracle-verdict: success for f203bf455a07c81063e02b862d495503cdea85d9
This result supersedes those for earlier heads; their open review threads still apply.
Tier: medium-high (264 code lines exceed 150).
Decision: SHIP.
Reviewer: muse (muse muse-spark-1.3-contributor, effort medium).
Prior D5/D6/D7 are fixed: lazy sql/query results now defer EXECUTED until an action/job completion, executescript failures record STATEMENT_FAILED per statement, and tests model lazy frames/jobs. No new must-fix defects found in the changed files; remaining wrapper gaps (unlisted frame actions, dunder passthrough) are not exercised by in-repo DDL callers.
Verified fixed: D5, D6, D7. Resolve their review threads.
Scope: files changed since head d48d18dede38965114d98373ea8c3f7992574f97 was reviewed.
The applied-tuning ledger only saw execute, cursor().execute and executescript, so tuned schema DDL on Spark, PySpark, LakeSail, Velox, DataFusion and BigQuery was never recorded and a tuned Spark run reported noop. Adapters now declare ledger_execute_verbs and the recording proxy wraps those methods. Readbacks stay unrecorded and capture failures never reach the run. BigQuery statements are recorded when the job reaches its final state, so a job that fails after submission is recorded as failed, and dry-run jobs are skipped. Multi-statement strings are split before recording, including several statements on one line, so a leading SET can no longer make a tuning statement look transient. The schema footprint now also matches PARTITIONED BY, CLUSTERED BY, DISTKEY, DISTSTYLE, DUPLICATE KEY, PRIMARY INDEX, distribution =, partitioning =, sorted_by and bucketed_by. Spark table definitions now place USING <format> before a trailing PARTITIONED BY or CLUSTER BY clause; Spark rejects the previous order.
…uning capture The recording proxy now forwards the declared SQL verb with the caller's own positional or keyword form (sql(sqlQuery=...), query(query=...)) and extracts the statement from whichever was used. Spark table definitions place USING after the real closing delimiter of the column list, ignoring parentheses inside string literals and quoted identifiers, so defaults are no longer altered. Tests that only froze verb declarations and attribute delegation are removed, along with assertions on mock return values.
…cript splitting Require bare schema-tuning keywords to sit in clause position so ordinary column names no longer record executed tuning, while every footprint keyword still records in its real clause form. Record CTAS sorts where they execute by wrapping the connection at the sort site, so the DataFusion load-time sort through execute reaches the ledger instead of staying silent. Skip line and block comments when locating the Spark column-list boundary, in addition to quoted text, so USING lands after the real closing delimiter. Exclude comment-only script fragments from recording on every proxy path, so trailing comments no longer corrupt tuning verdicts.
…ter-only fragments Recognize UNIQUE NULLS DISTINCT and NULLS NOT DISTINCT table constraints as clause syntax so executed uniqueness tuning reaches the ledger instead of being mistaken for a column name, while bare UNIQUE as a column name stays unrecorded. Treat fragments containing only statement delimiters, comments and whitespace as non-recordable on every proxy path, so empty split tails no longer corrupt tuning verdicts or the ledger hash.
f203bf4 to
53e28f2
Compare
There was a problem hiding this comment.
oracle-verdict: success for 53e28f27d6c984536283c6506dde45d1ed756326
This result supersedes those for earlier heads; their open review threads still apply.
Tier: medium-high (264 code lines exceed 150).
Decision: SHIP, carried from head f203bf455a07c81063e02b862d495503cdea85d9.
The patch is unchanged since head f203bf455a07c81063e02b862d495503cdea85d9 was reviewed, so its decision is carried to this head without running a reviewer. Its open review threads still apply.
This branch was successfully deployed
1 participant
Summary
The applied-tuning ledger only intercepted
execute,cursor().executeandexecutescript. Tuned schema DDL issued through.sql()(Spark, PySpark, LakeSail, Velox, DataFusion) or.query()(BigQuery) was never recorded, so a tuned Spark run reportednoop.ledger_execute_verbs; the recording proxy wraps those methods. The default is empty, so other adapters are unchanged.SELECT,SHOW,DESCRIBE,EXPLAIN) through the wrapped verbs execute but are not recorded. Capture errors never reach the run.result()orexception()reaches a final state, as executed or failed. Submission alone records nothing, a wait that times out is not final, and dry-run jobs are skipped._split_sql_scriptbefore recording, onexecute, cursorexecuteand the new verbs. The splitter now also separates several statements on one line, which the previous line-based loop did not.SET x=1; CREATE TABLE ... ORDER BY (z)is therefore recorded as a transientSETand a verifiableCREATE. A single statement is recorded unchanged.PARTITIONED BY,CLUSTERED BY,DISTKEY,DISTSTYLE,DUPLICATE KEY,PRIMARY INDEX,distribution =,partitioning =,sorted_byandbucketed_by.optimize_spark_table_definitionappendedUSING <format>after a trailingPARTITIONED BY/CLUSTER BY, which Spark 4.2 rejects with a parse error. It now insertsUSINGafter the column list. Statements without a trailing clause are unchanged. This was required for tuned Spark DDL to execute at all.Verification
uv run -- python -m pytest -m fast -q: 37013 passed, 26 skipped (run before the final comment and changelog edits; the changed-file tests were re-run bymake pr-preflight: 186 passed).uv run -- ruff check .andruff format --check .: clean.uv run -- ty check: no new diagnostics; the 11 in changed files are existing warnings inbigquery.py.make comment-policy-check: no violations in the changed files after the final edit;make pr-preflightpassed.tests/integration/platforms/test_spark_ledger_capture_live.pyruns the realSparkAdapter.create_schemaagainst a real session.PARTITIONED BYandCLUSTER BYDDL is recorded as executedddl, the untuned table is not, and the ledger status is no longernoop.tests/unit/core/tuning/test_ledger_execute_verbs.py) cover Spark-family, BigQuery and DataFusion schema DDL, readbacks, failure, job final state, splitting and the new footprint.--tuning tunedTPC-H SF0.01: DuckDB applied 6 statements, 20 executed, 0 failed, 0 dropped; DataFusion ran to completion with no template.Known limits
CREATEthat Spark's "already exists" retry then re-runs appears as a failed plus an executed statement, which blocks corroboration for that run.sql()is lazy for DML; DDL is eager. Only schema DDL passes the filter in the schema phase.partitioned_by(underscore) is not in the footprint list.Soundness review
This change touches
benchbox/core/tuning/applied_ledger.py, a soundness path, so it needs an external review and must not auto-merge. Points for the reviewer:applied_verifiedby itself, andoverall_statusstill returns at mostapplied_unverified.USINGplacement change is outside the ledger but is needed for the Spark acceptance.External soundness review: pending.