Repository navigation
R13 slice 3: one evaluator construction path, case-insensitive mode honoured - #86
Merged
Merged
Conversation
…ice 3) ArithmeticEvaluator construction rebuilt the function registry, both aggregate registries and the window registry every time. The registries are immutable after construction, so build them once per process in a OnceLock and hand each evaluator Arc clones. No-op: parity output byte-identical to main (174 AGREE / 204). Per-group construction in GROUP BY was the visible cost: over trades_100000.csv, net of ~3 s load, a high-cardinality GROUP BY drops 3.4 s -> 1.9 s and GROUP BY + HAVING 2.5 s -> 0.8 s. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01HX4HwUUyg1k6PDTFQm5Gha
ArithmeticEvaluator had three constructors, new, with_date_notation and with_date_notation_and_registry, and the only thing separating them was a date notation string stored in a field nothing ever read. Dates parse through the global notation. The registry variant had no callers. Collapse to ArithmeticEvaluator::new(table) plus the existing with_visible_rows / with_table_aliases builders, delete the dead field, the now-unused date_notation parameter on apply_group_by_expressions and QueryEngine's own unread date_notation field. Also hoist the evaluator out of the per-row loop in estimate_group_cardinality, matching the key loop in group_by_expressions. No-op: parity output byte-identical to main (174 AGREE / 204); cargo test green, evaluator matrix included. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01HX4HwUUyg1k6PDTFQm5Gha
…ice 3b) Under the engine's case-insensitive mode the WHERE evaluator folds case, but the value evaluator has no such mode, so the same predicate answers differently in SELECT, HAVING, aggregate arguments and CASE WHEN - and in WHERE too when the predicate sits under a CASE, which WHERE hands to an inner value evaluator. Extend the evaluator matrix with a case-insensitive run over the string column (10 predicates, expectations from DuckDB with NOCASE collation / ILIKE) and add clause-level cases through QueryEngine::with_case_insensitive(true). Today's answers are recorded as KNOWN, so the fix has to flip exactly these. Test-only; no engine change. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01HX4HwUUyg1k6PDTFQm5Gha
…ice 3b) Hand the engine's case-insensitive mode to every ArithmeticEvaluator construction site through a new with_case_insensitive builder, and use it in comparisons, IN, BETWEEN and LIKE. Under --case-insensitive a predicate in SELECT, HAVING, aggregate arguments or CASE WHEN now answers as it does in WHERE; so does WHERE itself when the predicate sits under a CASE, which it evaluates through its inner value evaluator. Flips exactly what the previous commit pinned: all 11 matrix entries and all 5 clause-level cases. Parity report byte-identical (the harness runs case-sensitive); cargo test, Python and FORMAL examples green. Not covered, recorded in R13: simple CASE x WHEN (own values_equal) and method calls like .Contains() still compare case-sensitively outside WHERE. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01HX4HwUUyg1k6PDTFQm5Gha
Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01HX4HwUUyg1k6PDTFQm5Gha
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
R13 slice 3 (docs/ENGINE_REFACTORING.md § R13), in the usual pattern: no-op commits first, then a pinned change that alters answers.
No-op (parity report byte-identical to main, 174 AGREE / 204)
e9e5269Evaluator registries (function, both aggregate, window) are built once per process in aOnceLockand shared viaArc. Overtrades_100000.csv, net of ~3 s load: high-cardinality GROUP BY 3.4 s -> 1.9 s, GROUP BY + HAVING 2.5 s -> 0.8 s.56e41ffOneArithmeticEvaluator::new. The three constructors differed only by a date-notation field nothing read; it is deleted along withQueryEngine's unreaddate_notation. The per-row evaluator inestimate_group_cardinalityis hoisted.3b: case-insensitive mode (changes answers only under
--case-insensitive)cc37217Pin: case-insensitive run of the evaluator matrix (10 predicates, expectations from DuckDB withCOLLATE NOCASE/ILIKE) plus 5 clause-level cases throughQueryEngine::with_case_insensitive(true). 11 + 5 divergences recorded.9fc3a73Fix:with_case_insensitivepassed in at all 16 construction sites and used in comparisons, IN, BETWEEN and LIKE. Changes exactly the pinned cases; parity unchanged.f595923Docs.Still case-sensitive outside WHERE, recorded for later slices: simple
CASE x WHEN(slice 4) and method calls like.Contains()(slice 5).Test plan
--checkreport byte-identical after each commit./run_all_tests.sh: Rust, Python (572 passed), all FORMAL examples pass (the one SMOKE failure,expander_rewritersquery 7, also fails on main)--case-insensitive -q "SELECT status = 'ACTIVE'"now returns true forActive🤖 Generated with Claude Code
https://claude.ai/code/session_01HX4HwUUyg1k6PDTFQm5Gha