feat: derive keys honestly, and use the id property that follows from it - #27
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>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR tightens and clarifies Tycho’s key derivation rules (including convention-based Id detection), enforces key/registration consistency under requireTypeRegistration, and leverages that guarantee to safely rewrite eligible id-property filters onto the indexed Key column for large query performance gains.
Changes:
- Implement convention-based id-property detection in
AddTypeRegistration<T>()(Id/<TypeName>Id, case-insensitive, public getter required). - In strict mode, guard call-site key selectors to reject writes whose keys diverge from the registered id property.
- Add a per-type “key-column rewrite” that can rewrite
Equals/Infilters on the id property to use theKeycolumn after a one-time divergence probe (with fallback to legacy behavior).
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| TychoDB/Tycho.cs | Adds strict-mode key divergence guard and integrates key-column rewrite plumbing into reads/counts/deletes. |
| TychoDB/RegisteredTypeInformation.cs | Adds convention-based id detection and stores id path segments for serializer-resolved query-time rendering. |
| TychoDB/Queries.cs | Introduces divergence-probe SQL used to validate rewrite soundness against existing data. |
| TychoDB/KeyColumnRewrite.cs | New helper that caches divergence-probe verdicts per type name for a connection lifetime. |
| TychoDB/FilterBuilder.cs | Adds optional key-column rewrite support and emits rewritten predicates for eligible id-property filters. |
| TychoDB.UnitTests/ZzBatchKeyBench.cs | Adds ignored scratch benchmark harness for rewrite performance measurement. |
| TychoDB.UnitTests/KeyRegistrationTests.cs | Adds tests covering convention registration, strict-mode divergence guard, and rewrite correctness/fallback behavior. |
| README.md | Updates docs to reflect when id-property filters can be answered from Key (strict + property registration). |
| CHANGELOG.md | Documents new behavior, performance impact, and breaking-change implications. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+777
to
+789
| commandBuilder.Append(KeyColumn).Append(InOperator); | ||
|
|
||
| for (var i = start; i < end; i++) | ||
| { | ||
| if (i > start) | ||
| { | ||
| commandBuilder.Append(ValueSeparator); | ||
| } | ||
|
|
||
| commandBuilder.Append(parameters.Add(values[i]!.ToString())); | ||
| } | ||
|
|
||
| commandBuilder.Append(CloseParen); |
Comment on lines
+2573
to
+2580
| return _keyColumnRewrites.GetOrAdd( | ||
| typeof(T), | ||
| static (_, state) => | ||
| new KeyColumnRewrite( | ||
| QueryPropertyPath.RenderPath( | ||
| state.Segments, | ||
| QueryPropertyPath.AsNameResolver(state.Serializer))), | ||
| (Segments: rti.IdPropertyPathSegments, Serializer: _jsonSerializer)); |
Comment on lines
+61
to
+65
| using var command = connection.CreateCommand(); | ||
|
|
||
| #pragma warning disable CA2100 // The path is rendered from a property expression and validated by QueryPropertyPath. | ||
| command.CommandText = Queries.SelectKeyDivergesFromIdProperty(ResolvedIdPath); | ||
| #pragma warning restore CA2100 |
michaelstonis
force-pushed
the
pr4/batch-key-reads
branch
from
August 31, 2026 14:29
d4c4d49 to
5aa9800
Compare
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>
michaelstonis
changed the base branch from
pr4/batch-key-reads
to
sync/pr3-to-main
August 31, 2026 14:51
Contributor
Author
|
Conflicts resolved and all three review comments addressed. Now Retargeted to
|
Comment on lines
+1319
to
+1323
| state.filter.Build( | ||
| commandBuilder, | ||
| state.jsonSerializer, | ||
| filterParameters, | ||
| state.keyRewrite?.VerifiedFor(conn, typeof(TIn).FullName!)); |
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.
Three findings about how a row's key is decided. Each one is what makes the next sound, which is why they're one PR rather than three.
1.
AddTypeRegistration<T>()documented a feature that didn't existIts summary said "convention-based ID property detection" and its remarks said it "attempts to find an ID property based on naming conventions." It did neither —
Create<T>()setrequiresIdMapping: trueand recorded no selector. Verified:WriteObjectAsync(obj),ReadObjectAsync(obj),ObjectExistsAsync(obj),DeleteObjectAsync(obj)andGetIdFor(obj)all threwAn id mapping has not been provided, on a type whose property was literally namedId.It now finds
Id, then<TypeName>Id, case-insensitively. It requires a public getter, not justCanRead—CanReadis true for a private getter, andpublic string Id { private get; set; }is returned by aPubliclookup. That compiles into a working selector (I checked), but neither serializer emits such a property, so the id would be absent from the stored document and its JSON path would match nothing.A type with no conventional id property still registers without a mapping, exactly as before, so registering a key-less type and supplying keys at the call site keeps working. That pattern is undocumented but real, and I didn't want to break it.
2. Strict registration now rejects a divergent key
WriteObjectsAsync(objs, keySelector, …)takes a key at the call site and never consults the registration. A row written under a key the registration wouldn't produce is unreachable by every by-object overload. Probe, on a type registeredx => x.Idthen written withx => "custom-" + x.Id:WriteObjectsAsync(objs, customSelector)ReadObjectAsync(obj)null— silent missObjectExistsAsync(GetIdFor(obj))falseDeleteObjectAsync(obj)false, and the row survivesThe silent failed delete is the sharp edge. Under
requireTypeRegistrationthat now throws, naming both keys. The check wraps the selector rather than pre-scanning, so a lazy sequence is enumerated exactly once — there's a test asserting that. Delegate registrations have no property to compare against and are untouched; outside strict mode the override stays permitted.3. That guarantee makes a rewrite sound
EqualsandInfilters on the id property are now answered from the indexedKeycolumn instead of aJSON_EXTRACTscan:EqualsIn, 100 keysI rejected this rewrite earlier in the investigation, because writes could override the key. #2 is what changes that. Soundness now has two halves: the guard means no row written through this instance can diverge, and 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 by content.
Deliberately not rewritten: negations (
NotEquals/NotIn) can't use an index either way, and a null comparison value is a question about the document rather than the key, sinceKeyisNOT NULL.What I did not do
ReadObjectAsync(obj)does not fall back to searching by id property on a miss. It would only work for property-expression registrations, would turn every legitimate not-found into a partition scan, and — worst — would convertDeleteObjectAsync(obj)from key-precise to content-matching, able to delete a different row sharing the id value under another key. Strictly more dangerous than the silent no-op it fixes.Breaking changes
AddTypeRegistration<T>()on a type with a conventional id now supplies a key; every by-object call previously threw.requireTypeRegistration, a divergent key selector now throws rather than writing an unreachable row.Caveat worth weighing
The rewrite only fires under
requireTypeRegistration: true, which is not the default. That's the correct gate — it's what the guard hangs on — but it means adopting the win is a deliberate config change rather than something that just happens.Verification
294 pass / 4 skipped;
dotnet build TychoDB.sln -c Releaseclean; library warning profile byte-identical tomain.🤖 Generated with Claude Code