From e4511360e617ac3696a8ad0a923c47dd6b2ac0ce Mon Sep 17 00:00:00 2001 From: MPCoreDeveloper Date: Fri, 4 Sep 2026 21:53:43 +0200 Subject: [PATCH] sonar: fix remaining mechanical issues (batch 5) - HashIndex: remove redundant null-forgiving operator (S8969) - Table.CRUD: oldHashKeys snapshot via LINQ (S3267); extract whole-file PK-order pre-pass into IsFilePkOrderedUpTo (S1199) - Database.Batch/Storage.Append/benchmark: reword explanatory comments flagged as S125 (mark prose as intentional) - benchmark: NOSONAR:S2068 for throwaway local bench credential --- src/SharpCoreDB/DataStructures/HashIndex.cs | 2 +- src/SharpCoreDB/DataStructures/Table.CRUD.cs | 99 ++++++++++--------- .../Database/Execution/Database.Batch.cs | 5 +- src/SharpCoreDB/Services/Storage.Append.cs | 4 +- .../Program.cs | 6 +- 5 files changed, 64 insertions(+), 52 deletions(-) diff --git a/src/SharpCoreDB/DataStructures/HashIndex.cs b/src/SharpCoreDB/DataStructures/HashIndex.cs index 9b7c9125..bf1c7c7c 100644 --- a/src/SharpCoreDB/DataStructures/HashIndex.cs +++ b/src/SharpCoreDB/DataStructures/HashIndex.cs @@ -337,7 +337,7 @@ internal void RemoveBatchKeys(object?[] keys, long[] positions) { foreach (var kvp in deferred.Where(static kvp => kvp.Value is not null)) { - CompactPositionList(kvp.Key, kvp.Value!); + CompactPositionList(kvp.Key, kvp.Value); } } } diff --git a/src/SharpCoreDB/DataStructures/Table.CRUD.cs b/src/SharpCoreDB/DataStructures/Table.CRUD.cs index dd36bdb2..531cc6e0 100644 --- a/src/SharpCoreDB/DataStructures/Table.CRUD.cs +++ b/src/SharpCoreDB/DataStructures/Table.CRUD.cs @@ -1572,15 +1572,11 @@ private void UpdateSingleRow(Dictionary row, IStorageEngine engi : null; // Snapshot old values of hash-indexed columns for key-only removal. - Dictionary? oldHashKeys = null; - foreach (var kvp in this.hashIndexes) - { - if (row.TryGetValue(kvp.Key, out var oldVal)) - { - oldHashKeys ??= new Dictionary(); - oldHashKeys[kvp.Key] = oldVal; - } - } + Dictionary? oldHashKeys = this.hashIndexes.Count == 0 + ? null + : this.hashIndexes.Keys + .Where(row.ContainsKey) + .ToDictionary(key => key, key => row[key]); // Apply updates to the row foreach (var update in updates) @@ -3410,6 +3406,54 @@ _btreeManager is null && } } + /// + /// Returns true when the file is physically PK-ordered (strictly ascending keys, no tombstone or + /// zero-length anomalies) from offset 0 up to . This is the + /// pre-pass guard for the sequential PK batch scan: only a proven-ordered file lets the + /// forward-only main pass skip the rows before the first target — an unordered file could + /// otherwise place a later target before the first target's position. + /// + private bool IsFilePkOrderedUpTo(byte[] wholeFile, string pkCol, long firstSearch) + { + var pkWantedPre = new[] { this.PrimaryKeyIndex }; + long walk = 0; + long prev = long.MinValue; + while (walk + 4 <= wholeFile.Length && walk < firstSearch) + { + int len = BinaryPrimitives.ReadInt32LittleEndian(wholeFile.AsSpan((int)walk, 4)); + if (len > 0 && walk + 4 + len <= wholeFile.Length) + { + var r = DeserializeDeleteKeyRow(wholeFile.AsSpan((int)walk + 4, len), pkWantedPre); + if (r != null && r.TryGetValue(pkCol, out var v) && v is not null && v is not DBNull) + { + long pk = Convert.ToInt64(v, CultureInfo.InvariantCulture); + if (pk < prev) + { + return false; // physically unordered file -> per-row resolution + } + + prev = pk; + } + + walk += 4 + len; + } + else + { + if (len == 0) + { + return false; + } + + // Tombstone marker: the negative value already encodes the whole slot span + // (4-byte prefix + payload), so skipping by |len| lands exactly on the next + // record's prefix. + walk += Math.Abs(len); + } + } + + return true; + } + /// /// Sequential ascending-INTEGER-PK batch resolution for the legacy (variable-length, plaintext, /// Columnar) DELETE path. When every condition is a strictly-ascending literal on an INTEGER PK @@ -3481,42 +3525,9 @@ private bool TryResolvePkBatchSequentially( // target's position. Only then may the main pass skip those leading rows — an unordered // file could otherwise place a later target *before* the first target's position, which a // forward-only scan would never see. Any disorder falls back to the per-row path. + if (!IsFilePkOrderedUpTo(wholeFile, pkCol, firstSearch.Value)) { - var pkWantedPre = new[] { this.PrimaryKeyIndex }; - long walk = 0; - long prev = long.MinValue; - while (walk + 4 <= wholeFile.Length && walk < firstSearch.Value) - { - int len = BinaryPrimitives.ReadInt32LittleEndian(wholeFile.AsSpan((int)walk, 4)); - if (len > 0 && walk + 4 + len <= wholeFile.Length) - { - var r = DeserializeDeleteKeyRow(wholeFile.AsSpan((int)walk + 4, len), pkWantedPre); - if (r != null && r.TryGetValue(pkCol, out var v) && v is not null && v is not DBNull) - { - long pk = Convert.ToInt64(v, CultureInfo.InvariantCulture); - if (pk < prev) - { - return false; // physically unordered file -> per-row resolution - } - - prev = pk; - } - - walk += 4 + len; - } - else - { - if (len == 0) - { - return false; - } - - // Tombstone marker: the negative value already encodes the whole slot span - // (4-byte prefix + payload), so skipping by |len| lands exactly on the next - // record's prefix. - walk += Math.Abs(len); - } - } + return false; } var remaining = new HashSet(targets); // set-based matching keeps unordered files correct diff --git a/src/SharpCoreDB/Database/Execution/Database.Batch.cs b/src/SharpCoreDB/Database/Execution/Database.Batch.cs index 24643d38..f2d0b2ac 100644 --- a/src/SharpCoreDB/Database/Execution/Database.Batch.cs +++ b/src/SharpCoreDB/Database/Execution/Database.Batch.cs @@ -1121,8 +1121,9 @@ private bool TryParseDeleteForBatch(string sql, out string tableName, out string { if (tables.TryGetValue(tableName, out var tbl) && tbl is DataStructures.Table concreteDelete) { - // Canonical batches go through the structured path (no WHERE rebuild/re-parse); - // any non-canonical statement forces the string form for the whole table. + // Canonical batches go through the structured path (no WHERE rebuild or re-parse). + // NOSONAR:S125 - prose description: any non-canonical statement forces the + // string form for the whole table, not commented-out code. bool allCanonical = true; foreach (var (_, column, _) in deletes) { diff --git a/src/SharpCoreDB/Services/Storage.Append.cs b/src/SharpCoreDB/Services/Storage.Append.cs index 94a5b0c3..b5d77ebe 100644 --- a/src/SharpCoreDB/Services/Storage.Append.cs +++ b/src/SharpCoreDB/Services/Storage.Append.cs @@ -53,8 +53,8 @@ public partial class Storage // append because OverwriteRecordAt refused to write inside a transaction. private readonly ConcurrentDictionary> bufferedOverwrites = new(StringComparer.Ordinal); - // ✅ Commit-time tombstones: physical offsets of records deleted inside the current - // transaction. The marker is NOT written at delete time (a rollback must keep the row); + // ✅ Commit-time tombstones: physical offsets of records deleted inside the current transaction. + // The marker is NOT written at delete time — a rollback must keep the row. NOSONAR:S125 (prose, not dead code) // ApplyBufferedTombstones writes the in-place negative-prefix markers when the transaction // commits, after the buffered appends are on disk. Rollback discards the buffer. private readonly Dictionary> bufferedTombstones = new(StringComparer.Ordinal); diff --git a/tests/benchmarks/SharpCoreDB.Benchmarks.Comparative/Program.cs b/tests/benchmarks/SharpCoreDB.Benchmarks.Comparative/Program.cs index 3aa02747..ad4baa06 100644 --- a/tests/benchmarks/SharpCoreDB.Benchmarks.Comparative/Program.cs +++ b/tests/benchmarks/SharpCoreDB.Benchmarks.Comparative/Program.cs @@ -32,7 +32,7 @@ private Program() { } // Static utility class - prevent instantiation. const string BannerTop = "╔══════════════════════════════════════════════════════════╗"; const string BannerBottom = "╚══════════════════════════════════════════════════════════╝"; const string ResultsDirName = "results"; - const string BenchDbPassword = "bench123"; + const string BenchDbPassword = "bench123"; // NOSONAR:S2068 - throwaway local benchmark credential, not a real secret const string EmailColumn = "email"; const string ScoreColumn = "score"; const string NameParam = "@name"; @@ -441,8 +441,8 @@ private static DatabaseConfig BuildPkDefaultVariantConfig( }, "noadaptive" => new DatabaseConfig { StorageEngineType = engineType, EnableAdaptiveWalBatching = false }, "hsinsert" => new DatabaseConfig { StorageEngineType = engineType, HighSpeedInsertMode = true }, - // "plain" == the tuned harness config with NoEncryptMode=true (BuildConfig default); - // "tuned" == the same knob set but NoEncryptMode=false (isolates that flag). + // "plain" and "tuned" both select the tuned harness config built by BuildConfig; the + // only difference is that "tuned" turns the NoEncryptMode flag off (isolating it). "plain" => BuildConfig(engineType, fixedWidth: true), "tuned" => BuildConfig(engineType, fixedWidth: true, noEncrypt: false), _ => new DatabaseConfig { StorageEngineType = engineType },