Simplify: Session, Result, Predicate, and fewer duplicates - #9
Merged
Merged
Conversation
A pass over the recent work for duplication and dead code. - Dead code: Efsql.hello/0, Efsql.stream/1, sql_to_logical/2 and Planner.to_ecto_query/1 had no callers. resolve_tenant/2 is private. - Tenant opening: Efsql.open_tenant/4 is now the one read-only opener, used by queries, the fan-out and the TUI navigator (which had its own copy). It starts a non-default storage id's tenant cache and, with check_exists, checks existence in that storage id. The CLI's storage_id.tenant.table path used to check the default storage id and never started the cache. - Efsql.Settings: one module for \set (limit, tenant_batch): parsing, validation, messages, help and query options. The CLI, the TUI and the TUI session each had their own. - Executor: every access node goes through one start-then-await path (Repo.all_range and friends are await(async_*) anyway), a NULL field failing a comparison, LIKE or IN is one helper, filter uses matches?/2, and sort/2 is public. - Aggregate orders its groups with Executor.sort/2 instead of its own NULL-aware comparator. Its equality key moves to Types.equality_key/1, next to the compare/2 it mirrors. - Render.columns/2 is the result column order for both the TUI and the CLI. For select *, the CLI now also puts id first. - Planner.indexes/2 is the one index-metadata read (Discover used its own), and Logical.take_first/2 replaces the matching helpers in Planner and Rewrite. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PFBZUrerge6bbWVCHpS5Ko
The CLI and the TUI each kept what a query runs in (the cache of open
tenants, the settings, and in the TUI the active tenant and storage id)
and each took apart a bare {plan, rows, tenants}, then asked Render and
Fanout for the columns and the transaction count.
Efsql.Session holds that state. Session.run/3 resolves the statement's
scope (a named tenant, the active tenant, or every tenant of the active
or named storage id), applies the settings as query options, runs it and
times it. It returns an Efsql.Result (rows, columns in order, the
transaction count, the plan, elapsed ms) and the session with its grown
tenant cache. Session.activate/3 is the navigator's "use this tenant".
The CLI state and the TUI model each hold one session, and the TUI model
holds the last Result instead of separate rows/columns/plan/elapsed_ms
fields. Efsql.Tui.Session is gone. Efsql.qall/3 stays as the sessionless
entry point, now built on Session.run/3. In the CLI, a table without a
tenant now gets the TUI's "no active tenant" message.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PFBZUrerge6bbWVCHpS5Ko
A predicate was a bare tuple interpreted in five places: Logical.predicate_field/1 (its field), Executor.eval (checking a row), Planner.kind/1 and range?/1 (how the planner can serve it), Planner.pred_to_expr/1 (the Ecto expression pushed to the adapter), and Logical.take_first/2. Adding a kind of predicate meant finding them all. Efsql.Predicate now owns them, one clause group per predicate type: field/1, matches?/2 (with SQL NULL semantics), pushdown/1 (:key | :in | :index | :filter), equality?/1, range?/1, take_first/2, to_ecto/1 and field_ref/1. The planner's classification becomes a group_by on pushdown/1. Rewrite keeps its rewrite passes, which transform predicates rather than interpret them. A new predicate type (NOT, <>, ...) is now one module, plus its translation and any rewrite. Row evaluation, previously tested only end to end, now has unit tests: NULL semantics, LIKE, IN, ranges, classification and the Ecto encoding. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PFBZUrerge6bbWVCHpS5Ko
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
2 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.
After GROUP BY, column order and cross-tenant queries, several concepts had ended up written two or three times. This goes through and gives each one a single home. Behavior is unchanged except for the three small differences noted at the end.
New abstractions
Efsql.Session/Efsql.Result. The CLI and the TUI each kept their own copy of what a query runs in: open tenants, settings, and (in the TUI) the active tenant and storage id. Each also took apart a bare{plan, rows, tenants}and then asked other modules for the columns and the transaction count. Now:Session.run(session, sql)works out which tenants the statement reads (a named tenant, the active one, or every tenant of a storage id), applies the settings, and runs and times the query.%Result{rows, columns, transactions, plan, elapsed_ms}and the updated session.Session.activate/3is the navigator's "use this tenant".The CLI state and the TUI model each hold one session, and the TUI holds the last
Result.Efsql.Tui.Sessionis gone.Efsql.qall/3stays as the sessionless entry point, now built onSession.run/3.Efsql.Predicate. AWHEREcondition was a bare tuple that five places interpreted separately: its field, checking it against a row, how the planner serves it, the Ecto expression pushed to the adapter, and a list helper. They're all in one module now:field/1,matches?/2,pushdown/1(:key | :in | :index | :filter),equality?/1,range?/1,to_ecto/1. AddingNOT,<>and the like is now one module plus the translation, instead of five edits.Efsql.Settings.\set limitand\set tenant_batchwere parsed three times (CLI, TUI, TUI session), each with its own messages and its own way of turning settings into query options. Now one module does it.Unified
Efsql.open_tenant/4is now the one read-only opener: queries, the cross-tenant fan-out and the TUI navigator all use it. This fixed a bug: a CLI query onstorage_id.tenant.tablechecked that the tenant existed in the default storage id, and never started the named storage id's tenant cache.all_rangeand friends areawait(async_*)anyway.sort/2, which already handles NULLs, instead of carrying its own comparator. The "values that compare equal group together" key moved toTypes.equality_key/1, next tocompare/2.Render.columns/2.Planner.indexes/2, whichDiscovernow uses too.Removed
Efsql.hello/0,Efsql.stream/1,sql_to_logical/2andPlanner.to_ecto_query/1had no callers.Behavior differences
select *, the CLI now putsidfirst, like the TUI.\setusage messages are slightly reworded.Tests
New unit tests for
Predicate,Settings,Session/Result,Render.columns/2,Types.equality_key/1andtake_first/2. ThePredicatetests are the first unit tests for evaluating conditions against rows (NULL handling, LIKE, IN, ranges); before, that was only covered by the FoundationDB tests.All 295 tests that don't need FoundationDB pass locally, with no new compiler warnings. This CI run is the first time the FoundationDB suite sees the shared read path and the planner changes.
🤖 Generated with Claude Code
https://claude.ai/code/session_01PFBZUrerge6bbWVCHpS5Ko
Generated by Claude Code