fix(parity): close P10/P6/P12 + make history test reliable cross-platform#38
Merged
Merged
Conversation
… in expr)
Three DuckDB-parity gaps, each now AGREEs and is locked by the --check gate
(corpus expectations dropped, SQL_PARITY.md entries flipped to FIXED). Board
goes 72->76 AGREE, 10->6 GAP.
P10 — HAVING NOT (...): added a `Not` arm to ArithmeticEvaluator::evaluate that
negates via the existing to_bool helper, with SQL three-valued logic on the NULL
edge (NOT NULL -> NULL). Was erroring "Unsupported expression type ... Not".
P6 — INTERSECT / EXCEPT: filled the two return-Err stubs in the set-op loop.
Both DISTINCT-by-default, filtering the materialized left table against the
right using the same Debug row-key apply_distinct uses for UNION, deduping
inline. The `except` corpus case threshold moved 2000 -> 3000 so it yields a
real {Oceania} difference instead of a trivially-empty result.
P12 — WITH in expression position: parse_subquery already dispatched a leading
WITH to the CTE parser; widened the subquery-detection guards from Token::Select
to Token::Select | Token::With in primary.rs (scalar + tuple IN) and
comparison.rs (IN / NOT IN). Retires previously-unreachable CTEHoister arms.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
…tform The test sandboxed the history dir by redirecting HOME/APPDATA, but dirs::data_dir() on Windows resolves via the Win32 known-folder API and ignores those env vars, so the test silently read the developer's real history and failed. Env-var redirection is also process-global, so #[serial] (which only serializes serial-tagged tests) couldn't stop a concurrent test from writing into the sandbox and tripping the history-protection guard — hence pass-alone / fail-in-suite flakiness. Decouple the test from global state: add CommandHistory::with_history_file(path) and have new() delegate to it. The test now builds against an explicit tempdir file — no env var, no #[serial], no cross-test races. Verified green across repeated full `cargo test --test integration` runs. Also add a SQL_CLI_DATA_DIR override to AppPaths (honored directly, so it works on every platform unlike the dirs-based env vars) — a legitimate way to relocate/sandbox app data. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
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
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.
Summary
Clears three DuckDB-parity gaps and fixes a long-standing flaky test. Parity board moves 72 → 76 AGREE, 10 → 6 GAP; each fixed case has its corpus
expectdropped so the--checkgate now locks the gain.Two logically separate commits:
1.
fix(parity)— P10, P6, P12HAVING NOT (…)errored (Unsupported expression type … Not)Notarm toArithmeticEvaluator::evaluate, negating via the existingto_boolhelper with SQL three-valued logic on the NULL edge (NOT NULL → NULL).INTERSECT/EXCEPTunimplemented (return Errstubs)apply_distinctuses for UNION, deduping inline.WITHrejected in expression positionparse_subqueryalready dispatched a leadingWITHto the CTE parser; widened the subquery-detection guards fromToken::SelecttoToken::Select | Token::Withinprimary.rs(scalar + tupleIN) andcomparison.rs(IN/NOT IN). Retires previously-unreachableCTEHoisterarms.Corpus note: the
exceptcase threshold moved2000 → 3000— every region has a sale > 2000, so the original form was trivially empty and would have AGREEd for the wrong reason;> 3000leaves{Oceania}, a genuine left-minus-right difference.docs/SQL_PARITY.mdentries flipped to 🟢 FIXED with rationale.2.
test(history)— reliable cross-platformhistory_protection_integrationsandboxed the history dir by redirectingHOME/APPDATA, butdirs::data_dir()on Windows resolves via the Win32 known-folder API and ignores those env vars, so the test silently read the developer's real history and failed. Env-var redirection is also process-global, so#[serial](which only serializes serial-tagged tests) couldn't stop a concurrent test from writing into the sandbox and tripping the history-protection guard — hence pass-alone / fail-in-suite flakiness.Fix: added
CommandHistory::with_history_file(path)and hadnew()delegate to it. The test now builds against an explicit tempdir file — no env var, no#[serial], no cross-test races. Also added aSQL_CLI_DATA_DIRoverride toAppPaths(honored directly, so it works on every platform) as a legitimate way to relocate/sandbox app data.Testing
76 AGREE / 1 DIFFER / 6 GAP, contract holds (83 cases).cargo test --lib: 687 passed, 0 failed.cargo test --test integration: 407 passed, 0 failed — verified green across three repeated full runs (previously flaky).cargo fmtclean.🤖 Generated with Claude Code