fix: land the merged filter work that never reached main - #29
Merged
Conversation
FilterBuilder appended the caller's terms bare, so the generated clause read "FullTypeName = ? AND Partition = ? AND term1 OR term2". AND binds tighter than OR, so SQL parsed that as "(FullTypeName AND Partition AND term1) OR (term2)": every term after the first Or() was matched against the whole table. A two-term Or() returned rows from other partitions, and rows of other stored types, which the reader deserialized as T with no error. The same clause backs DeleteObjectsAsync, so an ungrouped Or() could delete rows in other partitions and of other types, and CountObjectsAsync over-counted. The caller's filter is now emitted inside its own parentheses. Losing the Partition predicate also cost the partition-prefixed indexes, so this was a large performance regression too; a test asserts via EXPLAIN QUERY PLAN that a two-term OR-chain reaches the index again. TychoQueryable had the same failure one level down: && and || were emitted flat, so Where(x => (x.A || x.B) && x.C) became "A OR B AND C" — read as "A OR (B AND C)" — returning rows that matched only A despite failing C. Each composite boolean node is now emitted in its own group. Every regression test here seeds more than one partition and more than one type: a single-partition, single-type fixture cannot observe the difference, which is presumably why this went unnoticed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Set membership expressed as one atomic term rather than a chain of Or()s,
so the whole class of mis-grouping the previous commit fixed is
unreachable for this query shape. It renders through the same numeric
CAST the scalar comparisons use, which is what lets an expression index
over the property serve the query.
FilterBuilder<Item>.Create().Filter(FilterType.In, x => x.DepartmentId, new[] { 33, 47 });
Decisions worth reviewing:
- An empty set renders "0 = 1" for In and "1 = 1" for NotIn. IN () is a
syntax error, and silently dropping the term would widen the result
set, which is the dangerous direction.
- A null in the set becomes an IS NULL disjunct, which SQL's own IN would
never match. NotIn keeps SQL's NULL semantics — a row whose member is
null is not returned — because that is what NotEquals already does, and
diverging from its scalar sibling would be its own trap. A test asserts
the two agree.
- Lists longer than 900 values split across several IN terms rather than
exceeding SQLITE_MAX_VARIABLE_NUMBER, which is 999 on older SQLite
builds. Which build ships is the host application's choice, not ours.
- The raw-path overload takes IEnumerable<object>, not a generic
parameter: a generic overload there captures an ordinary string
comparison value, because string is IEnumerable<char>. A value-type
collection therefore needs Cast<object>(), and both the parameter docs
and the rejection message say so.
Passing a collection to a scalar FilterType now throws instead of
rendering "System.Int32[]" and matching nothing. A literal null still
binds to the new overload but keeps its old meaning as the null
comparison; two existing tests caught that when I first got it wrong, and
there is now a test naming it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Batch key reads
---------------
Reading N keys meant N round trips through the connection gate. The new
overload does it in one, binding the key set as a single JSON array
expanded by JSON_EACH rather than one parameter per key — so there is no
SQLITE_MAX_VARIABLE_NUMBER ceiling, no chunking for callers to think
about, and one prepared statement whatever the batch size. Keys lead the
PRIMARY KEY, so each expanded key is a primary-key probe.
The shape was chosen by measurement, not assumption. Query cost alone on
a 250,000-row store:
batch json_each chunked IN single IN temp table
200 0.2 ms 0.3 ms 0.3 ms 40.3 ms
999 1.0 ms 3.3 ms 4.0 ms 47.3 ms
4,949 5.6 ms 19.6 ms 65.8 ms 52.9 ms
23,784 27.5 ms 91.8 ms 1,297.7 ms 67.3 ms
A single IN collapses at scale because its statement text and plan grow
with the batch; a temp-table join carries fixed setup that never
amortises at these sizes. End to end against a loop of ReadObjectAsync,
both including deserialization: 1.9 -> 0.9 ms at 200 keys, 10.6 -> 4.5 at
999, 36.8 -> 16.9 at 4,949, 183.2 -> 67.3 at 23,784.
Keys not present are simply absent from the result, so it may be shorter
than the key set, and its order is the database's rather than the key
set's. Tests cover what the JSON encoding could break: keys carrying
quotes, backslashes, control characters, non-BMP emoji and a SQL
injection string, plus a 5,000-key set.
Counting
--------
CountObjectsAsync issued "SELECT 1 FROM JsonValue WHERE ..." and
incremented a counter once per matching row — a reader round trip per
row. It now issues SELECT COUNT(*) and reads the single scalar: 16.0 ms
-> 6.5 ms counting a 250,000-row partition. The same query backs the
pre-count a progress-reporting read performs, so those pay half of what
they did.
The benchmark harness is committed as ZzBatchKeyBench.cs, [Ignore]d so it
never runs in CI; remove the attribute to reproduce any number above.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
feat: add ReadObjectsByKeysAsync, and count in the engine
ce98d5e rewrote the set-membership doc block and removed its </para> without removing the matching <para>, leaving the summary unbalanced. That produced two CS1570 "badly formed XML" warnings (four counting the duplicated build output) on this branch and everything downstream of it. The prose the autofix introduced is kept; only the tag is restored. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR lands a previously approved stack of query/filter work that never reached main, restoring critical correctness fixes to filter composition/precedence and adding new query capabilities and optimizations.
Changes:
- Fixes filter composition so ungrouped
Or()terms cannot escape the partition/type predicates, and preserves boolean precedence when translating LINQ (&&/||) to SQL. - Adds
FilterType.In/FilterType.NotIn(set membership) andReadObjectsByKeysAsync<T>for efficient batch key reads usingJSON_EACH. - Improves counting performance by switching
CountObjectsAsyncand progress pre-counting toCOUNT(*), and updates docs/tests accordingly.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| TychoDB/TychoQueryable.cs | Wraps boolean subexpressions in groups to preserve &&/` |
| TychoDB/Tycho.cs | Adds ReadObjectsByKeysAsync, key-set JSON binding, and switches counting to COUNT(*). |
| TychoDB/Queries.cs | Introduces new SELECT/COUNT query variants for batch key reads and updates count query projection. |
| TychoDB/FilterBuilder.cs | Binds caller filter as a single conjunct; adds In/NotIn support and value resolution for sets. |
| TychoDB/Filter.cs | Extends FilterType with In and NotIn. |
| TychoDB.UnitTests/ZzBatchKeyBench.cs | Adds an ignored scratch benchmark harness for batch key read / count shapes. |
| TychoDB.UnitTests/FilterInTests.cs | Adds coverage for In/NotIn semantics, casting/index usage, null/empty/duplicates, and overload behavior. |
| TychoDB.UnitTests/FilterCompositionTests.cs | Adds regression tests for OR-leak prevention, deletion/count correctness, and LINQ precedence preservation. |
| TychoDB.UnitTests/BatchKeyReadTests.cs | Adds coverage for ReadObjectsByKeysAsync correctness, escaping, partition/type isolation, and progress. |
| README.md | Documents batch key reads, key-vs-property performance note, and set-membership examples. |
| CHANGELOG.md | Documents the critical OR precedence fix, new APIs, COUNT(*) optimization, and breaking changes. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.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
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.
Why this exists
#24, #25 and #26 were each approved and merged — but each merged child into parent, and every parent had already been merged upward before its child landed. The content never reached
main:Verified directly against each branch:
mainpr1/benchmarks-build-and-flaky-keypr2/filter-or-precedencepr3/filter-in-notinmaincurrently has neither theOr()precedence fix norIn/NotIn. The critical correctness bug — where an ungroupedOr()escaped the partition and type predicates, returning rows from other partitions, deserializing other stored types asT, and lettingDeleteObjectsAsyncdelete rows it was never asked to touch — reads as merged on GitHub and is not inmain.This PR brings
pr3/filter-in-notin, the branch that actually accumulated all four, intomain. It merges cleanly.What lands
Everything already reviewed and approved in #23–#26:
FilterType.In/NotInReadObjectsByKeysAsync, andCOUNT(*)instead of counting rows client-sidePlus every Copilot autofix already applied to those branches.
One new commit
4ca1b6crestores a closing</para>that autofixce98d5edropped while rewriting a doc block — it removed the tag without removing the matching<para>, leaving the summary unbalanced and producingCS1570badly-formed-XML warnings on this branch and everything downstream. The autofix's prose is kept; only the tag is restored. Warning count onTychoDB.csprojgoes 4 → 0.Verification
dotnet build TychoDB.sln -c Release— 0 errors, 0CS1570mainwith no conflictsNot included
pr5/key-derivation(#27) stays separate — it has unresolved conflicts and three open Copilot comments, one of which is a genuine correctness bug I'm fixing next.🤖 Generated with Claude Code