fix: bind the caller's filter as a single conjunct, and derive keys honestly

[michaelstonis]

Prompted by an external bug report evaluating TychoDB 5.1.4 as a query engine for a 257,974-row item master. The reported bug reproduced, turned out to be worse than reported, and pulled in a family of related problems.

1. Critical: an ungrouped Or() escaped the partition and type predicates

FilterBuilder appended the caller's terms bare, so the generated clause read:

WHERE 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.

var f = FilterBuilder<ItemModel>.Create()
    .Filter(FilterType.Equals, x => x.DepartmentId, 33).Or()
    .Filter(FilterType.Equals, x => x.DepartmentId, 47);

await db.ReadObjectsAsync<ItemModel>("partitionA", f);
// was: ids [1, 2, 4]  — id 4 lives in partitionB

With two types in one partition it returned a VendorModel deserialized as an ItemModel, with no exception.

Beyond the report: 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 as well — a grouped OR-chain uses the index again (EXPLAIN QUERY PLAN asserted in tests).

2. TychoQueryable lost precedence inside a predicate

Found while investigating the above. && and || were emitted flat, so Where(x => (x.A || x.B) && x.C) became A OR B AND C — read by SQL as A OR (B AND C) — returning rows matching only A despite failing C. Each composite boolean node is now emitted in its own group.

3. FilterType.In / NotIn

Set membership as one atomic term, so it cannot be mis-grouped the way an Or() chain can, rendered through the same numeric CAST the scalar comparisons use so an expression index still serves it.

• Duplicates removed; caller order preserved.
• Empty set matches nothing (In) / everything (NotIn) — never IN (), a syntax error, and never a silently dropped term, which would widen results.
• A null in the set is tested with IS NULL, which SQL's own IN would never match. NotIn keeps SQL's NULL semantics, matching what NotEquals already does (pinned by a test asserting the two agree).
• Long lists split across several IN terms rather than exceeding SQLITE_MAX_VARIABLE_NUMBER, which is only 999 on older SQLite builds.

4. ReadObjectsByKeysAsync<T>

Batch key reads in one round trip. The key set binds as a single JSON array expanded by JSON_EACH, not one parameter per key — no ceiling, no chunking, one prepared statement regardless of batch size.

The shape was chosen by measurement, not assumption (250,000-row store, best of five after warm-up):

batch	raw json_each	raw chunked IN	raw 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

End to end against a loop of ReadObjectAsync (both include deserialization, which dominates the remainder):

batch	looped	ReadObjectsByKeysAsync
200	1.9 ms	0.9 ms
999	10.6 ms	4.5 ms
4,949	36.8 ms	16.9 ms
23,784	183.2 ms	67.3 ms

5. CountObjectsAsync stopped counting rows on the client

It issued SELECT 1 FROM JsonValue WHERE … and incremented a counter once per matching row — a reader round trip per row. Now SELECT COUNT(*): 16.0 ms → 6.5 ms counting a 250,000-row partition. The same query backs the pre-count a progress-reporting read performs.

6. Key derivation

Three findings that compose:

AddTypeRegistration<T>() documented convention-based ID detection and did none. It recorded no selector, so WriteObjectAsync(obj), ReadObjectAsync(obj), ObjectExistsAsync(obj), DeleteObjectAsync(obj) and GetIdFor(obj) all threw An id mapping has not been provided — on a type whose property was literally named Id. It now finds Id, then <TypeName>Id, case-insensitively. A type with no such property still registers without a mapping, so registering a key-less type and supplying keys at the call site keeps working.

Strict registration now rejects a divergent key. WriteObjectsAsync(objs, keySelector, …) overrides the registration, and a row written under a key the registration would not produce is unreachable by every by-object overload — DeleteObjectAsync(obj) returns false while the row survives. Under requireTypeRegistration that now throws, naming both keys. The check wraps the selector rather than pre-scanning, so a lazy sequence is enumerated exactly once.

That guarantee makes a rewrite sound. Equals/In filters on the id property are answered from the indexed Key column instead of a JSON_EXTRACT scan:

filter on the id property, 250k rows	scan	rewritten
Equals	79.3 ms	0.0 ms
In, 100 keys	101.2 ms	0.2 ms

Rows already in the database are checked once per type by a divergence probe (~92 ms, lazy, cached for the connection). A single divergent row disables the rewrite for that type and the ordinary predicate is emitted, so the worst case is the behaviour that was there before. Negations are left alone (they cannot use an index either way), as is a null comparison value (Key is NOT NULL).

Breaking changes

• An ungrouped Or() now means what it reads as. Code that depended on the leaked rows returns fewer rows. This is the fix, not a regression.
• Passing a collection to a scalar FilterType throws. It previously bound to object and rendered as "System.Int32[]", matching nothing silently. Use FilterType.In. A literal null also binds to the new overload but keeps its old meaning — Filter(Equals, x => x.Value, null) is still the null comparison (regression test included; two existing tests caught this when I first got it wrong).
• AddTypeRegistration<T>() on a type with a conventional id now supplies a key. Previously every by-object call threw; they now work.

Verification

• 291 pass / 4 skipped, repeated runs; dotnet build TychoDB.sln -c Release clean.
• Library warning profile byte-identical to main.
• Verified the committed tree is green on its own, independent of unrelated working-tree changes.
• Benchmarks live in ZzBatchKeyBench.cs, [Ignore]d so they never run in CI — remove the attribute to reproduce any number above. Happy to drop that file if you'd rather it stayed local.

Notes

