Skip to content

feat: add ReadObjectsByKeysAsync, and count in the engine - #26

Merged
michaelstonis merged 2 commits into
pr3/filter-in-notinfrom
pr4/batch-key-reads
Aug 31, 2026
Merged

feat: add ReadObjectsByKeysAsync, and count in the engine#26
michaelstonis merged 2 commits into
pr3/filter-in-notinfrom
pr4/batch-key-reads

Conversation

@michaelstonis

Copy link
Copy Markdown
Contributor

Stacked on #25. Review that one first; this diff is against it.

ReadObjectsByKeysAsync<T>

The bug report asked for batch key reads but measured it as not worth doing — a raw Key IN (5,000 params) came out at 107.9 ms against 90.3 ms for a loop of ReadObjectAsync. That conclusion was right for the shape they measured. IN (@p0…@pN) doesn't scale, because its statement text and plan grow with the batch.

Binding the key set as a single JSON array expanded by JSON_EACH avoids that entirely: one parameter, one prepared statement, no SQLITE_MAX_VARIABLE_NUMBER ceiling, no chunking for callers to think about. Keys lead the PRIMARY KEY, so each expanded key is a primary-key probe.

Query cost alone, 250,000-row store, best of five after warm-up:

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

End to end against the loop (both include deserialization, which is identical between them and 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

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 actually break: keys carrying quotes, backslashes, control characters, non-BMP emoji and a SQL-injection string, plus a 5,000-key set that would blow the 999-parameter ceiling.

CountObjectsAsync stopped counting rows on the client

Found while investigating the above. It issued SELECT 1 FROM JsonValue WHERE … and incremented a counter once per matching row — a reader round trip per row. Now SELECT COUNT(*) with a single scalar read: 16.0 ms → 6.5 ms counting a 250,000-row partition. The same query backs the pre-count a progress-reporting ReadObjectsAsync performs, so those pay half of what they did.

A filtered count is unchanged and still bounded by indexing — a 1-in-200 filter on an unindexed path takes ~79 ms on that store, essentially all JSON_EXTRACT scan.

Benchmark harness

ZzBatchKeyBench.cs is committed [Ignore]d so it never runs in CI. Remove the attribute to reproduce any number above. Happy to drop it from the branch if you'd rather it stayed local — the repo's Zz files have historically been untracked scratch.

Verification

275 pass / 3 skipped; dotnet build TychoDB.sln -c Release clean.

All measurements are against synthetic 250k-row stores seeded locally, not the reporter's real 291 MB item master. The shapes should hold; the absolute numbers are from my machine.

🤖 Generated with Claude Code

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds a batch key-read API and updates counting to be performed in SQLite rather than client-side iteration, improving performance for large reads and progress-enabled operations.

Changes:

  • Added ReadObjectsByKeysAsync<T> that binds a key set as a single JSON array and expands it via JSON_EACH for efficient primary-key probes.
  • Switched CountObjectsAsync / progress pre-count queries to SELECT COUNT(*) to avoid per-row reader round trips.
  • Added unit tests for batch key reads (including hostile keys and >999 key sets), plus an ignored benchmark harness; updated README/CHANGELOG with usage and performance notes.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
TychoDB/Tycho.cs Adds ReadObjectsByKeysAsync<T> and threads an optional JSON key-set parameter through the shared read core; updates counting logic.
TychoDB/Queries.cs Introduces new key-batch SQL shapes using JSON_EACH and updates count query to COUNT(*).
TychoDB.UnitTests/ZzBatchKeyBench.cs Adds an ignored benchmark harness for reproducing performance measurements.
TychoDB.UnitTests/BatchKeyReadTests.cs Adds correctness tests for batch key reads, including JSON-escaping and large key sets.
README.md Documents new batch key-read API and guidance on key-property filtering vs key APIs.
CHANGELOG.md Records performance and feature additions for the new API and count optimization.

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

Comment thread TychoDB/Tycho.cs Outdated
Copilot AI review requested due to automatic review settings August 31, 2026 14:25
michaelstonis and others added 2 commits August 31, 2026 09:29
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>
@michaelstonis
michaelstonis merged commit 764a001 into pr3/filter-in-notin Aug 31, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

TychoDB/Tycho.cs:902

  • The XML doc for BuildKeyArrayJson claims it uses “the same ToString() form the single-key overloads bind”, but the single-key APIs bind the key object directly via SqliteParameter (e.g., ReadObjectAsync/WriteObjectsAsync set ParameterKey.Value = key). This makes the doc misleading and obscures that batch-key reads are doing their own string conversion path.
    /// <summary>
    /// Renders the key set as a JSON array of strings for JSON_EACH to expand, using the same
    /// ToString() form the single-key overloads bind. Returns null for an empty set, which has
    /// no query to run.
    /// </summary>

TychoDB/Tycho.cs:998

  • This block allocates a new SqliteCommand and new SqliteParameter instances on every call, but the comment says “Use cached parameters”. That’s misleading and makes it harder to reason about whether parameters need to be conditionally added (e.g., $keys). Either actually cache/reuse parameters or rename the comment to reflect what the code does.
                    try
                    {
                        // Use cached parameters
                        selectCommand.Parameters.Add(new SqliteParameter(ParameterFullTypeName, SqliteType.Text) { Value = TypeCache<T>.FullName });
                        selectCommand.Parameters.Add(new SqliteParameter(ParameterPartition, SqliteType.Text) { Value = state.partition.AsValueOrEmptyString() });

Copilot AI review requested due to automatic review settings August 31, 2026 14:31

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

Comment thread TychoDB/Queries.cs
Comment on lines +238 to +241
// COUNT(*), not "SELECT 1" counted row by row on the client: the engine counts without
// materializing a result row per match, and the reader makes one round trip instead of one
// per matching row. Measured 2.3x faster counting a 250,000-row partition (5.0 ms vs
// 11.6 ms). This also halves the pre-count a progress-reporting read performs.
Comment thread TychoDB/Tycho.cs
Comment on lines +898 to +902
/// <summary>
/// Renders the key set as a JSON array of strings for JSON_EACH to expand, using the same
/// ToString() form the single-key overloads bind. Returns null for an empty set, which has
/// no query to run.
/// </summary>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants