fix(optimizer): handle non-commutative subtraction in simplify_equality - #7791
Merged
georgesittas merged 2 commits intoJun 24, 2026
Merged
Conversation
geooo109
approved these changes
Jun 24, 2026
geooo109
left a comment
geooo109
left a comment
Collaborator
There was a problem hiding this comment.
Nice work, LGMT!
left a small comment.
georgesittas
approved these changes
Jun 24, 2026
georgesittas
left a comment
georgesittas
left a comment
Collaborator
There was a problem hiding this comment.
Thanks, great catch!
code-alexander
added a commit
to monospaceai/evaldata
that referenced
this pull request
Jul 19, 2026
* chore: upgrade sqlglot to 30.12.0 and drop simplifier patch The upstream constant-minus-variable fold bug (0 - a = 1) is fixed in sqlglot 30.12.0 (tobymao/sqlglot#7791). Remove _PatchedSimplifier and its canary test, raise the floor to sqlglot>=30.12.0, and call the stock Simplifier directly. The two behavioural tests remain as regression cover. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat: pydantic-evals integration Add `evaldata.pydantic_evals.SqlEquivalence`, a drop-in Pydantic Evals Evaluator that scores generated SQL by executing it against a real warehouse and checking row/semantic equivalence via observed_equivalence(). - Optional `pydantic-evals` extra (added to `all`). - Reads ctx.output (generated SQL) and ctx.expected_output (gold SQL str or an evaldata GoldQuery/UntypedResultSet/TypedResultSet); returns an EvaluationReason(value, reason) with a composed diff-aware reason. - Scores under a module lock: Pydantic Evals runs cases concurrently but a resolved adapter's connection is not thread-safe. - Unit tests on DuckDB (incl. an end-to-end Dataset.evaluate_sync run), 100% line+branch coverage; guide + reference docs and nav. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat: concurrent warehouse execution via per-platform connection pool Make query execution safe to run concurrently across eval cases so an outer scheduler (Pydantic Evals max_concurrency, run_benchmark) genuinely parallelizes execution and scoring — removing the bridge's serialization caveat. Design: a per-platform-name ConnectionPool. Each case acquires a session member for its whole execute+score pipeline and releases it; members are built lazily, so serial callers reuse one. Keyed by case, not thread, because execute_within_budget runs each query on an ephemeral watchdog thread (thread-local keying would thrash). - platforms/pool.py: bounded pool (acquire/release/utility/close), Condition-based blocking, closed-flag wakes waiters, dedupes shared member/utility on close. - registry.py: pools replace the single-adapter cache; resolve() returns a dedicated utility adapter (never a checkout member); acquired() context manager; module lock around pool creation. Per-engine sizes (DuckDB 8, cloud 4, SQLite 1 serial). - DuckDB: shared parent connection, each member a .cursor() of it (shares the in-process DB; concurrent cursor queries run in parallel and .interrupt() is isolated per cursor — both verified empirically, guarded by a canary test). - Dedicated-connection engines (Postgres/Snowflake/BigQuery/Databricks): each member its own connection; the shared _cursor/_job slot is safe under exclusive checkout. - runner.py: _score_output acquires a session (try/finally release); run_benchmark parallelizes the whole per-case pipeline, preserving case order. - pydantic_evals: SqlEquivalence offloads to a worker thread and drops the global lock; the pool provides safety. Docs: caveat replaced with a Concurrency section. - Tests: pool units, DuckDB concurrency + interrupt-isolation canary, bridge async offload + concurrent dataset (free); bounded Postgres e2e and Snowflake smoke behind their markers. 100% line+branch coverage. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix: address concurrency review findings - run_benchmark: revert to parallel-solver + serial, case-ordered scoring. Running the whole pipeline in workers concurrently re-entered shared scorer instances, whose protocol has no thread-safety contract. Concurrent execution lives in the Pydantic Evals bridge (which only uses the stateless built-in scorer). Concurrency correctness is now exercised via concurrent evaluate_case. - sqlite: give the pool distinct connections over a per-name shared-cache in-memory URI, so the utility adapter is no longer also a checkout member (upholding the pool invariant) while members still share seeded data. - pool: utility() rejects use after close (no resurrected adapter); release() after close drops the member instead of requeuing or double-closing it. - pydantic_evals: failure reason now includes column, column-order, type, and per-column value mismatches, not just row counts. - duckdb interrupt-isolation canary: use two genuinely slow overlapping queries so the sibling is provably still running when the other cursor is interrupted. - docs: correct the session-state note (pooled sessions are reused and not reset, so session-local state can leak across cases) and note the uncancellable-query and stale-connection edge cases. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * chore: refine platform lifecycle handling * docs: improve pydantic evals guide * chore: add 0.8.0 changelog entries * refactor: tighten pool lifecycle state * docs: update README * fix: correct cloud test setup --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
simplify_equalityuses the addition and subtraction properties of (in)equality to cancel a constant operand and leave the variable on its own, sox + 1 = 3becomesx = 2.It breaks when the variable is the right operand of a subtraction. A constant on the left is handled by swapping the two operands and reusing the same rewrite, which is only valid when the operator commutes. Subtraction doesn't, so
0 - ais treated likea - 0: the sign is dropped, and inequalities aren't reversed.1 = 0 - a -> a = 1 (should be a = -1)
5 - x > 2 -> x > 3 (should be x < 3)
For subtraction the fix isolates the variable directly instead of swapping. No fixtures covered the constant-minus-variable shape, which is how it slipped through, so I added them across the six comparison operators.