From b726bed3167d88b01c1ab992257b09771c94a4fb Mon Sep 17 00:00:00 2001 From: Michael Stonis <120685+michaelstonis@users.noreply.github.com> Date: Sat, 29 Aug 2026 11:40:22 -0500 Subject: [PATCH 1/3] feat: add ReadObjectsByKeysAsync, and count in the engine MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- CHANGELOG.md | 35 +++ README.md | 24 ++ TychoDB.UnitTests/BatchKeyReadTests.cs | 272 +++++++++++++++++ TychoDB.UnitTests/ZzBatchKeyBench.cs | 405 +++++++++++++++++++++++++ TychoDB/Queries.cs | 37 ++- TychoDB/Tycho.cs | 129 +++++++- 6 files changed, 889 insertions(+), 13 deletions(-) create mode 100644 TychoDB.UnitTests/BatchKeyReadTests.cs create mode 100644 TychoDB.UnitTests/ZzBatchKeyBench.cs diff --git a/CHANGELOG.md b/CHANGELOG.md index c28c15b..afa4755 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -141,6 +141,15 @@ index); batch writes **−44%**; database file with three indexes **20.2 → 8.4 ### Performance +- **`CountObjectsAsync` no longer counts rows on the client.** It issued + `SELECT 1 FROM JsonValue WHERE …` and incremented a counter once per matching row, costing 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 (2.5x). The same query backs the + pre-count a progress-reporting `ReadObjectsAsync` performs, so progress-enabled reads pay + half of what they did. A *filtered* count is still bounded by whether the filtered property + is indexed — counting a 1-in-200 selective filter on an unindexed path takes ~79 ms on the + same store, essentially all of it the `JSON_EXTRACT` scan. + - **`PRAGMA optimize` on connect and disconnect.** `Connect`/`ConnectAsync` and `Disconnect`/`DisconnectAsync`/`Dispose` run SQLite's recommended `PRAGMA optimize` (bounded by `analysis_limit = 400`) so the query planner keeps fresh statistics and @@ -177,6 +186,26 @@ index); batch writes **−44%**; database file with three indexes **20.2 → 8.4 ### Added +- **`ReadObjectsByKeysAsync(keys, partition, sort, …)`.** Reads a batch of keys in one round + trip. The key set is bound as a **single JSON array** expanded by `JSON_EACH`, not as one + parameter per key, so there is no `SQLITE_MAX_VARIABLE_NUMBER` ceiling (999 on older SQLite + builds), no chunking for callers to think about, and one prepared statement regardless of + batch size. Keys lead the primary key, so each is a primary-key probe. Measured against a + loop of `ReadObjectAsync` on a 250,000-row store (best of five, after warm-up): + + | batch | looped `ReadObjectAsync` | `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 | + + That is 2.1–2.7x end to end. Both figures include deserialization, which is identical between + them and dominates what is left — the query alone is 27.5 ms at 23,784 keys. The `JSON_EACH` + shape was chosen by measurement: a single `IN (@p0…@pN)` collapses at scale (1,297.7 ms at + 23,784 keys, because the statement text and plan grow with the batch), a chunked `IN` is + 91.8 ms, and a temp-table join carries ~40 ms of fixed setup. Keys not present are simply + absent from the result. - **`FilterType.In` and `FilterType.NotIn`.** Set membership as a single atomic term, via new `Filter` overloads taking an `IEnumerable`: @@ -269,6 +298,12 @@ index); batch writes **−44%**; database file with three indexes **20.2 → 8.4 ### Notes +- **Serializer choice is the largest remaining lever on read throughput.** Reading a whole + 250,000-row partition measured 254.7 ms with `SystemTextJsonSerializer` against 358.9 ms with + `NewtonsoftJsonSerializer` (~1.4x), because the former implements `IUtf8JsonDeserializer` and + receives rows as UTF-8 spans. Deserialization dominates any large read: of the 67.3 ms + `ReadObjectsByKeysAsync` takes for 23,784 keys, only 27.5 ms is the query. + - Performance guidance: prefer `WriteObjectsAsync` for writing many objects — it is ~10× faster and ~6× lower-allocation than looping `WriteObjectAsync`, and `withTransaction: true` is faster than `false` for bulk writes. diff --git a/README.md b/README.md index 115b517..1ea8fc8 100644 --- a/README.md +++ b/README.md @@ -190,8 +190,19 @@ var exists = await db.ObjectExistsAsync("123"); // Count objects var count = await db.CountObjectsAsync(); + +// Read many objects by key in one round trip. Prefer this over a loop of ReadObjectAsync: +// keys lead the primary key, and there is no limit on how many may be passed. +var people = await db.ReadObjectsByKeysAsync(new object[] { "id-1", "id-2", "id-3" }); ``` +> **Filtering on the property that is also the Tycho key** (`x => x.Id`) goes through +> `JSON_EXTRACT` and scans — it does not use the primary key, because Tycho stores the key in +> its own `Key` column and cannot assume the property still matches it (a write may supply its +> own key selector). Reach those rows through `ReadObjectAsync` / `ReadObjectsByKeysAsync`, or +> index the property like any other. On a 250,000-row store, one equality lookup measured +> 71.6 ms as an unindexed filter and 0.0 ms all three other ways. + ### Filtering ```csharp @@ -339,6 +350,19 @@ var result = await db.DeleteBlobsAsync("documents"); Console.WriteLine($"Deleted {result.Count} blobs"); ``` +## Performance notes + +- **Reach rows by key through the key APIs.** A filter on the key property scans; see the note + under [Basic Querying](#basic-querying). `ReadObjectsByKeysAsync` fetches a whole batch in one + round trip and has no limit on batch size. +- **`SystemTextJsonSerializer` deserializes faster.** It implements `IUtf8JsonDeserializer`, so + rows are handed to it as UTF-8 spans and skip an intermediate stream. Reading a whole + 250,000-row partition measured **254.7 ms** with `SystemTextJsonSerializer` against + **358.9 ms** with `NewtonsoftJsonSerializer` — about 1.4x. Deserialization dominates any large + read, so this is usually the largest single lever on read throughput. +- **Index anything you filter or sort on.** An unindexed `JSON_EXTRACT` predicate scans the + partition; see below. + ## Indexing Create indexes to improve query performance: diff --git a/TychoDB.UnitTests/BatchKeyReadTests.cs b/TychoDB.UnitTests/BatchKeyReadTests.cs new file mode 100644 index 0000000..b71b022 --- /dev/null +++ b/TychoDB.UnitTests/BatchKeyReadTests.cs @@ -0,0 +1,272 @@ +#nullable enable + +using System; +using System.Collections.Generic; +using System.Globalization; +using System.IO; +using System.Linq; +using System.Threading.Tasks; +using Microsoft.VisualStudio.TestTools.UnitTesting; +using Shouldly; + +namespace TychoDB.UnitTests; + +/// +/// Reading a batch of keys in one round trip. The key set is bound as a JSON array rather than +/// one parameter per key, so the tests that matter most are the ones that would break that +/// encoding: keys carrying JSON metacharacters, and key sets larger than SQLite's parameter +/// ceiling. +/// +[TestClass] +public class BatchKeyReadTests +{ + private const string PartitionA = "partitionA"; + private const string PartitionB = "partitionB"; + + [TestMethod] + public async Task ReadsTheObjectsForTheGivenKeys() + { + using var db = Connect(); + await SeedAsync(db); + + var results = await db.ReadObjectsByKeysAsync(new object[] { "k1", "k3" }, PartitionA); + + results.Select(x => x.Key).OrderBy(x => x).ShouldBe(new[] { "k1", "k3" }); + } + + [TestMethod] + public async Task KeysThatAreNotPresent_AreSimplyAbsent() + { + using var db = Connect(); + await SeedAsync(db); + + var results = await db.ReadObjectsByKeysAsync(new object[] { "k1", "nope", "k2" }, PartitionA); + + results.Select(x => x.Key).OrderBy(x => x).ShouldBe(new[] { "k1", "k2" }); + } + + [TestMethod] + public async Task EmptyKeySet_ReturnsNothing() + { + using var db = Connect(); + await SeedAsync(db); + + var results = await db.ReadObjectsByKeysAsync(Array.Empty(), PartitionA); + + results.ShouldBeEmpty(); + } + + [TestMethod] + public async Task StaysWithinItsPartition() + { + using var db = Connect(); + await SeedAsync(db); + + // Both keys exist in both partitions, holding different values; only partition A's + // versions may come back. + var results = await db.ReadObjectsByKeysAsync(new object[] { "k1", "shared" }, PartitionA); + + results.Select(x => x.Description).OrderBy(x => x, StringComparer.Ordinal) + .ShouldBe(new[] { "a-shared", "a1" }); + } + + [TestMethod] + public async Task StaysWithinItsType() + { + // Same key, same partition, different stored type. + using var db = Connect(); + await SeedAsync(db); + await db.WriteObjectsAsync(new[] { new Other { Key = "k1", Note = "wrong type" } }, x => x.Key, PartitionA); + + var results = await db.ReadObjectsByKeysAsync(new object[] { "k1" }, PartitionA); + + results.Select(x => x.Description).ShouldBe(new[] { "a1" }); + } + + [TestMethod] + public async Task DuplicateKeys_YieldOneObjectEach() + { + using var db = Connect(); + await SeedAsync(db); + + var results = await db.ReadObjectsByKeysAsync(new object[] { "k1", "k1", "k1" }, PartitionA); + + results.Select(x => x.Key).ShouldBe(new[] { "k1" }); + } + + [TestMethod] + public async Task KeysCarryingJsonMetacharacters_RoundTrip() + { + // The key set is rendered as JSON, so a quote, a backslash, a control character or a + // non-BMP character in a key would corrupt the array if it were not properly encoded. + var hostile = new[] + { + "he said \"hi\"", + @"back\slash", + "tab\tand\nnewline", + "emoji \U0001F389", + "unicode é中", + "'; DROP TABLE JsonValue; --", + }; + + using var db = Connect(); + await db.WriteObjectsAsync( + hostile.Select(k => new Doc { Key = k, Description = "held" }), x => x.Key, PartitionA); + + var results = await db.ReadObjectsByKeysAsync(hostile.Cast(), PartitionA); + + results.Select(x => x.Key).OrderBy(x => x, StringComparer.Ordinal) + .ShouldBe(hostile.OrderBy(x => x, StringComparer.Ordinal)); + } + + [TestMethod] + public async Task KeySetLargerThanTheParameterCeiling_Works() + { + // One bound parameter regardless of key count, so SQLITE_MAX_VARIABLE_NUMBER — 999 on + // older builds — does not apply and no chunking is needed. + const int count = 5_000; + + using var db = Connect(); + await db.WriteObjectsAsync( + Enumerable.Range(0, count).Select(i => new Doc + { + Key = "b" + i.ToString(CultureInfo.InvariantCulture), + Description = "bulk", + }), + x => x.Key, + PartitionA); + + var keys = Enumerable.Range(0, count).Select(i => (object)("b" + i.ToString(CultureInfo.InvariantCulture))); + + var results = await db.ReadObjectsByKeysAsync(keys, PartitionA); + + results.Count().ShouldBe(count); + } + + [TestMethod] + public async Task NonStringKeys_MatchTheSingleKeyOverload() + { + using var db = Connect(); + await db.WriteObjectsAsync( + new[] { new Numbered { Id = 7, Note = "seven" }, new Numbered { Id = 8, Note = "eight" } }, + x => x.Id, + PartitionA); + + var batch = await db.ReadObjectsByKeysAsync(new object[] { 7, 8 }, PartitionA); + var single = await db.ReadObjectAsync(7, PartitionA); + + batch.Select(x => x.Note).OrderBy(x => x).ShouldBe(new[] { "eight", "seven" }); + single.Note.ShouldBe("seven"); + } + + [TestMethod] + public async Task AppliesSorting() + { + using var db = Connect(); + await SeedAsync(db); + + var results = + await db.ReadObjectsByKeysAsync( + new object[] { "k1", "k2", "k3" }, + PartitionA, + SortBuilder.Create().OrderBy(SortDirection.Descending, x => x.Description)); + + results.Select(x => x.Description).ShouldBe(new[] { "a3", "a2", "a1" }); + } + + [TestMethod] + public async Task ReportsProgress() + { + using var db = Connect(); + await SeedAsync(db); + + // A synchronous reporter, not Progress, which dispatches its callbacks + // asynchronously and would make this a race. + var progress = new RecordingProgress(); + + await db.ReadObjectsByKeysAsync(new object[] { "k1", "k2", "k3" }, PartitionA, progress: progress); + + progress.Reports.ShouldNotBeEmpty(); + progress.Reports[^1].ShouldBe(1.0); + } + + [TestMethod] + public async Task NullKeyInTheSet_IsRejected() + { + using var db = Connect(); + await SeedAsync(db); + + await Should.ThrowAsync( + async () => await db.ReadObjectsByKeysAsync(new object?[] { "k1", null }!, PartitionA)); + } + + [TestMethod] + public async Task NullKeySet_IsRejected() + { + using var db = Connect(); + await SeedAsync(db); + + await Should.ThrowAsync( + async () => await db.ReadObjectsByKeysAsync(null!, PartitionA)); + } + + private static Tycho Connect() + { + var dir = Path.GetTempPath(); + var name = $"{Guid.NewGuid()}.db"; + + var db = new Tycho(dir, new NewtonsoftJsonSerializer(), dbName: name, rebuildCache: true, requireTypeRegistration: false); + return db.Connect(); + } + + private static async Task SeedAsync(Tycho db) + { + await db.WriteObjectsAsync( + new[] + { + new Doc { Key = "k1", Description = "a1" }, + new Doc { Key = "k2", Description = "a2" }, + new Doc { Key = "k3", Description = "a3" }, + new Doc { Key = "shared", Description = "a-shared" }, + }, + x => x.Key, + PartitionA); + + await db.WriteObjectsAsync( + new[] + { + new Doc { Key = "k1", Description = "b1" }, + new Doc { Key = "shared", Description = "b-shared" }, + }, + x => x.Key, + PartitionB); + } + + private sealed class RecordingProgress : IProgress + { + public List Reports { get; } = new(); + + public void Report(double value) => Reports.Add(value); + } + + public class Doc + { + public string Key { get; set; } = string.Empty; + + public string Description { get; set; } = string.Empty; + } + + public class Other + { + public string Key { get; set; } = string.Empty; + + public string Note { get; set; } = string.Empty; + } + + public class Numbered + { + public int Id { get; set; } + + public string Note { get; set; } = string.Empty; + } +} diff --git a/TychoDB.UnitTests/ZzBatchKeyBench.cs b/TychoDB.UnitTests/ZzBatchKeyBench.cs new file mode 100644 index 0000000..132f3f3 --- /dev/null +++ b/TychoDB.UnitTests/ZzBatchKeyBench.cs @@ -0,0 +1,405 @@ +#nullable enable +#pragma warning disable CA1305, CA1307, CA1848, CA2100, SA1600, SA1601, SA1201, SA1202, SA1204, SA1516 + +using System; +using System.Collections.Generic; +using System.Diagnostics; +using System.Globalization; +using System.IO; +using System.Linq; +using System.Text; +using System.Text.Json; +using System.Threading.Tasks; +using Microsoft.Data.Sqlite; +using Microsoft.VisualStudio.TestTools.UnitTesting; + +namespace TychoDB.UnitTests; + +/// +/// Scratch harness comparing the ways a batch of keys can be fetched. Not a correctness test — +/// it prints timings. Run explicitly. +/// +[TestClass] +public class ZzBatchKeyBench +{ + private const int RowCount = 250_000; + private const string Partition = "itemMaster|v1"; + + private static readonly IJsonSerializer Serializer = new NewtonsoftJsonSerializer(); + + public class Item + { + public string Key { get; set; } = string.Empty; + + public int DepartmentId { get; set; } + + public string Description { get; set; } = string.Empty; + + public string Filler { get; set; } = string.Empty; + } + + [TestMethod] + [Ignore("Scratch performance harness: seeds 250k rows. Run it by removing this attribute.")] + public async Task Compare() + { + var dir = Path.Combine(Path.GetTempPath(), "tycho_batchkey", Guid.NewGuid().ToString()); + Directory.CreateDirectory(dir); + var name = "bench.db"; + var file = Path.Combine(dir, name); + + var sw = Stopwatch.StartNew(); + using (var db = new Tycho(dir, Serializer, dbName: name, rebuildCache: true, requireTypeRegistration: false).Connect()) + { + const int batch = 10_000; + for (var offset = 0; offset < RowCount; offset += batch) + { + var slice = + Enumerable.Range(offset, Math.Min(batch, RowCount - offset)) + .Select(i => new Item + { + Key = "K" + i.ToString(CultureInfo.InvariantCulture), + DepartmentId = i % 200, + Description = "Item number " + i.ToString(CultureInfo.InvariantCulture), + Filler = new string('x', 200), + }); + + await db.WriteObjectsAsync(slice, x => x.Key, Partition); + } + } + + SqliteConnection.ClearAllPools(); + Console.WriteLine($"seed {RowCount} rows: {sw.ElapsedMilliseconds} ms, {new FileInfo(file).Length / 1_048_576} MB"); + + var rng = new Random(20260828); + var allKeys = Enumerable.Range(0, RowCount).Select(i => "K" + i.ToString(CultureInfo.InvariantCulture)).ToArray(); + + foreach (var batchSize in new[] { 200, 1_000, 5_000, 25_000 }) + { + var keys = Enumerable.Range(0, batchSize).Select(_ => allKeys[rng.Next(RowCount)]).Distinct().ToArray(); + + Console.WriteLine($"\n=== batch of {keys.Length} keys ==="); + + // Tycho paths: end to end, including deserialization. + SqliteConnection.ClearAllPools(); + using (var db = new Tycho(dir, Serializer, dbName: name, rebuildCache: false, requireTypeRegistration: false).Connect()) + { + await Timed("looped ReadObjectAsync", async () => + { + var found = 0; + foreach (var k in keys) + { + if (await db.ReadObjectAsync(k, Partition) is not null) + { + found++; + } + } + + return found; + }); + + await Timed("ReadObjectsByKeysAsync", async () => + (await db.ReadObjectsByKeysAsync(keys, Partition)).Count()); + } + + // Raw SQL shapes: query cost only, no deserialization, so they are not comparable + // with the two above — they are here to compare the shapes against each other. + SqliteConnection.ClearAllPools(); + var conn = Open(file); + try + { + await Timed(" raw json_each(@keys)", () => Task.FromResult(RunJsonEach(conn, keys))); + await Timed(" raw single IN", () => Task.FromResult(RunIn(conn, keys, keys.Length))); + await Timed(" raw chunked IN (900)", () => Task.FromResult(RunIn(conn, keys, 900))); + await Timed(" raw temp table + join", () => Task.FromResult(RunTempTable(conn, keys))); + } + finally + { + conn.Close(); + conn.Dispose(); + SqliteConnection.ClearAllPools(); + } + } + + Directory.Delete(dir, true); + } + + [TestMethod] + [Ignore("Scratch performance harness: seeds 250k rows. Run it by removing this attribute.")] + public async Task CountShapes() + { + var dir = Path.Combine(Path.GetTempPath(), "tycho_countbench", Guid.NewGuid().ToString()); + Directory.CreateDirectory(dir); + var name = "bench.db"; + var file = Path.Combine(dir, name); + + using (var db = new Tycho(dir, Serializer, dbName: name, rebuildCache: true, requireTypeRegistration: false).Connect()) + { + const int batch = 10_000; + for (var offset = 0; offset < RowCount; offset += batch) + { + await db.WriteObjectsAsync( + Enumerable.Range(offset, Math.Min(batch, RowCount - offset)) + .Select(i => new Item + { + Key = "K" + i.ToString(CultureInfo.InvariantCulture), + DepartmentId = i % 200, + Description = "Item number " + i.ToString(CultureInfo.InvariantCulture), + Filler = new string('x', 200), + }), + x => x.Key, + Partition); + } + } + + SqliteConnection.ClearAllPools(); + + using (var db = new Tycho(dir, Serializer, dbName: name, rebuildCache: false, requireTypeRegistration: false).Connect()) + { + await Timed("CountObjectsAsync (all)", async () => await db.CountObjectsAsync(Partition)); + await Timed("CountObjectsAsync (1/200)", async () => await db.CountObjectsAsync( + Partition, FilterBuilder.Create().Filter(FilterType.Equals, x => x.DepartmentId, 7))); + } + + SqliteConnection.ClearAllPools(); + + var conn = Open(file); + try + { + await Timed(" raw SELECT 1 + client loop", () => Task.FromResult(CountVia(conn, "SELECT 1"))); + await Timed(" raw SELECT COUNT(*)", () => Task.FromResult(CountVia(conn, "SELECT COUNT(*)"))); + } + finally + { + conn.Close(); + conn.Dispose(); + SqliteConnection.ClearAllPools(); + } + + Directory.Delete(dir, true); + } + + /// + /// Two questions at once: what a filter on the key property costs versus reaching the key + /// through the primary key, and what the serializer choice costs on the deserialization + /// that dominates a large read. + /// + [TestMethod] + [Ignore("Scratch performance harness: seeds 250k rows. Run it by removing this attribute.")] + public async Task KeyFilterAndSerializerShapes() + { + foreach (var (serializer, label) in new (IJsonSerializer, string)[] + { + (new NewtonsoftJsonSerializer(), "Newtonsoft"), + (new SystemTextJsonSerializer(), "SystemTextJson"), + }) + { + var dir = Path.Combine(Path.GetTempPath(), "tycho_keybench", Guid.NewGuid().ToString()); + Directory.CreateDirectory(dir); + var name = "bench.db"; + + using (var db = new Tycho(dir, serializer, dbName: name, rebuildCache: true, requireTypeRegistration: false).Connect()) + { + const int batch = 10_000; + for (var offset = 0; offset < RowCount; offset += batch) + { + await db.WriteObjectsAsync( + Enumerable.Range(offset, Math.Min(batch, RowCount - offset)) + .Select(i => new Item + { + Key = "K" + i.ToString(CultureInfo.InvariantCulture), + DepartmentId = i % 200, + Description = "Item number " + i.ToString(CultureInfo.InvariantCulture), + Filler = new string('x', 200), + }), + x => x.Key, + Partition); + } + } + + SqliteConnection.ClearAllPools(); + Console.WriteLine($"\n=== {label} ==="); + + using (var db = new Tycho(dir, serializer, dbName: name, rebuildCache: false, requireTypeRegistration: false).Connect()) + { + // Reaching one row three ways. + await Timed("filter on key property", async () => + (await db.ReadObjectsAsync( + Partition, + FilterBuilder.Create().Filter(FilterType.Equals, x => x.Key, "K123456"))).Count()); + + await Timed("ReadObjectAsync (PK)", async () => + await db.ReadObjectAsync("K123456", Partition) is null ? 0 : 1); + + await Timed("ReadObjectsByKeysAsync (PK)", async () => + (await db.ReadObjectsByKeysAsync(new object[] { "K123456" }, Partition)).Count()); + + // The safe alternative to rewriting the filter onto the Key column: index the + // key property like any other. + await db.CreateIndexAsync(x => x.Key, "ix_key_property"); + + await Timed("filter on key property, indexed", async () => + (await db.ReadObjectsAsync( + Partition, + FilterBuilder.Create().Filter(FilterType.Equals, x => x.Key, "K123456"))).Count()); + + // Deserialization-dominated read: every row in the partition. + await Timed("read all 250k", async () => (await db.ReadObjectsAsync(Partition)).Count()); + } + + SqliteConnection.ClearAllPools(); + Directory.Delete(dir, true); + } + } + + private static int CountVia(SqliteConnection conn, string projection) + { + using var cmd = conn.CreateCommand(); + cmd.CommandText = $"{projection} FROM JsonValue WHERE FullTypeName = $t AND Partition = $p"; + cmd.Parameters.AddWithValue("$t", typeof(Item).FullName!); + cmd.Parameters.AddWithValue("$p", Partition); + + using var reader = cmd.ExecuteReader(); + if (projection == "SELECT COUNT(*)") + { + return reader.Read() ? reader.GetInt32(0) : 0; + } + + var n = 0; + while (reader.Read()) + { + n++; + } + + return n; + } + + private static SqliteConnection Open(string file) + { + var conn = new SqliteConnection($"Data Source={file}"); + conn.Open(); + using var pragma = conn.CreateCommand(); + pragma.CommandText = + "PRAGMA journal_mode = WAL; PRAGMA synchronous = NORMAL; PRAGMA temp_store = MEMORY;" + + "PRAGMA cache_size = -65536; PRAGMA mmap_size = 268435456;"; + pragma.ExecuteNonQuery(); + return conn; + } + + private static async Task Timed(string label, Func> action) + { + // Two warm-up passes (JIT, page cache, statement cache), then the best of five. Best + // rather than mean: the run without a GC pause or a scheduler hiccup is the one that + // reflects the work actually being done. + await action().ConfigureAwait(false); + await action().ConfigureAwait(false); + + var best = double.MaxValue; + var n = 0; + + for (var i = 0; i < 5; i++) + { + var sw = Stopwatch.StartNew(); + n = await action().ConfigureAwait(false); + sw.Stop(); + best = Math.Min(best, sw.Elapsed.TotalMilliseconds); + } + + Console.WriteLine($" {label,-28} {best,9:F1} ms ({n} rows)"); + } + + private static int RunIn(SqliteConnection conn, string[] keys, int chunkSize) + { + var total = 0; + + for (var start = 0; start < keys.Length; start += chunkSize) + { + var end = Math.Min(start + chunkSize, keys.Length); + + var sb = new StringBuilder(); + sb.Append("SELECT Data FROM JsonValue WHERE FullTypeName = $t AND Partition = $p AND Key IN ("); + for (var i = start; i < end; i++) + { + if (i > start) + { + sb.Append(','); + } + + sb.Append("$k").Append(i.ToString(CultureInfo.InvariantCulture)); + } + + sb.Append(')'); + + using var cmd = conn.CreateCommand(); + cmd.CommandText = sb.ToString(); + cmd.Parameters.AddWithValue("$t", typeof(Item).FullName!); + cmd.Parameters.AddWithValue("$p", Partition); + for (var i = start; i < end; i++) + { + cmd.Parameters.AddWithValue("$k" + i.ToString(CultureInfo.InvariantCulture), keys[i]); + } + + total += Drain(cmd); + } + + return total; + } + + private static int RunJsonEach(SqliteConnection conn, string[] keys) + { + using var cmd = conn.CreateCommand(); + cmd.CommandText = + "SELECT Data FROM JsonValue WHERE FullTypeName = $t AND Partition = $p" + + " AND Key IN (SELECT value FROM json_each($keys))"; + cmd.Parameters.AddWithValue("$t", typeof(Item).FullName!); + cmd.Parameters.AddWithValue("$p", Partition); + cmd.Parameters.AddWithValue("$keys", JsonSerializer.Serialize(keys)); + + return Drain(cmd); + } + + private static int RunTempTable(SqliteConnection conn, string[] keys) + { + using (var ddl = conn.CreateCommand()) + { + ddl.CommandText = "DROP TABLE IF EXISTS temp.BatchKeys; CREATE TEMP TABLE BatchKeys(Key TEXT PRIMARY KEY);"; + ddl.ExecuteNonQuery(); + } + + using (var tx = conn.BeginTransaction()) + { + using var ins = conn.CreateCommand(); + ins.Transaction = tx; + ins.CommandText = "INSERT OR IGNORE INTO temp.BatchKeys(Key) VALUES ($k)"; + var p = ins.Parameters.Add("$k", SqliteType.Text); + foreach (var k in keys) + { + p.Value = k; + ins.ExecuteNonQuery(); + } + + tx.Commit(); + } + + using var cmd = conn.CreateCommand(); + cmd.CommandText = + "SELECT j.Data FROM temp.BatchKeys b JOIN JsonValue j" + + " ON j.Key = b.Key AND j.FullTypeName = $t AND j.Partition = $p"; + cmd.Parameters.AddWithValue("$t", typeof(Item).FullName!); + cmd.Parameters.AddWithValue("$p", Partition); + + return Drain(cmd); + } + + private static int Drain(SqliteCommand cmd) + { + var n = 0; + using var reader = cmd.ExecuteReader(System.Data.CommandBehavior.SequentialAccess); + while (reader.Read()) + { + _ = reader.GetString(0); + n++; + } + + return n; + } +} diff --git a/TychoDB/Queries.cs b/TychoDB/Queries.cs index e752f9d..123072e 100644 --- a/TychoDB/Queries.cs +++ b/TychoDB/Queries.cs @@ -235,14 +235,49 @@ FROM JsonValue Partition = $partition """; + // 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. public const string SelectCountFromJsonValueWithFullTypeName = """ - SELECT 1 + SELECT COUNT(*) + FROM JsonValue + Where + FullTypeName = $fullTypeName + AND + Partition = $partition + """; + + // Batch key lookup. The key set arrives as a single JSON array bound to $keys and is + // expanded by JSON_EACH, rather than as one bound parameter per key. That keeps the + // statement text and parameter count constant no matter how many keys are asked for, + // so it is not subject to SQLITE_MAX_VARIABLE_NUMBER (999 on older builds) and does not + // force a distinct statement — and therefore a re-parse and re-plan — for every batch + // size. Key leads the PRIMARY KEY (Key, FullTypeName, Partition), so each expanded key + // is a primary-key probe. JSON1 is verified at connect, so JSON_EACH is always present. + public const string SelectDataFromJsonValueWithFullTypeNameAndKeys = + """ + SELECT rowid, Data FROM JsonValue Where FullTypeName = $fullTypeName AND Partition = $partition + AND + Key IN (SELECT value FROM JSON_EACH($keys)) + """; + + public const string SelectCountFromJsonValueWithFullTypeNameAndKeys = + """ + SELECT COUNT(*) + FROM JsonValue + Where + FullTypeName = $fullTypeName + AND + Partition = $partition + AND + Key IN (SELECT value FROM JSON_EACH($keys)) """; public const string DeleteDataFromJsonValueWithKeyAndFullTypeName = diff --git a/TychoDB/Tycho.cs b/TychoDB/Tycho.cs index c866ec9..fe432d6 100644 --- a/TychoDB/Tycho.cs +++ b/TychoDB/Tycho.cs @@ -7,6 +7,7 @@ using System.Linq; using System.Linq.Expressions; using System.Text; +using System.Text.Json; using System.Threading; using System.Threading.Tasks; using Microsoft.Data.Sqlite; @@ -21,6 +22,7 @@ private const string ParameterFullTypeName = "$fullTypeName", ParameterPartition = "$partition", ParameterKey = "$key", + ParameterKeys = "$keys", ParameterJson = "$json", ParameterBlob = "$blob", ParameterBlobLength = "$blobLength", @@ -558,12 +560,8 @@ public ValueTask CountObjectsAsync(string? partition = null, FilterBuild using var reader = selectCommand.ExecuteReader(); - int count = 0; - - while (reader.Read()) - { - ++count; - } + // One row holding the count, rather than one row per match. + int count = reader.Read() ? reader.GetInt32(0) : 0; transaction?.Commit(); @@ -848,6 +846,96 @@ public ValueTask> ReadObjectsAsync( bool withTransaction = false, IProgress? progress = null, CancellationToken cancellationToken = default) + => ReadObjectsCoreAsync(partition, filter, sort, top, withTransaction, progress, null, cancellationToken); + + /// + /// Reads the objects stored under a set of keys, in one round trip. + /// + /// Keys lead the primary key, so each is a primary-key probe; a filter on the key + /// property goes through JSON_EXTRACT instead and scans. Prefer this over a + /// loop of : + /// it takes the connection gate once rather than once per key, which matters under + /// contention, and it measured 2.1–2.7x faster end to end than the loop across batches of + /// 200 to 24,000 keys on a 250,000-row store. Both include deserialization, which is + /// identical between them and dominates the remainder; the query alone is roughly 2.5x + /// faster again. + /// + /// + /// Keys are bound as a single JSON array, so there is no limit on how many may be passed + /// and no chunking to think about. Keys that are not present are simply absent from the + /// result, so the result may be shorter than the key set, and its order is the database's, + /// not the key set's. Duplicate keys yield one object each. + /// + /// + /// The type of objects to read. + /// The keys to read. An empty set returns no objects without querying. + /// Optional partition to read from. + /// Optional sorting to apply to the result set. + /// Whether to use a transaction for the operation. + /// Optional progress reporter; see . + /// A token to cancel the asynchronous operation. + /// A ValueTask containing the objects found for those keys. + public ValueTask> ReadObjectsByKeysAsync( + IEnumerable keys, + string? partition = null, + SortBuilder? sort = null, + bool withTransaction = false, + IProgress? progress = null, + CancellationToken cancellationToken = default) + { + ArgumentNullException.ThrowIfNull(keys); + + var keysJson = BuildKeyArrayJson(keys); + + if (keysJson is null) + { + return new ValueTask>(Array.Empty()); + } + + return ReadObjectsCoreAsync(partition, null, sort, null, withTransaction, progress, keysJson, cancellationToken); + } + + /// + /// 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. + /// + private static string? BuildKeyArrayJson(IEnumerable keys) + { + using var buffer = new MemoryStream(); + using (var writer = new Utf8JsonWriter(buffer)) + { + writer.WriteStartArray(); + + var any = false; + foreach (var key in keys) + { + ArgumentNullException.ThrowIfNull(key, nameof(keys)); + + writer.WriteStringValue(key.ToString()); + any = true; + } + + if (!any) + { + return null; + } + + writer.WriteEndArray(); + } + + return Encoding.UTF8.GetString(buffer.GetBuffer(), 0, (int)buffer.Length); + } + + private ValueTask> ReadObjectsCoreAsync( + string? partition, + FilterBuilder? filter, + SortBuilder? sort, + int? top, + bool withTransaction, + IProgress? progress, + string? keysJson, + CancellationToken cancellationToken) { if (_requireTypeRegistration) { @@ -857,9 +945,9 @@ public ValueTask> ReadObjectsAsync( ArgumentNullException.ThrowIfNull(_connection); return _connection - .WithConnectionBlockAsync, (string? partition, FilterBuilder? filter, SortBuilder? sort, int? top, bool withTransaction, IProgress? progress, StringBuilder commandBuilder, int commandTimeout, IJsonSerializer jsonSerializer, CancellationToken cancellationToken)>( + .WithConnectionBlockAsync, (string? partition, FilterBuilder? filter, SortBuilder? sort, int? top, bool withTransaction, IProgress? progress, StringBuilder commandBuilder, int commandTimeout, IJsonSerializer jsonSerializer, string? keysJson, CancellationToken cancellationToken)>( _connectionGate, - (partition, filter, sort, top, withTransaction, progress, _commandBuilder, _commandTimeout, _jsonSerializer, cancellationToken), + (partition, filter, sort, top, withTransaction, progress, _commandBuilder, _commandTimeout, _jsonSerializer, keysJson, cancellationToken), static async (conn, state) => { SqliteTransaction? transaction = null; @@ -870,10 +958,14 @@ static async (conn, state) => } var commandBuilder = state.commandBuilder; - commandBuilder.Clear().Append(Queries.SelectDataFromJsonValueWithFullTypeName); + commandBuilder.Clear().Append( + state.keysJson is null + ? Queries.SelectDataFromJsonValueWithFullTypeName + : Queries.SelectDataFromJsonValueWithFullTypeNameAndKeys); // Apply filters and sorting var filterParameters = new FilterParameters(); + if (state.filter is not null) { state.filter.Build(commandBuilder, state.jsonSerializer, filterParameters); @@ -904,6 +996,11 @@ static async (conn, state) => // Use cached parameters selectCommand.Parameters.Add(new SqliteParameter(ParameterFullTypeName, SqliteType.Text) { Value = TypeCache.FullName }); selectCommand.Parameters.Add(new SqliteParameter(ParameterPartition, SqliteType.Text) { Value = state.partition.AsValueOrEmptyString() }); + if (state.keysJson is not null) + { + selectCommand.Parameters.Add(new SqliteParameter(ParameterKeys, SqliteType.Text) { Value = state.keysJson }); + } + selectCommand.AddFilterParameters(filterParameters); // Overall progress needs the result-set size up front. Count with the same @@ -914,7 +1011,10 @@ static async (conn, state) => if (state.progress is not null) { - commandBuilder.Clear().Append(Queries.SelectCountFromJsonValueWithFullTypeName); + commandBuilder.Clear().Append( + state.keysJson is null + ? Queries.SelectCountFromJsonValueWithFullTypeName + : Queries.SelectCountFromJsonValueWithFullTypeNameAndKeys); var countFilterParameters = new FilterParameters(); if (state.filter is not null) @@ -930,12 +1030,17 @@ static async (conn, state) => countCommand.CommandTimeout = state.commandTimeout; countCommand.Parameters.Add(new SqliteParameter(ParameterFullTypeName, SqliteType.Text) { Value = TypeCache.FullName }); countCommand.Parameters.Add(new SqliteParameter(ParameterPartition, SqliteType.Text) { Value = state.partition.AsValueOrEmptyString() }); + if (state.keysJson is not null) + { + countCommand.Parameters.Add(new SqliteParameter(ParameterKeys, SqliteType.Text) { Value = state.keysJson }); + } + countCommand.AddFilterParameters(countFilterParameters); await using var countReader = await countCommand.ExecuteReaderAsync(state.cancellationToken).ConfigureAwait(false); - while (countReader.Read()) + if (countReader.Read()) { - ++totalRows; + totalRows = countReader.GetInt64(0); } if (state.top is not null && totalRows > state.top.Value) From ba86453f1a1b1504a78b9a4d745e68c26733558d Mon Sep 17 00:00:00 2001 From: Michael Stonis <120685+michaelstonis@users.noreply.github.com> Date: Sat, 29 Aug 2026 11:40:52 -0500 Subject: [PATCH 2/3] feat: derive keys honestly, and use the id property that follows from it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three findings that compose; each one is what makes the next sound. AddTypeRegistration() 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 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 --- CHANGELOG.md | 63 +++- README.md | 13 +- TychoDB.UnitTests/KeyRegistrationTests.cs | 401 ++++++++++++++++++++++ TychoDB.UnitTests/ZzBatchKeyBench.cs | 62 ++++ TychoDB/FilterBuilder.cs | 79 ++++- TychoDB/KeyColumnRewrite.cs | 71 ++++ TychoDB/Queries.cs | 23 ++ TychoDB/RegisteredTypeInformation.cs | 97 +++++- TychoDB/Tycho.cs | 153 ++++++++- 9 files changed, 931 insertions(+), 31 deletions(-) create mode 100644 TychoDB.UnitTests/KeyRegistrationTests.cs create mode 100644 TychoDB/KeyColumnRewrite.cs diff --git a/CHANGELOG.md b/CHANGELOG.md index afa4755..5a3317a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -186,6 +186,40 @@ index); batch writes **−44%**; database file with three indexes **20.2 → 8.4 ### Added +- **`AddTypeRegistration()` now detects the id property by convention, as documented.** It + previously did nothing of the kind: it recorded no selector, so `WriteObjectAsync(obj)`, + `ReadObjectAsync(obj)`, `ObjectExistsAsync(obj)`, `DeleteObjectAsync(obj)` and `GetIdFor(obj)` + all threw `TychoException: An id mapping has not been provided`, on a type whose property was + literally named `Id`. The property is now found by name — `Id`, then `Id`, matched + case-insensitively, and it must be public, readable and non-indexed. When no such property + exists the type is still registered but without an id mapping, exactly as before, so + registering a key-less type and supplying keys at the call site keeps working. +- **Strict registration now rejects a key that diverges from the registered id property.** + `WriteObjectsAsync(objs, keySelector, …)` takes a key at the call site and overrides the + registration. A row written under a key the registration would not produce is unreachable by + every by-object overload, and the delete failure is silent — `DeleteObjectAsync(obj)` returns + false while the row survives. With `requireTypeRegistration: true` and a type registered by id + property, such a write now throws `TychoException` naming both keys. Delegate registrations + (`AddTypeRegistrationWithCustomKeySelector`) have no property to compare against and are + unaffected, as is everything outside strict mode. The check wraps the selector rather than + pre-scanning, so a lazy sequence is still enumerated exactly once. +- **Filters on the id property are answered from the `Key` column where that is provably + correct.** `Filter(Equals, x => x.Id, …)` and `Filter(In, x => x.Id, …)` previously went + through `JSON_EXTRACT` and scanned. Under `requireTypeRegistration` with a type registered by + id property, they are now emitted against the indexed `Key` column instead: + + | filter on the id property, 250,000 rows | scan | rewritten | + |---|---:|---:| + | `Equals` | 79.3 ms | **0.0 ms** | + | `In`, 100 keys | 101.2 ms | **0.2 ms** | + + Soundness comes from two things together: the write guard above means no row written through + this instance can diverge, and rows already in the database are checked once per type with a + divergence probe before the rewrite is used (~92 ms on that store, on the first such query + only, then 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. Negated forms (`NotEquals`, `NotIn`) are deliberately left alone — they cannot use an + index either way — as is a null comparison value, since `Key` is `NOT NULL`. - **`ReadObjectsByKeysAsync(keys, partition, sort, …)`.** Reads a batch of keys in one round trip. The key set is bound as a **single JSON array** expanded by `JSON_EACH`, not as one parameter per key, so there is no `SQLITE_MAX_VARIABLE_NUMBER` ceiling (999 on older SQLite @@ -201,11 +235,10 @@ index); batch writes **−44%**; database file with three indexes **20.2 → 8.4 |23,784 | 183.2 ms | 67.3 ms | That is 2.1–2.7x end to end. Both figures include deserialization, which is identical between - them and dominates what is left — the query alone is 27.5 ms at 23,784 keys. The `JSON_EACH` - shape was chosen by measurement: a single `IN (@p0…@pN)` collapses at scale (1,297.7 ms at - 23,784 keys, because the statement text and plan grow with the batch), a chunked `IN` is - 91.8 ms, and a temp-table join carries ~40 ms of fixed setup. Keys not present are simply - absent from the result. + them and dominates what is left — the query alone is 27.5 ms at 23,784 keys. The `JSON_EACH` shape was chosen by + measurement: a single `IN (@p0…@pN)` collapses at scale (1,297.7 ms at 23,784 keys, because + the statement text and plan grow with the batch), a chunked `IN` is 91.8 ms, and a temp-table + join carries ~40 ms of fixed setup. Keys not present are simply absent from the result. - **`FilterType.In` and `FilterType.NotIn`.** Set membership as a single atomic term, via new `Filter` overloads taking an `IEnumerable`: @@ -225,10 +258,6 @@ index); batch writes **−44%**; database file with three indexes **20.2 → 8.4 - Longer lists are split across several `IN` terms rather than exceeding `SQLITE_MAX_VARIABLE_NUMBER`, which is only 999 on older SQLite builds, so a large set works regardless of which build the host application ships. - - The raw-path overload takes `IEnumerable` rather than a generic parameter on - purpose: a generic overload there captures an ordinary `string` comparison value, since - `string` is an `IEnumerable`. A value-type collection needs `Cast()`; the - expression overload infers the element type from the property and needs no cast. - **`IJsonValueResolver`.** A second optional serializer capability, feature-detected the same way, reporting the scalar form a CLR value takes in JSON so filter comparisons are made against what was stored. Implemented by `SystemTextJsonSerializer` and @@ -243,15 +272,21 @@ index); batch writes **−44%**; database file with three indexes **20.2 → 8.4 ### Breaking changes +- **`AddTypeRegistration()` on a type with a conventional id property now supplies a key.** + Previously every by-object operation on such a type threw; they now work. Code that caught + that exception, or that relied on `WriteObjectsAsync(objs, keySelector)` disagreeing with a + conventionally-named `Id` property, changes behaviour — under `requireTypeRegistration` the + disagreement is now an error rather than a silently unreachable row. + +- **An ungrouped `Or()` now means what it reads as.** Code that (unknowingly) depended on the + leaked rows — most plausibly a query written against a single-partition, single-type database + where the bug was invisible — returns fewer rows now. This is the fix, not a regression. - **Passing a collection to a scalar `FilterType` now throws `ArgumentException`.** Adding the `IEnumerable` overloads changes overload resolution for a collection argument, which previously bound to `object` and was rendered as `ToString()` (`"System.Int32[]"`), matching nothing silently. Use `FilterType.In`. A literal `null` argument also now binds to the new overload, but keeps its old meaning — `Filter(Equals, x => x.Value, null)` is still the null comparison. -- **An ungrouped `Or()` now means what it reads as.** Code that (unknowingly) depended on the - leaked rows — most plausibly a query written against a single-partition, single-type database - where the bug was invisible — returns fewer rows now. This is the fix, not a regression. - **Enum, `DateOnly` and `TimeOnly` filter values now compare against their JSON form.** Code that worked around the enum mismatch by casting to `(int)` keeps working. Code that relied on a string-enum converter's name matching by coincidence also keeps working, and now stays @@ -303,6 +338,10 @@ index); batch writes **−44%**; database file with three indexes **20.2 → 8.4 `NewtonsoftJsonSerializer` (~1.4x), because the former implements `IUtf8JsonDeserializer` and receives rows as UTF-8 spans. Deserialization dominates any large read: of the 67.3 ms `ReadObjectsByKeysAsync` takes for 23,784 keys, only 27.5 ms is the query. +- **Outside strict mode, a filter on the id property still scans.** The `Key`-column rewrite + needs the write guard to hold, and that guard only applies under `requireTypeRegistration`. + Without it, index the id property or reach those rows through `ReadObjectsByKeysAsync`; both + measured 0.0 ms against the 71.6 ms scan. - Performance guidance: prefer `WriteObjectsAsync` for writing many objects — it is ~10× faster and ~6× lower-allocation than looping `WriteObjectAsync`, and diff --git a/README.md b/README.md index 1ea8fc8..8452fbe 100644 --- a/README.md +++ b/README.md @@ -196,12 +196,13 @@ var count = await db.CountObjectsAsync(); var people = await db.ReadObjectsByKeysAsync(new object[] { "id-1", "id-2", "id-3" }); ``` -> **Filtering on the property that is also the Tycho key** (`x => x.Id`) goes through -> `JSON_EXTRACT` and scans — it does not use the primary key, because Tycho stores the key in -> its own `Key` column and cannot assume the property still matches it (a write may supply its -> own key selector). Reach those rows through `ReadObjectAsync` / `ReadObjectsByKeysAsync`, or -> index the property like any other. On a 250,000-row store, one equality lookup measured -> 71.6 ms as an unindexed filter and 0.0 ms all three other ways. +> **Filtering on the property that is also the Tycho key** (`x => x.Id`) normally goes through +> `JSON_EXTRACT` and scans, because a write may supply its own key selector and Tycho cannot +> assume the property still matches the stored key. Under `requireTypeRegistration: true`, with +> the type registered by id property, that assumption *is* enforced, and `Equals` / `In` filters +> on the id property are answered from the indexed `Key` column instead — 79.3 ms to 0.0 ms on a +> 250,000-row store. Otherwise, reach those rows through `ReadObjectAsync` / +> `ReadObjectsByKeysAsync`, or index the property like any other. ### Filtering diff --git a/TychoDB.UnitTests/KeyRegistrationTests.cs b/TychoDB.UnitTests/KeyRegistrationTests.cs new file mode 100644 index 0000000..b1590e1 --- /dev/null +++ b/TychoDB.UnitTests/KeyRegistrationTests.cs @@ -0,0 +1,401 @@ +#nullable enable + +using System; +using System.Collections.Generic; +using System.Globalization; +using System.IO; +using System.Linq; +using System.Text; +using System.Text.Json; +using System.Threading.Tasks; +using Microsoft.Data.Sqlite; +using Microsoft.VisualStudio.TestTools.UnitTesting; +using Shouldly; + +namespace TychoDB.UnitTests; + +/// +/// How a row's key is decided, and what follows from it: convention-based registration, the +/// strict-mode guard that stops a write from producing a key the by-object overloads cannot +/// reach, and the key-column rewrite those two together make sound. +/// +[TestClass] +public class KeyRegistrationTests +{ + private const string Partition = "p"; + + // ---------- convention-based registration ---------- + [TestMethod] + public async Task ConventionRegistration_FindsAnIdProperty() + { + using var db = Connect(t => t.AddTypeRegistration()); + + var doc = new Doc { Id = "id-1", Value = "v" }; + await db.WriteObjectAsync(doc, Partition); + + (await db.ReadObjectAsync(doc, Partition)).Value.ShouldBe("v"); + db.GetIdFor(doc).ShouldBe("id-1"); + } + + [TestMethod] + public async Task ConventionRegistration_FindsTypeNameIdProperty() + { + using var db = Connect(t => t.AddTypeRegistration()); + + var widget = new Widget { WidgetId = 7, Value = "w" }; + await db.WriteObjectAsync(widget, Partition); + + (await db.ReadObjectAsync(widget, Partition)).Value.ShouldBe("w"); + } + + [TestMethod] + public async Task ConventionRegistration_PrefersIdOverTypeNameId() + { + using var db = Connect(t => t.AddTypeRegistration()); + + db.GetIdFor(new Both { Id = "plain", BothId = "prefixed" }).ShouldBe("plain"); + } + + [TestMethod] + public async Task ConventionRegistration_WithNoIdProperty_StillAllowsExplicitKeys() + { + // The pre-existing behaviour has to survive: registering a key-less type is how a caller + // satisfies requireTypeRegistration while supplying keys at the call site. + using var db = Connect(t => t.AddTypeRegistration()); + + await db.WriteObjectsAsync(new[] { new Keyless { Value = "v" } }, x => "explicit", Partition); + + (await db.ReadObjectAsync("explicit", Partition)).Value.ShouldBe("v"); + Should.Throw(() => db.GetIdFor(new Keyless())).Message.ShouldContain("id mapping"); + } + + [TestMethod] + public void ConventionRegistration_IgnoresAPropertyWithoutAPublicGetter() + { + // A public setter is enough for the property to show up in a Public lookup, and the + // expression selector would even compile against a private getter — but neither + // serializer writes such a property, so its JSON path would match nothing. + using var db = Connect(t => t.AddTypeRegistration()); + + Should.Throw(() => db.GetIdFor(new PrivateGetterId())).Message.ShouldContain("id mapping"); + } + + [TestMethod] + public async Task ConventionRegistration_IgnoresAnIndexer() + { + // An indexer cannot become a property path; the type must fall back to explicit keys. + using var db = Connect(t => t.AddTypeRegistration()); + + Should.Throw(() => db.GetIdFor(new Indexed())).Message.ShouldContain("id mapping"); + await Task.CompletedTask; + } + + // ---------- strict-mode divergence guard ---------- + [TestMethod] + public async Task StrictMode_RejectsAKeySelectorThatDisagreesWithTheRegistration() + { + using var db = Connect(t => t.AddTypeRegistration(x => x.Id), strict: true); + + var ex = await Should.ThrowAsync( + async () => await db.WriteObjectsAsync( + new[] { new Doc { Id = "id-1", Value = "v" } }, x => "custom-" + x.Id, Partition)); + + // The guard fires while the sequence is being enumerated inside the write, so it + // arrives wrapped in the write path's usual TychoException, as every write failure does. + var guard = ex.InnerException.ShouldBeOfType(); + guard.Message.ShouldContain("custom-id-1"); + guard.Message.ShouldContain("registered id property"); + + // Nothing was written under either key. + (await db.ReadObjectsAsync(Partition)).ShouldBeEmpty(); + } + + [TestMethod] + public async Task StrictMode_AllowsAKeySelectorThatAgrees() + { + using var db = Connect(t => t.AddTypeRegistration(x => x.Id), strict: true); + + await db.WriteObjectsAsync(new[] { new Doc { Id = "id-1", Value = "v" } }, x => x.Id, Partition); + + (await db.ReadObjectAsync(new Doc { Id = "id-1" }, Partition)).Value.ShouldBe("v"); + } + + [TestMethod] + public async Task StrictMode_WithADelegateRegistration_DoesNotGuard() + { + // A delegate registration has no id property to compare against, so the override stays + // available exactly as before. + using var db = Connect(t => t.AddTypeRegistrationWithCustomKeySelector(x => x.Id), strict: true); + + await db.WriteObjectsAsync(new[] { new Doc { Id = "id-1", Value = "v" } }, x => "anything", Partition); + + (await db.ReadObjectAsync("anything", Partition)).Value.ShouldBe("v"); + } + + [TestMethod] + public async Task OutsideStrictMode_TheOverrideIsStillPermitted() + { + using var db = Connect(t => t.AddTypeRegistration(x => x.Id), strict: false); + + await db.WriteObjectsAsync(new[] { new Doc { Id = "id-1", Value = "v" } }, x => "custom-" + x.Id, Partition); + + (await db.ReadObjectAsync("custom-id-1", Partition)).Value.ShouldBe("v"); + } + + [TestMethod] + public async Task Guard_EnumeratesTheSequenceOnlyOnce() + { + // The guard wraps the selector rather than pre-scanning, because callers pass lazy + // sequences that must not be enumerated twice. + using var db = Connect(t => t.AddTypeRegistration(x => x.Id), strict: true); + + var enumerations = 0; + + IEnumerable Lazy() + { + enumerations++; + yield return new Doc { Id = "id-1", Value = "v" }; + } + + await db.WriteObjectsAsync(Lazy(), x => x.Id, Partition); + + enumerations.ShouldBe(1); + } + + // ---------- key-column rewrite ---------- + [TestMethod] + public async Task KeyPropertyFilter_UsesTheKeyColumn_InStrictMode() + { + var (db, path) = ConnectAt(t => t.AddTypeRegistration(x => x.Id), strict: true); + using var scoped = db; + await SeedAsync(db); + + var results = + await db.ReadObjectsAsync( + Partition, FilterBuilder.Create().Filter(FilterType.Equals, x => x.Id, "id-2")); + + results.Select(x => x.Value).ShouldBe(new[] { "v2" }); + (await PlanFor(db, path)).ShouldNotContain("SCAN JsonValue", Case.Sensitive); + } + + [TestMethod] + public async Task KeyPropertyFilter_WithIn_UsesTheKeyColumn() + { + var (db, path) = ConnectAt(t => t.AddTypeRegistration(x => x.Id), strict: true); + using var scoped = db; + await SeedAsync(db); + + var results = + await db.ReadObjectsAsync( + Partition, + FilterBuilder.Create().Filter(FilterType.In, x => x.Id, new[] { "id-1", "id-3" })); + + results.Select(x => x.Value).OrderBy(x => x, StringComparer.Ordinal).ShouldBe(new[] { "v1", "v3" }); + } + + [TestMethod] + public async Task KeyPropertyFilter_OutsideStrictMode_IsNotRewritten() + { + // Without the write guard the invariant is unenforced, so the ordinary predicate stands. + using var db = Connect(t => t.AddTypeRegistration(x => x.Id), strict: false); + await SeedAsync(db); + + var results = + await db.ReadObjectsAsync( + Partition, FilterBuilder.Create().Filter(FilterType.Equals, x => x.Id, "id-2")); + + results.Select(x => x.Value).ShouldBe(new[] { "v2" }); + } + + [TestMethod] + public async Task KeyPropertyFilter_WithLegacyDivergentRows_FallsBackAndStaysCorrect() + { + // Rows written before the guard existed can violate the invariant. The probe must catch + // that and fall back to the JSON predicate rather than answer from the Key column. + var dir = Path.GetTempPath(); + var name = $"{Guid.NewGuid()}.db"; + + using (var loose = new Tycho(dir, new NewtonsoftJsonSerializer(), dbName: name, rebuildCache: true, requireTypeRegistration: false) + .AddTypeRegistration(x => x.Id) + .Connect()) + { + await loose.WriteObjectsAsync(new[] { new Doc { Id = "id-1", Value = "v1" } }, x => x.Id, Partition); + + // Divergent: stored under a key its id property would never produce. + await loose.WriteObjectsAsync(new[] { new Doc { Id = "id-2", Value = "v2" } }, x => "other", Partition); + } + + SqliteConnection.ClearAllPools(); + + using var strict = new Tycho(dir, new NewtonsoftJsonSerializer(), dbName: name, rebuildCache: false, requireTypeRegistration: true) + .AddTypeRegistration(x => x.Id) + .Connect(); + + var results = + await strict.ReadObjectsAsync( + Partition, FilterBuilder.Create().Filter(FilterType.Equals, x => x.Id, "id-2")); + + // Found by content, which the Key column could not have done. + results.Select(x => x.Value).ShouldBe(new[] { "v2" }); + } + + [TestMethod] + public async Task KeyPropertyFilter_CountAndDeleteAgreeWithTheRead() + { + using var db = Connect(t => t.AddTypeRegistration(x => x.Id), strict: true); + await SeedAsync(db); + + var filter = FilterBuilder.Create().Filter(FilterType.In, x => x.Id, new[] { "id-1", "id-3" }); + + (await db.CountObjectsAsync(Partition, filter)).ShouldBe(2); + (await db.DeleteObjectsAsync(Partition, filter)).ShouldBe(2); + (await db.ReadObjectsAsync(Partition)).Select(x => x.Value).ShouldBe(new[] { "v2" }); + } + + [TestMethod] + public async Task KeyPropertyFilter_UnderACamelCasePolicy_StillMatches() + { + // The registered id path and the filter path must both be resolved through the + // serializer, or they would not compare equal and the rewrite would silently not apply. + var serializer = new SystemTextJsonSerializer( + new JsonSerializerOptions { PropertyNamingPolicy = JsonNamingPolicy.CamelCase }); + + using var db = Connect(t => t.AddTypeRegistration(x => x.Id), strict: true, serializer: serializer); + await SeedAsync(db); + + var results = + await db.ReadObjectsAsync( + Partition, FilterBuilder.Create().Filter(FilterType.Equals, x => x.Id, "id-2")); + + results.Select(x => x.Value).ShouldBe(new[] { "v2" }); + } + + [TestMethod] + public async Task NonKeyPropertyFilter_IsNeverRewritten() + { + using var db = Connect(t => t.AddTypeRegistration(x => x.Id), strict: true); + await SeedAsync(db); + + var results = + await db.ReadObjectsAsync( + Partition, FilterBuilder.Create().Filter(FilterType.Equals, x => x.Value, "v2")); + + results.Select(x => x.Id).ShouldBe(new[] { "id-2" }); + } + + [TestMethod] + public async Task NegatedKeyPropertyFilter_StaysOnTheJsonPath() + { + using var db = Connect(t => t.AddTypeRegistration(x => x.Id), strict: true); + await SeedAsync(db); + + var results = + await db.ReadObjectsAsync( + Partition, FilterBuilder.Create().Filter(FilterType.NotEquals, x => x.Id, "id-2")); + + results.Select(x => x.Id).OrderBy(x => x, StringComparer.Ordinal).ShouldBe(new[] { "id-1", "id-3" }); + } + + private static async Task PlanFor(Tycho db, string path) + { + await Task.CompletedTask; + db.Dispose(); + SqliteConnection.ClearAllPools(); + + var sb = new StringBuilder(Queries.SelectDataFromJsonValueWithFullTypeName); + var parameters = new FilterParameters(); + var filter = FilterBuilder.Create().Filter(FilterType.Equals, x => x.Id, "id-2"); + filter.Build(sb, new NewtonsoftJsonSerializer(), parameters, new KeyColumnRewrite("$.Id")); + + using var conn = new SqliteConnection($"Data Source={path}"); + conn.Open(); + using var command = conn.CreateCommand(); +#pragma warning disable CA2100 + command.CommandText = "EXPLAIN QUERY PLAN " + sb; +#pragma warning restore CA2100 + command.Parameters.AddWithValue("$fullTypeName", typeof(Doc).FullName); + command.Parameters.AddWithValue("$partition", Partition); + for (var i = 0; i < parameters.Count; i++) + { + command.Parameters.AddWithValue( + FilterParameters.ParameterPrefix + i.ToString(CultureInfo.InvariantCulture), + parameters.Values[i] ?? (object)DBNull.Value); + } + + var plan = new StringBuilder(); + using var reader = command.ExecuteReader(); + while (reader.Read()) + { + plan.AppendLine(reader.GetString(reader.FieldCount - 1)); + } + + return plan.ToString(); + } + + private static async Task SeedAsync(Tycho db) + { + await db.WriteObjectsAsync( + new[] + { + new Doc { Id = "id-1", Value = "v1" }, + new Doc { Id = "id-2", Value = "v2" }, + new Doc { Id = "id-3", Value = "v3" }, + }, + x => x.Id, + Partition); + } + + private static Tycho Connect(Func register, bool strict = false, IJsonSerializer? serializer = null) + => ConnectAt(register, strict, serializer).Db; + + private static (Tycho Db, string Path) ConnectAt(Func register, bool strict = false, IJsonSerializer? serializer = null) + { + var dir = Path.GetTempPath(); + var name = $"{Guid.NewGuid()}.db"; + + var db = new Tycho( + dir, serializer ?? new NewtonsoftJsonSerializer(), dbName: name, rebuildCache: true, requireTypeRegistration: strict); + + return (register(db).Connect(), Path.Combine(dir, name)); + } + + public class Doc + { + public string Id { get; set; } = string.Empty; + + public string Value { get; set; } = string.Empty; + } + + public class Widget + { + public int WidgetId { get; set; } + + public string Value { get; set; } = string.Empty; + } + + public class Both + { + public string Id { get; set; } = string.Empty; + + public string BothId { get; set; } = string.Empty; + } + + public class Keyless + { + public string Value { get; set; } = string.Empty; + } + + public class PrivateGetterId + { + public string Id { private get; set; } = "unreachable"; + + public string Value { get; set; } = string.Empty; + } + + public class Indexed + { + public string this[int i] => i.ToString(CultureInfo.InvariantCulture); + + public string Value { get; set; } = string.Empty; + } +} diff --git a/TychoDB.UnitTests/ZzBatchKeyBench.cs b/TychoDB.UnitTests/ZzBatchKeyBench.cs index 132f3f3..1b03ebc 100644 --- a/TychoDB.UnitTests/ZzBatchKeyBench.cs +++ b/TychoDB.UnitTests/ZzBatchKeyBench.cs @@ -251,6 +251,68 @@ await Timed("filter on key property, indexed", async () => } } + [TestMethod] + [Ignore("Scratch performance harness: seeds 250k rows. Run it by removing this attribute.")] + public async Task KeyColumnRewrite() + { + var dir = Path.Combine(Path.GetTempPath(), "tycho_rewrite", Guid.NewGuid().ToString()); + Directory.CreateDirectory(dir); + var name = "bench.db"; + + using (var db = new Tycho(dir, Serializer, dbName: name, rebuildCache: true, requireTypeRegistration: false).Connect()) + { + const int batch = 10_000; + for (var offset = 0; offset < RowCount; offset += batch) + { + await db.WriteObjectsAsync( + Enumerable.Range(offset, Math.Min(batch, RowCount - offset)) + .Select(i => new Item + { + Key = "K" + i.ToString(CultureInfo.InvariantCulture), + DepartmentId = i % 200, + Description = "Item number " + i.ToString(CultureInfo.InvariantCulture), + Filler = new string('x', 200), + }), + x => x.Key, + Partition); + } + } + + SqliteConnection.ClearAllPools(); + + foreach (var strict in new[] { false, true }) + { + using var db = new Tycho(dir, Serializer, dbName: name, rebuildCache: false, requireTypeRegistration: strict) + .AddTypeRegistration(x => x.Key) + .Connect(); + + Console.WriteLine($"\n=== requireTypeRegistration: {strict} ==="); + + // First call pays the one-time divergence probe (a scan of this type's rows). + var sw = Stopwatch.StartNew(); + _ = (await db.ReadObjectsAsync( + Partition, FilterBuilder.Create().Filter(FilterType.Equals, x => x.Key, "K123456"))).Count(); + sw.Stop(); + Console.WriteLine($" {"first call (incl. probe)",-28} {sw.Elapsed.TotalMilliseconds,9:F1} ms (1 rows)"); + + await Timed("steady state", async () => + (await db.ReadObjectsAsync( + Partition, FilterBuilder.Create().Filter(FilterType.Equals, x => x.Key, "K123456"))).Count()); + + await Timed("In (100 keys)", async () => + (await db.ReadObjectsAsync( + Partition, + FilterBuilder.Create().Filter( + FilterType.In, + x => x.Key, + Enumerable.Range(0, 100).Select(i => "K" + i.ToString(CultureInfo.InvariantCulture))))).Count()); + + SqliteConnection.ClearAllPools(); + } + + Directory.Delete(dir, true); + } + private static int CountVia(SqliteConnection conn, string projection) { using var cmd = conn.CreateCommand(); diff --git a/TychoDB/FilterBuilder.cs b/TychoDB/FilterBuilder.cs index 20c5a54..8698f66 100644 --- a/TychoDB/FilterBuilder.cs +++ b/TychoDB/FilterBuilder.cs @@ -38,6 +38,7 @@ public class FilterBuilder private const string NotInOperator = " NOT IN ("; private const string ValueSeparator = ", "; private const string OrJoin = " OR "; + private const string KeyColumn = "Key"; private const string AndJoin = " AND "; // IN () is a syntax error in SQLite, so an empty set has to be rendered as a @@ -302,7 +303,7 @@ public FilterBuilder EndGroup() return this; } - internal void Build(StringBuilder commandBuilder, IJsonSerializer jsonSerializer, FilterParameters parameters) + internal void Build(StringBuilder commandBuilder, IJsonSerializer jsonSerializer, FilterParameters parameters, KeyColumnRewrite? keyRewrite = null) { if (_filters.Count == 0) { @@ -359,6 +360,11 @@ internal void Build(StringBuilder commandBuilder, IJsonSerializer jsonSerializer } else if (filter.FilterType.HasValue) { + if (keyRewrite is not null && TryBuildKeyColumnFilter(commandBuilder, filter, parameters, keyRewrite)) + { + continue; + } + BuildSimpleFilter(commandBuilder, filter, jsonSerializer, parameters); } } @@ -720,6 +726,77 @@ private void BuildExistsFilter(StringBuilder commandBuilder, in Filter filter, I } } + /// + /// Emits an equality or set-membership test against the Key column when the filter + /// targets the type's id property, and reports whether it did. + /// + /// Only and are rewritten. Their + /// negations would still scan — a negated predicate cannot use the index either way — so + /// rewriting them would trade a clear predicate for no gain. A null comparison value is left + /// alone too: Key is NOT NULL, so "the id is null" is a question about the + /// document, not about the key. + /// + /// + /// Values bind as text because that is the form the key was stored in — both the write path + /// and the by-key reads put the key through ToString(). + /// + /// + private static bool TryBuildKeyColumnFilter(StringBuilder commandBuilder, in Filter filter, FilterParameters parameters, KeyColumnRewrite keyRewrite) + { + if (!string.Equals(filter.PropertyPath, keyRewrite.ResolvedIdPath, StringComparison.Ordinal) + || !string.IsNullOrEmpty(filter.PropertyValuePath)) + { + return false; + } + + switch (filter.FilterType!.Value) + { + case FilterType.Equals when filter.Value is not null: + commandBuilder.Append(KeyColumn).Append(Equals) + .Append(parameters.Add(filter.Value.ToString())) + .AppendLine(); + return true; + + case FilterType.In when filter.Value is object?[] values && Array.IndexOf(values, null) < 0: + if (values.Length == 0) + { + commandBuilder.AppendLine(MatchNothing); + return true; + } + + for (var chunk = 0; chunk * MaxValuesPerInClause < values.Length; chunk++) + { + if (chunk > 0) + { + commandBuilder.Append(OrJoin); + } + + var start = chunk * MaxValuesPerInClause; + var end = Math.Min(start + MaxValuesPerInClause, values.Length); + + 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); + } + + commandBuilder.AppendLine(); + return true; + + default: + return false; + } + } + private void BuildSimpleFilter(StringBuilder commandBuilder, in Filter filter, IJsonSerializer jsonSerializer, FilterParameters parameters) { switch (filter.FilterType!.Value) diff --git a/TychoDB/KeyColumnRewrite.cs b/TychoDB/KeyColumnRewrite.cs new file mode 100644 index 0000000..a9e9f30 --- /dev/null +++ b/TychoDB/KeyColumnRewrite.cs @@ -0,0 +1,71 @@ +using System; +using System.Collections.Concurrent; +using Microsoft.Data.Sqlite; + +namespace TychoDB; + +/// +/// Lets an equality or set-membership filter on a type's id property be answered from the +/// indexed Key column instead of a JSON_EXTRACT scan of every row. +/// +/// +/// +/// The rewrite is only correct while every row of the type is keyed by that id property, which +/// Tycho does not guarantee in general: WriteObjectsAsync(objs, keySelector, …) takes a +/// key at the call site and may disagree with the registration. Two things together make it +/// safe here. Strict registration rejects a divergent write outright, so no row written through +/// this instance can break the invariant; and rows already in the database — written by an +/// earlier version, or outside strict mode — are checked once with +/// before the rewrite is used for that +/// type. A single divergent row disables the rewrite for the type, and the ordinary predicate +/// is emitted instead, so the worst case is the performance that was there before. +/// +/// +/// The probe is a scan, so it is run lazily — only when a query that could benefit actually +/// arrives — and its verdict is cached for the lifetime of the connection. +/// +/// +internal sealed class KeyColumnRewrite +{ + private readonly ConcurrentDictionary _usableByTypeName = new(StringComparer.Ordinal); + + public KeyColumnRewrite(string resolvedIdPath) + { + ResolvedIdPath = resolvedIdPath; + } + + /// + /// Gets the id property's JSON path, already rendered through the serializer's member names + /// so it can be compared directly against a filter's resolved path. + /// + public string ResolvedIdPath { get; } + + /// + /// Returns this rewrite when the stored keys for provably + /// match the id property, and when they do not — in which case the + /// caller emits the ordinary JSON path predicate. + /// + public KeyColumnRewrite? VerifiedFor(SqliteConnection connection, string fullTypeName) + { + var usable = + _usableByTypeName.GetOrAdd( + fullTypeName, + static (name, state) => !state.Self.HasDivergentRow(state.Connection, name), + (Self: this, Connection: connection)); + + return usable ? this : null; + } + + private bool HasDivergentRow(SqliteConnection connection, string fullTypeName) + { + 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 + command.Parameters.Add("$fullTypeName", SqliteType.Text).Value = fullTypeName; + + using var reader = command.ExecuteReader(); + return reader.Read(); + } +} diff --git a/TychoDB/Queries.cs b/TychoDB/Queries.cs index 123072e..d3a7c79 100644 --- a/TychoDB/Queries.cs +++ b/TychoDB/Queries.cs @@ -219,6 +219,29 @@ FROM StreamValue LIMIT 1 """; + // Divergence probe for the key-column rewrite. Answers "is every row of this type keyed by + // its id property?" — the invariant that lets a filter on the id property be answered from + // the indexed Key column instead of a JSON_EXTRACT scan. A row whose id member is missing + // counts as divergent: its key cannot have come from a value that is not there. Returns at + // most one row, and only rows written before the strict-mode write guard existed (or + // outside strict mode) can produce it. + public static string SelectKeyDivergesFromIdProperty(string resolvedIdPath) + { + return string.Concat( + """ + SELECT 1 + FROM JsonValue + Where + FullTypeName = $fullTypeName + AND + (JSON_EXTRACT(Data, ' + """.TrimEnd(), + resolvedIdPath, + "') IS NULL OR Key <> CAST(JSON_EXTRACT(Data, '", + resolvedIdPath, + "') AS TEXT))\nLIMIT 1"); + } + public const string SelectPartitions = """ SELECT DISTINCT Partition diff --git a/TychoDB/RegisteredTypeInformation.cs b/TychoDB/RegisteredTypeInformation.cs index dcc7904..b8e0c97 100644 --- a/TychoDB/RegisteredTypeInformation.cs +++ b/TychoDB/RegisteredTypeInformation.cs @@ -1,6 +1,7 @@ using System; using System.Collections.Generic; using System.Linq.Expressions; +using System.Reflection; namespace TychoDB; @@ -54,6 +55,13 @@ public RegisteredTypeInformation(bool requiresIdMapping, string? typeFullName, s public string? IdPropertyPath { get; private set; } + /// + /// Gets the id property captured as unresolved segments, so it can be rendered against the + /// serializer's JSON member names at query time exactly as a filter path is. Null unless the + /// type was registered by property expression. + /// + internal PropertyPathSegment[]? IdPropertyPathSegments { get; private set; } + public bool IsNumeric { get; private set; } public bool IsBool { get; private set; } @@ -130,7 +138,10 @@ x1 is TId id1 && x2 is TId id2 && new RegisteredTypeInformation(requiresIdMapping: false, idSelector: compiledExpression!, idComparer: idComparerFunc!, idProperty: idProperty.GetExpressionMemberName(), idPropertyPath: QueryPropertyPath.BuildPath(idProperty), isNumeric: QueryPropertyPath.IsNumeric(idProperty), isBool: QueryPropertyPath.IsBool(idProperty), typeFullName: type.FullName, - typeName: type.Name, safeTypeName: type.GetSafeTypeName(), typeNamespace: type.Namespace, objectType: type!); + typeName: type.Name, safeTypeName: type.GetSafeTypeName(), typeNamespace: type.Namespace, objectType: type!) + { + IdPropertyPathSegments = QueryPropertyPath.BuildSegments(idProperty), + }; return rti; } @@ -154,12 +165,96 @@ x1 is string id1 && x2 is string id2 && typeName: type.Name, safeTypeName: type.GetSafeTypeName(), typeNamespace: type.Namespace, objectType: type!); } + /// + /// Registers , detecting its id property by convention. + /// + /// The property is looked for by name, in order: Id, then <TypeName>Id + /// (for example PersonId on Person), each matched case-insensitively. It must + /// be a public, readable, non-indexed instance property. + /// + /// + /// When no such property exists the type is still registered — satisfying + /// requireTypeRegistration — but without an id mapping, so it can only be reached by + /// an explicitly supplied key, exactly as before. That keeps registering a key-less type + /// and writing it with a call-site key selector working. + /// + /// + /// The type to register. + /// The registration information for . public static RegisteredTypeInformation Create() { var type = typeof(T); + var idProperty = FindConventionalIdProperty(type); + + if (idProperty is not null) + { + return Create(BuildPropertySelector(idProperty)); + } + return new RegisteredTypeInformation(requiresIdMapping: true, typeFullName: type.FullName, typeName: type.Name, safeTypeName: type.GetSafeTypeName(), typeNamespace: type.Namespace, objectType: type!); } + + /// + /// Finds the property a conventional id would live on: Id, else + /// <TypeName>Id. Returns null when neither exists. + /// + private static PropertyInfo? FindConventionalIdProperty(Type type) + { + var properties = + type.GetProperties(BindingFlags.Public | BindingFlags.Instance); + + // "Id" wins over "Id" when a type declares both, so the more specific name + // never quietly shadows the obvious one. + foreach (var candidate in new[] { "Id", type.Name + "Id" }) + { + foreach (var property in properties) + { + if (!property.Name.Equals(candidate, StringComparison.OrdinalIgnoreCase)) + { + continue; + } + + // An id has to be readable and addressable as a plain member: an indexer or a + // write-only property cannot be turned into a property path. + // + // CanRead is not enough — it is true for a private getter, and a property with a + // public setter and a private getter is still returned by a Public lookup. Such + // a property would compile into a working selector, but neither serializer emits + // it, so the id would be absent from the stored document: the JSON path would + // match nothing and every row would look divergent to the key-column rewrite's + // probe. Requiring a public getter keeps the convention aligned with what is + // actually written to the document. + if (property.GetIndexParameters().Length > 0 + || property.GetMethod is not { IsPublic: true }) + { + continue; + } + + return property; + } + } + + return null; + } + + /// + /// Builds the x => x.Prop expression the property-based registration path expects, + /// boxing value types through a Convert node exactly as a hand-written + /// Expression<Func<T, object>> would. + /// + private static Expression> BuildPropertySelector(PropertyInfo property) + { + var parameter = Expression.Parameter(typeof(T), "x"); + Expression body = Expression.Property(parameter, property); + + if (property.PropertyType.IsValueType) + { + body = Expression.Convert(body, typeof(object)); + } + + return Expression.Lambda>(body, parameter); + } } diff --git a/TychoDB/Tycho.cs b/TychoDB/Tycho.cs index fe432d6..d012f02 100644 --- a/TychoDB/Tycho.cs +++ b/TychoDB/Tycho.cs @@ -47,6 +47,9 @@ private const string private readonly IJsonSerializer _jsonSerializer; private readonly bool _persistConnection; private readonly bool _requireTypeRegistration; + + // One rewrite per type, so its divergence verdict is probed once and reused. + private readonly ConcurrentDictionary _keyColumnRewrites = new(); private readonly int _commandTimeout; private readonly Dictionary _registeredTypeInformation = new(); @@ -332,6 +335,20 @@ public ValueTask WriteObjectAsync(T obj, string? partition = null, bool /// Whether to use a transaction for the operation. /// A token to cancel the asynchronous operation. /// A ValueTask containing a boolean indicating success or failure. + /// + /// The key this selector returns is the key the row is stored under, and it overrides any + /// key the type's registration would supply. If the two disagree, the by-object overloads + /// keep using the registered key and stop finding the row: + /// + /// returns null and DeleteObjectAsync(obj) returns false without deleting anything, + /// while the row is still there under the key this selector produced. + /// + /// Under requireTypeRegistration, a type registered by id property will not let that + /// happen: a selector that disagrees with the registration throws + /// rather than writing a row the by-object overloads cannot reach. Outside strict mode the + /// override is permitted and unchecked. + /// + /// public ValueTask WriteObjectAsync(T obj, Func keySelector, string? partition = null, bool withTransaction = true, CancellationToken cancellationToken = default) { @@ -363,6 +380,20 @@ public ValueTask WriteObjectsAsync(IEnumerable objs, string? partiti /// Whether to use a transaction for the operation. /// A token to cancel the asynchronous operation. /// A ValueTask containing a boolean indicating success or failure. + /// + /// The key this selector returns is the key the row is stored under, and it overrides any + /// key the type's registration would supply. If the two disagree, the by-object overloads + /// keep using the registered key and stop finding the row: + /// + /// returns null and DeleteObjectAsync(obj) returns false without deleting anything, + /// while the row is still there under the key this selector produced. + /// + /// Under requireTypeRegistration, a type registered by id property will not let that + /// happen: a selector that disagrees with the registration throws + /// rather than writing a row the by-object overloads cannot reach. Outside strict mode the + /// override is permitted and unchecked. + /// + /// public ValueTask WriteObjectsAsync(IEnumerable objs, Func keySelector, string? partition = null, bool withTransaction = true, CancellationToken cancellationToken = default) { @@ -370,6 +401,8 @@ public ValueTask WriteObjectsAsync(IEnumerable objs, Func ArgumentNullException.ThrowIfNull(keySelector); ArgumentNullException.ThrowIfNull(_connection); + keySelector = GuardAgainstKeyDivergence(keySelector); + return _connection .WithConnectionBlockAsync( _connectionGate, @@ -527,7 +560,7 @@ public ValueTask CountObjectsAsync(string? partition = null, FilterBuild return _connection .WithConnectionBlockAsync( _connectionGate, - (partition, filter, withTransaction, commandBuilder: _commandBuilder, _jsonSerializer), + (partition, filter, withTransaction, commandBuilder: _commandBuilder, _jsonSerializer, keyRewrite: GetKeyColumnRewriteFor()), static (conn, state) => { SqliteTransaction? transaction = null; @@ -550,7 +583,11 @@ public ValueTask CountObjectsAsync(string? partition = null, FilterBuild var filterParameters = new FilterParameters(); if (state.filter is not null) { - state.filter.Build(state.commandBuilder, state._jsonSerializer, filterParameters); + state.filter.Build( + state.commandBuilder, + state._jsonSerializer, + filterParameters, + state.keyRewrite?.VerifiedFor(conn, TypeCache.FullName)); } #pragma warning disable CA2100 // Comparison values are parameterized (AddFilterParameters); only validated JSON paths/identifiers are concatenated. @@ -945,9 +982,9 @@ private ValueTask> ReadObjectsCoreAsync( ArgumentNullException.ThrowIfNull(_connection); return _connection - .WithConnectionBlockAsync, (string? partition, FilterBuilder? filter, SortBuilder? sort, int? top, bool withTransaction, IProgress? progress, StringBuilder commandBuilder, int commandTimeout, IJsonSerializer jsonSerializer, string? keysJson, CancellationToken cancellationToken)>( + .WithConnectionBlockAsync, (string? partition, FilterBuilder? filter, SortBuilder? sort, int? top, bool withTransaction, IProgress? progress, StringBuilder commandBuilder, int commandTimeout, IJsonSerializer jsonSerializer, string? keysJson, KeyColumnRewrite? keyRewrite, CancellationToken cancellationToken)>( _connectionGate, - (partition, filter, sort, top, withTransaction, progress, _commandBuilder, _commandTimeout, _jsonSerializer, keysJson, cancellationToken), + (partition, filter, sort, top, withTransaction, progress, _commandBuilder, _commandTimeout, _jsonSerializer, keysJson, GetKeyColumnRewriteFor(), cancellationToken), static async (conn, state) => { SqliteTransaction? transaction = null; @@ -966,9 +1003,13 @@ state.keysJson is null // Apply filters and sorting var filterParameters = new FilterParameters(); + // Resolved once and reused by the progress pre-count below, so the count and + // the rows it is measuring can never be built from different predicates. + var keyRewrite = state.keyRewrite?.VerifiedFor(conn, TypeCache.FullName); + if (state.filter is not null) { - state.filter.Build(commandBuilder, state.jsonSerializer, filterParameters); + state.filter.Build(commandBuilder, state.jsonSerializer, filterParameters, keyRewrite); } if (state.sort is not null) @@ -1019,7 +1060,7 @@ state.keysJson is null var countFilterParameters = new FilterParameters(); if (state.filter is not null) { - state.filter.Build(commandBuilder, state.jsonSerializer, countFilterParameters); + state.filter.Build(commandBuilder, state.jsonSerializer, countFilterParameters, keyRewrite); } using var countCommand = conn.CreateCommand(); @@ -1228,9 +1269,9 @@ await ReadObjectsWithKeysAsync(innerObjectSelection, partition, filter, withTran string selectionPath = QueryPropertyPath.BuildPath(innerObjectSelection, NameResolver); return _connection - .WithConnectionBlockAsync, (string selectionPath, string? partition, FilterBuilder? filter, bool withTransaction, StringBuilder commandBuilder, IJsonSerializer jsonSerializer, CancellationToken cancellationToken)>( + .WithConnectionBlockAsync, (string selectionPath, string? partition, FilterBuilder? filter, bool withTransaction, StringBuilder commandBuilder, IJsonSerializer jsonSerializer, KeyColumnRewrite? keyRewrite, CancellationToken cancellationToken)>( _connectionGate, - (selectionPath, partition, filter, withTransaction, _commandBuilder, _jsonSerializer, cancellationToken), + (selectionPath, partition, filter, withTransaction, _commandBuilder, _jsonSerializer, keyRewrite: GetKeyColumnRewriteFor(), cancellationToken), static async (conn, state) => { SqliteTransaction? transaction = null; @@ -1257,7 +1298,11 @@ static async (conn, state) => var filterParameters = new FilterParameters(); if (state.filter is not null) { - state.filter.Build(commandBuilder, state.jsonSerializer, filterParameters); + state.filter.Build( + commandBuilder, + state.jsonSerializer, + filterParameters, + state.keyRewrite?.VerifiedFor(conn, typeof(TIn).FullName!)); } #pragma warning disable CA2100 // Comparison values are parameterized (AddFilterParameters); only validated JSON paths/identifiers are concatenated. @@ -1412,7 +1457,7 @@ public ValueTask DeleteObjectsAsync(string? partition = null, FilterBuil return _connection .WithConnectionBlockAsync( _connectionGate, - (partition, filter, withTransaction, commandBuilder: _commandBuilder, _jsonSerializer), + (partition, filter, withTransaction, commandBuilder: _commandBuilder, _jsonSerializer, keyRewrite: GetKeyColumnRewriteFor()), static (conn, state) => { SqliteTransaction? transaction = null; @@ -1435,7 +1480,11 @@ public ValueTask DeleteObjectsAsync(string? partition = null, FilterBuil var filterParameters = new FilterParameters(); if (state.filter is not null) { - state.filter.Build(state.commandBuilder, state._jsonSerializer, filterParameters); + state.filter.Build( + state.commandBuilder, + state._jsonSerializer, + filterParameters, + state.keyRewrite?.VerifiedFor(conn, TypeCache.FullName)); } #pragma warning disable CA2100 // Comparison values are parameterized (AddFilterParameters); only validated JSON paths/identifiers are concatenated. @@ -2503,6 +2552,88 @@ public Func GetIdSelectorFor() return rti.GetIdSelector(); } + /// + /// Builds the key-column rewrite candidate for , or null when the + /// preconditions do not hold: strict registration must be on (so the write guard forbids a + /// divergent key) and the type must have been registered by id property (so there is a path + /// to compare a filter against). The candidate still has to clear its divergence probe + /// against the data before it is used. + /// + private KeyColumnRewrite? GetKeyColumnRewriteFor() + { + if (!_requireTypeRegistration + || !_registeredTypeInformation.TryGetValue(typeof(T), out var rti) + || rti is null + || rti.RequiresIdMapping + || rti.IdPropertyPathSegments is null) + { + return null; + } + + return _keyColumnRewrites.GetOrAdd( + typeof(T), + static (_, state) => + new KeyColumnRewrite( + QueryPropertyPath.RenderPath( + state.Segments, + QueryPropertyPath.AsNameResolver(state.Serializer))), + (Segments: rti.IdPropertyPathSegments, Serializer: _jsonSerializer)); + } + + /// + /// In strict mode, wraps a caller-supplied key selector so that a key disagreeing with the + /// type's registered id property is rejected instead of written. + /// + /// A row stored under a key the registration would not produce is unreachable by + /// ReadObjectAsync(obj), ObjectExistsAsync(obj) and DeleteObjectAsync(obj), + /// all of which key off the registration — and the delete failure is silent, returning false + /// while the row survives. Turning that into an exception at the write is the only point + /// where the disagreement is still visible. + /// + /// + /// The check is a wrapper rather than a pre-pass so the object sequence is enumerated once: + /// callers routinely pass a lazy query. It applies only when strict registration is on and + /// the type was registered by id property — a delegate registration has no property to + /// compare against, and outside strict mode the override is deliberate and permitted. + /// + /// + private Func GuardAgainstKeyDivergence(Func keySelector) + { + if (!_requireTypeRegistration + || !_registeredTypeInformation.TryGetValue(typeof(T), out var rti) + || rti is null + || rti.RequiresIdMapping + || rti.IdPropertyPath is null) + { + return keySelector; + } + + var registeredSelector = rti.GetIdSelector(); + var idProperty = rti.IdProperty; + + return obj => + { + var supplied = keySelector(obj); + + // Compared as text because that is what the Key column stores: both sides go + // through ToString() on their way into the database. + var suppliedKey = supplied?.ToString(); + var registeredKey = registeredSelector(obj)?.ToString(); + + if (!string.Equals(suppliedKey, registeredKey, StringComparison.Ordinal)) + { + throw new TychoException( + $"The supplied key selector produced \"{suppliedKey}\" for {typeof(T).Name}, but its registered id property {idProperty} gives \"{registeredKey}\". " + + $"A row written under \"{suppliedKey}\" could not be read or deleted by object, because those overloads use the registered id. " + + "Supply the registered key, register the type with a custom key selector instead, or turn off requireTypeRegistration."); + } + + // Func promises a non-null key; the null-conditional above is defensive + // against a selector that breaks that contract, not an admission that it may. + return supplied!; + }; + } + /// /// Gets the ID value for an object instance. /// From 8f3305e3330333b4bf9607708ec36e010201d65b Mon Sep 17 00:00:00 2001 From: Michael Stonis <120685+michaelstonis@users.noreply.github.com> Date: Mon, 31 Aug 2026 09:51:25 -0500 Subject: [PATCH 3/3] fix: address Copilot review on the key-column rewrite MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- TychoDB.UnitTests/KeyRegistrationTests.cs | 64 ++++++++++++++++++++++- TychoDB/FilterBuilder.cs | 21 +++++++- TychoDB/KeyColumnRewrite.cs | 12 ++++- TychoDB/Tycho.cs | 23 +++++++- 4 files changed, 114 insertions(+), 6 deletions(-) diff --git a/TychoDB.UnitTests/KeyRegistrationTests.cs b/TychoDB.UnitTests/KeyRegistrationTests.cs index b1590e1..dfcd774 100644 --- a/TychoDB.UnitTests/KeyRegistrationTests.cs +++ b/TychoDB.UnitTests/KeyRegistrationTests.cs @@ -193,6 +193,68 @@ await db.ReadObjectsAsync( results.Select(x => x.Value).OrderBy(x => x, StringComparer.Ordinal).ShouldBe(new[] { "v1", "v3" }); } + [TestMethod] + public async Task KeyPropertyFilter_WithMoreValuesThanOneChunk_StaysBoundToItsOwnTerm() + { + // A set larger than one chunk emits several "Key IN (...)" terms joined by OR. AND binds + // tighter than OR, so if that chain is not wrapped, a following AND term captures only + // the last chunk and every id in the earlier chunks comes back regardless of it — the + // same precedence bug the enclosing parentheses exist to prevent, one level in. + using var db = Connect(t => t.AddTypeRegistration(x => x.Id), strict: true); + + const int count = 1_500; + await db.WriteObjectsAsync( + Enumerable.Range(0, count).Select(i => new Doc + { + Id = "id-" + i.ToString("D5", CultureInfo.InvariantCulture), + Value = i % 2 == 0 ? "keep" : "drop", + }), + x => x.Id, + Partition); + + var everyId = + Enumerable.Range(0, count).Select(i => "id-" + i.ToString("D5", CultureInfo.InvariantCulture)); + + var results = + await db.ReadObjectsAsync( + Partition, + FilterBuilder.Create() + .Filter(FilterType.In, x => x.Id, everyId).And() + .Filter(FilterType.Equals, x => x.Value, "keep")); + + results.Count().ShouldBe(count / 2); + results.ShouldAllBe(x => x.Value == "keep"); + } + + [TestMethod] + public async Task Reregistration_DoesNotLeaveAStaleRewritePathCached() + { + // The rewrite caches the resolved id path per type. Re-registering with a different id + // property must not leave filters pointing at the old one. + var dir = Path.GetTempPath(); + var name = $"{Guid.NewGuid()}.db"; + + var tycho = new Tycho(dir, new NewtonsoftJsonSerializer(), dbName: name, rebuildCache: true, requireTypeRegistration: true) + .AddTypeRegistration(x => x.Id); + + using var db = tycho.Connect(); + await SeedAsync(db); + + // Warm the cache against $.Id. + await db.ReadObjectsAsync( + Partition, FilterBuilder.Create().Filter(FilterType.Equals, x => x.Id, "id-2")); + + // Re-register against a different property; rows are still keyed by Id. + db.AddTypeRegistration(x => x.Value); + + // A filter on Value must not be answered from the Key column, which holds Ids. + var results = + await db.ReadObjectsAsync( + Partition, FilterBuilder.Create().Filter(FilterType.Equals, x => x.Value, "v2")); + + results.Select(x => x.Id).ShouldBe(new[] { "id-2" }); + } + [TestMethod] public async Task KeyPropertyFilter_OutsideStrictMode_IsNotRewritten() { @@ -305,7 +367,7 @@ private static async Task PlanFor(Tycho db, string path) var sb = new StringBuilder(Queries.SelectDataFromJsonValueWithFullTypeName); var parameters = new FilterParameters(); var filter = FilterBuilder.Create().Filter(FilterType.Equals, x => x.Id, "id-2"); - filter.Build(sb, new NewtonsoftJsonSerializer(), parameters, new KeyColumnRewrite("$.Id")); + filter.Build(sb, new NewtonsoftJsonSerializer(), parameters, new KeyColumnRewrite("$.Id", 30)); using var conn = new SqliteConnection($"Data Source={path}"); conn.Open(); diff --git a/TychoDB/FilterBuilder.cs b/TychoDB/FilterBuilder.cs index 490c2c5..0387360 100644 --- a/TychoDB/FilterBuilder.cs +++ b/TychoDB/FilterBuilder.cs @@ -761,7 +761,21 @@ private static bool TryBuildKeyColumnFilter(StringBuilder commandBuilder, in Fil return true; } - for (var chunk = 0; chunk * MaxValuesPerInClause < values.Length; chunk++) + // A chunked chain is several IN terms joined by OR, and AND binds tighter than + // OR: left unwrapped, a following AND term would 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)". That is the same precedence bug the enclosing parentheses in Build + // exist to prevent, one level further in, so this chain is bound together too. + // BuildSetFilter does the same for the JSON-path form. + var chunks = ((values.Length - 1) / MaxValuesPerInClause) + 1; + var wrap = chunks > 1; + + if (wrap) + { + commandBuilder.Append(OpenParen); + } + + for (var chunk = 0; chunk < chunks; chunk++) { if (chunk > 0) { @@ -786,6 +800,11 @@ private static bool TryBuildKeyColumnFilter(StringBuilder commandBuilder, in Fil commandBuilder.Append(CloseParen); } + if (wrap) + { + commandBuilder.Append(CloseParen); + } + commandBuilder.AppendLine(); return true; diff --git a/TychoDB/KeyColumnRewrite.cs b/TychoDB/KeyColumnRewrite.cs index a9e9f30..b71e61b 100644 --- a/TychoDB/KeyColumnRewrite.cs +++ b/TychoDB/KeyColumnRewrite.cs @@ -22,16 +22,20 @@ namespace TychoDB; /// /// /// The probe is a scan, so it is run lazily — only when a query that could benefit actually -/// arrives — and its verdict is cached for the lifetime of the connection. +/// arrives — it runs under the connection's configured command timeout, and its verdict is +/// cached for the lifetime of the connection. /// /// internal sealed class KeyColumnRewrite { private readonly ConcurrentDictionary _usableByTypeName = new(StringComparer.Ordinal); - public KeyColumnRewrite(string resolvedIdPath) + private readonly int _commandTimeout; + + public KeyColumnRewrite(string resolvedIdPath, int commandTimeout) { ResolvedIdPath = resolvedIdPath; + _commandTimeout = commandTimeout; } /// @@ -63,6 +67,10 @@ private bool HasDivergentRow(SqliteConnection connection, string fullTypeName) #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 + + // The probe is a scan, which makes it the command most likely to need the caller's + // raised timeout rather than the provider default. + command.CommandTimeout = _commandTimeout; command.Parameters.Add("$fullTypeName", SqliteType.Text).Value = fullTypeName; using var reader = command.ExecuteReader(); diff --git a/TychoDB/Tycho.cs b/TychoDB/Tycho.cs index 3020e7f..f8f2fe8 100644 --- a/TychoDB/Tycho.cs +++ b/TychoDB/Tycho.cs @@ -173,6 +173,12 @@ public Tycho AddTypeRegistration( _registeredTypeInformation[rti.ObjectType] = rti; + // The rewrite caches the id path resolved from the previous registration. Re-registering + // a type can change that path — or remove the id property altogether — so the cached + // entry has to go, or filters would be rewritten against a path the stored keys no + // longer come from. + _keyColumnRewrites.TryRemove(rti.ObjectType, out _); + return this; } @@ -189,6 +195,12 @@ public Tycho AddTypeRegistration() _registeredTypeInformation[rti.ObjectType] = rti; + // The rewrite caches the id path resolved from the previous registration. Re-registering + // a type can change that path — or remove the id property altogether — so the cached + // entry has to go, or filters would be rewritten against a path the stored keys no + // longer come from. + _keyColumnRewrites.TryRemove(rti.ObjectType, out _); + return this; } @@ -208,6 +220,12 @@ public Tycho AddTypeRegistrationWithCustomKeySelector( _registeredTypeInformation[rti.ObjectType] = rti; + // The rewrite caches the id path resolved from the previous registration. Re-registering + // a type can change that path — or remove the id property altogether — so the cached + // entry has to go, or filters would be rewritten against a path the stored keys no + // longer come from. + _keyColumnRewrites.TryRemove(rti.ObjectType, out _); + return this; } @@ -2576,8 +2594,9 @@ public Func GetIdSelectorFor() new KeyColumnRewrite( QueryPropertyPath.RenderPath( state.Segments, - QueryPropertyPath.AsNameResolver(state.Serializer))), - (Segments: rti.IdPropertyPathSegments, Serializer: _jsonSerializer)); + QueryPropertyPath.AsNameResolver(state.Serializer)), + state.CommandTimeout), + (Segments: rti.IdPropertyPathSegments, Serializer: _jsonSerializer, CommandTimeout: _commandTimeout)); } ///