fix: land the key-derivation work that never reached main - #30
Merged
Merged
Conversation
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>
Three findings that compose; each one is what makes the next sound. AddTypeRegistration<T>() documented convention-based ID detection and did none of it. 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, requiring a public getter: CanRead is true for a private getter, and such a property is not emitted by either serializer, so its JSON path would match nothing. 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. 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, which is a silent failed delete. 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; a test asserts that. That guarantee is what makes a rewrite sound. Equals and In filters on the id property are now answered from the indexed Key column instead of a JSON_EXTRACT scan: 79.3 -> 0.0 ms for Equals, 101.2 -> 0.2 ms for In over 100 keys, on a 250,000-row store. Soundness has two halves. The guard means no row written through this instance can diverge. Rows already in the database — written by an older version, or outside strict mode — 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. A test seeds a deliberately divergent legacy row, reopens in strict mode, and asserts the query still finds it. Negations are deliberately left alone — a negated predicate cannot use an index either way — as is a null comparison value, since Key is NOT NULL: "the id is null" is a question about the document, not the key. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Retargeted onto the branch that carries PR1-PR4 into main, rather than pr4/batch-key-reads, so this lands in main instead of into another stack branch that main has not taken. Conflicts, all mechanical: - Tycho.cs (3): took the base's checked((int)GetInt64(0)) for the count — COUNT(*) is 64-bit and the original GetInt32 would have thrown past 2^31 rows — and kept this branch's keyRewrite plumbing. - CHANGELOG.md (4): kept this branch's key-derivation entries; took the base's reflow, and its correction to the SQLITE_MAX_VARIABLE_NUMBER claim. That limit is statement-wide, so chunking an IN list does not reduce the parameter count for parameterized values; my original text said otherwise and was wrong. The chunking still earns its place — it took 23,784 keys from 1,297.7 ms to 91.8 ms by keeping each IN list small enough for the planner — just not for the reason I gave. - README.md, ZzBatchKeyBench.cs (1 each): this branch is the superset. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The chunked Key IN chain was not bound together. Several IN terms joined by OR, unwrapped, let a following AND term capture only the last chunk: "Key IN (a) OR Key IN (b) AND other" reads as "Key IN (a) OR (Key IN (b) AND other)", so every id in the earlier chunks came back regardless of the other term. That is the same precedence bug this stack exists to fix, reintroduced one level in — BuildSetFilter wraps its chunks and this path did not. It needs more than 900 values in an In on the id property, in strict mode, combined with another term, and it is silent when it hits. The regression test fails without the fix and passes with it. The rewrite cache was never invalidated. AddTypeRegistration is an indexer assignment, so a type can be re-registered against a different id property; the cached resolved path would then point at a property the stored keys no longer come from, and the divergence probe would not re-run if it had already returned a clean verdict. All three registration methods now drop the cached entry. The divergence probe ignored the configured command timeout. It is a scan, which makes it the command most likely to need a raised timeout rather than the provider default; it now runs under Tycho's _commandTimeout, threaded through the rewrite rather than through five state tuples. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
feat: derive keys honestly, and use the id property that follows from it
Contributor
There was a problem hiding this comment.
Pull request overview
This PR brings the previously-reviewed “key derivation + key-column rewrite” work onto main, restoring missing functionality around conventional ID registration, strict-mode key consistency, and the performance rewrite that allows Equals/In filters on the ID property to use the indexed Key column when it’s proven safe.
Changes:
- Add convention-based ID detection to
AddTypeRegistration<T>()(Id/<TypeName>Id) and store unresolved ID-path segments for serializer-aware rendering at query time. - Introduce strict-mode guarding to reject call-site key selectors that diverge from the registered ID property, and add the
KeyColumnRewriteoptimization with divergence probing + re-registration cache invalidation. - Extend filter-building to optionally rewrite eligible ID filters onto
Key, including correct parenthesization for chunkedIN (...) OR IN (...)chains, and add extensive tests + docs/changelog updates.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| TychoDB/Tycho.cs | Adds rewrite caching + invalidation, wires rewrite into read/count/delete paths, and adds strict-mode key divergence guard. |
| TychoDB/RegisteredTypeInformation.cs | Implements conventional ID discovery and persists ID path segments for serializer-resolved query rewriting. |
| TychoDB/Queries.cs | Adds SQL for the “key diverges from ID property” probe used to validate rewrites against existing data. |
| TychoDB/KeyColumnRewrite.cs | New component that probes once per type (per connection) and enables/disables the rewrite accordingly. |
| TychoDB/FilterBuilder.cs | Adds optional key rewrite support and emits Key = ... / Key IN (...) when targeting the verified ID path. |
| TychoDB.UnitTests/ZzBatchKeyBench.cs | Adds an ignored benchmark harness for measuring rewrite/probe performance impact. |
| TychoDB.UnitTests/KeyRegistrationTests.cs | New test suite covering convention registration, strict-mode guard behavior, rewrite correctness, and regression coverage for chunked-IN precedence. |
| README.md | Updates guidance to reflect that ID-property filtering can be rewritten under strict registration when safe. |
| CHANGELOG.md | Documents added behavior, performance impact, and breaking changes attributable to the landed work. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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
#27 was approved and merged — but it merged into
sync/pr3-to-mainat 15:40, andmainhad already absorbed that branch via #29 at 15:08. Merging into a branchmainhas already taken doesn't reachmain. Same trap that stranded #24–#26, one layer up.Verified against
mainas it stands today:mainOr()parenthesisation + LINQ precedenceFilterType.In/NotInReadObjectsByKeysAsync,COUNT(*), 64-bit count readSo
mainis currently missing:KeyColumnRewrite—Equals/Infilters on the id property answered from the indexedKeycolumn (79.3 ms → 0.0 ms;Inover 100 keys 101.2 ms → 0.2 ms)AddTypeRegistration<T>(), which previously documented a feature it did not implementDeleteObjectAsync(obj)can returnfalseand leave the rowKey IN (…) OR Key IN (…)parenthesisation fix, which Copilot correctly identified as a real correctness bug — the same precedence defect the rest of this work exists to fix, reintroduced one level inCommandTimeoutWhat this is
sync/key-derivation-to-mainissync/pr3-to-mainatce00d25— 6 commits ahead ofmain, everything already reviewed in #27. Renamed only because the old name says "pr3" while the branch now carries PR5, which would mislead anyone reading the history.No new work. Nothing here has not already been through review.
Verification
Run against this exact branch, both configurations:
dotnet build TychoDB.sln -c Release— 0 errors, 0CS1570mainwith zero conflictsmainFor contrast,
maintoday is 275 pass / 3 skipped — the 21 missing tests areKeyRegistrationTests, including the regression test for the chunked-INbug (verified to fail with the fix reverted and pass with it applied).After this merges
pr1/…,pr2/…,pr3/…,pr4/…,pr5/…,sync/pr3-to-main,sync/key-derivation-to-mainandfix/filter-composition-and-key-derivationall become redundant — every one is a subset ofmainat that point. Worth deleting so nothing stale can be re-merged.🤖 Generated with Claude Code