• All measurements are against synthetic 250k-row stores seeded locally, not the reporter's real 291 MB item master. The shapes should hold, but the absolute numbers are from my machine.
• Also included: two pre-existing repairs in their own commit — TychoDB.Benchmarks had not compiled since 123c79f (so dotnet build TychoDB.sln failed outright), and a test keyed 1000 objects by GetHashCode(), which collides 0.80% of the time (measured over 2000 trials) and failed the suite at random.
• This is large enough to reasonably be several PRs; say the word and I'll split it.

🤖 Generated with Claude Code

[Copilot]

Pull request overview

This PR fixes several correctness and safety issues in TychoDB query/filter generation (notably OR-precedence escaping partition/type predicates) and adds performance-oriented query features (set-membership filters, batch key reads, server-side counts) plus stricter, more “honest” key/ID derivation under type registration.

Changes:

• Correctly preserves boolean precedence by grouping emitted filter/LINQ boolean nodes, preventing OR terms from escaping partition/type constraints.
• Adds FilterType.In/NotIn, ReadObjectsByKeysAsync<T>(), and rewrites eligible ID-property filters to the indexed Key column when sound under strict registration.
• Improves counting performance by switching to COUNT(*), and expands tests/docs/changelog to cover the new behavior and APIs.

Reviewed changes

Copilot reviewed 16 out of 16 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
TychoDB/TychoQueryable.cs	Groups composite && / `
TychoDB/Tycho.cs	Adds batch key reads, key-column rewrite plumbing, strict key divergence guard, and uses COUNT(*) for counts.
TychoDB/RegisteredTypeInformation.cs	Implements convention-based ID property detection and captures ID path segments for later query-time rendering.
TychoDB/Queries.cs	Adds SQL for batch key reads, count-by-keys, and key/id divergence probing; switches count query to COUNT(*).
TychoDB/KeyColumnRewrite.cs	Introduces cached, per-type rewrite eligibility probing to safely use Key column for eligible ID filters.
TychoDB/FilterBuilder.cs	Binds caller filters as a single conjunct, adds In/NotIn, chunks large IN lists, and supports key-column rewrite emission.
TychoDB/Filter.cs	Extends FilterType with In / NotIn.
TychoDB.UnitTests/ZzBatchKeyBench.cs	Adds an ignored scratch benchmark harness for key batch read/query-shape comparisons.
TychoDB.UnitTests/TychoDbTests.cs	Fixes flaky test keying caused by GetHashCode() collisions.
TychoDB.UnitTests/KeyRegistrationTests.cs	Adds coverage for convention registration, strict-mode divergence guarding, and key-column rewrite behavior.
TychoDB.UnitTests/FilterInTests.cs	Adds behavioral and query-shape tests for In/NotIn semantics, chunking, null handling, and index usability.
TychoDB.UnitTests/FilterCompositionTests.cs	Adds regression tests ensuring OR composition can’t escape partition/type predicates; verifies LINQ precedence.
TychoDB.UnitTests/BatchKeyReadTests.cs	Adds correctness tests for ReadObjectsByKeysAsync (partition/type safety, JSON metacharacters, large batches, progress).
TychoDB.Benchmarks/Diagnostics.cs	Updates benchmark diagnostics to pass serializer into sort building after signature change.
README.md	Documents ReadObjectsByKeysAsync, ID-property filter rewrite behavior under strict mode, and performance guidance.
CHANGELOG.md	Records the precedence/security fixes, new APIs, performance improvements, and breaking-change notes.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[michaelstonis]

Superseded by a stacked split, as requested. Same final tree — I verified the tip of the last branch is byte-identical to this one.

Review in order; each is based on the one before:

1. fix: repair the benchmarks build and a flaky identity-hash key #23 — fix: repair the benchmarks build and a flaky identity-hash key (base main)
2. fix: bind the caller's filter as a single conjunct #24 — fix: bind the caller's filter as a single conjunct — the correctness bug
3. feat: add FilterType.In and FilterType.NotIn #25 — feat: add FilterType.In and FilterType.NotIn
4. feat: add ReadObjectsByKeysAsync, and count in the engine #26 — feat: add ReadObjectsByKeysAsync, and count in the engine
5. feat: derive keys honestly, and use the id property that follows from it #27 — feat: derive keys honestly, and use the id property that follows from it

Each branch builds TychoDB.sln -c Release clean and passes its tests independently (227 / 237 / 262 / 275 / 294, growing as each layer adds coverage).

Copilot's three review comments here have been addressed in the split — two applied, one declined with evidence:

• Duplicated <summary> block (FilterBuilder.cs) — correct, my bug: a method I inserted took ResolveValue's doc comment with it. Reattached, and I scanned the rest of the library for the same mistake (no others). Fixed in feat: add FilterType.In and FilterType.NotIn #25.
• CanRead doesn't imply a public getter — premise correct, predicted harm not. Such a property is picked up by a Public lookup and CanRead is true, but the expression compiles and works rather than failing at runtime. The real problem is that neither serializer emits a property without a public getter, so the id would be absent from the stored document. Now requires a public getter, for that reason. Fixed in feat: derive keys honestly, and use the id property that follows from it #27.
• Raw-path overload should take a generic/non-generic IEnumerable — declined, with evidence. A generic overload there captures an ordinary string comparison value, since string is IEnumerable<char>, routing Filter(Equals, "$.name", …, "abc") to set membership. Verified by probe. The stated harm ("reintroducing the old silent System.Int32[] behavior") also doesn't apply — an uncast int[] throws rather than matching nothing. Kept IEnumerable<object>; the parameter docs and the rejection message now tell callers to Cast<object>(). In feat: add FilterType.In and FilterType.NotIn #25.