Repository navigation
refactor(sql): one prepared-VALUES splitter, one literal skipper, and no silent VALUE truncation - #480
Merged
Conversation
…t VALUE truncation Sonar failed the 2.1.2 CI gate on new-code duplication (over the 3 % ceiling): the prepared-insert path carried five copies of two algorithms. Fixed the cause rather than suppressing the gate. One shared algorithm per concern: - PreparedInsertStatement.ParseValues and ParseValuesToArray held identical splitting loops. Both now drive one `ValueRangeEnumerator` ref struct that yields [start,end) bounds — allocation-free, same paren/bracket/quote handling, same bounds guards. - The two canonical-shape scanners (IsInsertStatement, TryReadLiteralUntilWhere) re-implemented the quote machine. They now call SqlLiteralScanner through a shared TrySkipLiteral helper. - PreparedStatement.ExtractParameterPositions was a byte-for-byte copy of ParameterBinder.ExtractPositionalParameterPositions; the copy is gone. Defect exposed by the dedup, and fixed: the batch/prepared path stopped counting a tuple at the table's column count, so `INSERT INTO docs VALUES (1,'a',0.5,21)` against a three-column table discarded the 4th value and succeeded, while the statement path has rejected that shape since 2.1.2. Surplus values now throw with the table, tuple and both counts; a short tuple still fills NULLs. Tests: new PreparedInsertValuesParsingTests (11 cases) parses every case through both prepared parsers and asserts they agree — neither had coverage before. Three test fixtures passed 4 values to 3-column tables and only passed because of the truncation (FixedWidthBulkUpdateTests DDLs realigned with their own 4-column fixture; FormatCompatPolicyTests and DefaultEngineSelectionTests also rendered `i * 0.5` as "0,5" on this nl-NL machine, i.e. two values — now formatted invariant, as EncryptionCoverageTests already does). Verified on this tree: core suite 2118 / 0 failed, build clean (net11.0), and the §7 reproduction in docs/sharpcoredb-known-issues.md still prints the documented fixed output for all five bound-value shapes.
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.



Why
Sonar flagged new-code duplication in the
2.1.2diff above the 3 % ceiling. The duplicated blocks were real: the prepared-insert path carried five copies of two algorithms, and the copies had already drifted apart in behaviour.What changed — one algorithm per concern
SqlLiteralScannerDatabase.Batch.cs, behind a newTrySkipLiteralhelperValueRangeEnumerator(ref struct, yields[start,end))ParseValues/ParseValuesToArray?placeholder positionsParameterBinder.ExtractPositionalParameterPositionsPreparedStatementThe enumerator is a
ref structon purpose: the prepared-insert path must not allocate per row, so the dedup costs no allocation. It keeps the old semantics exactly — single-quote literals skipped as a whole throughSqlLiteralScanner,()and[](VECTOR literals) suppressed, top-level comma splits, same bounds guards.Defect the dedup exposed, and fixed
The prepared/batch path stopped counting a tuple at the table's column count:
It bound the first three values, discarded
21and succeeded. The statement path has rejected that shape since[2.1.2], so the two disagreed. Surplus values now throw (InvalidOperationException; the message names the table, the tuple and both counts), and a short tuple keeps its documented meaning (missing columns fall toNULL). This was the last copy of the known-issue-7 truncation class.Tests
PreparedInsertValuesParsingTests(11 cases): every case is parsed through both prepared parsers and asserted to agree — neither had any coverage before, which is how they could drift. Covers quoted commas, doubled quotes, backslashes, VECTOR literals,NULL, short tuples and the surplus-tuple rejection.FixedWidthBulkUpdateTests(five DDLs realigned to the 4-columndocsshape its own helper inserts into),FormatCompatPolicyTestsandDefaultEngineSelectionTests. The latter two also renderedi * 0.5as0,5on thisnl-NLmachine — two SQL values — and now format invariant, asEncryptionCoverageTestsandWritePathProfilerTestsalready do.Verification
Category!=Debug&Category!=Manual&Category!=Performance), build clean onnet11.0.ReopenRoundTripMatrixTests,FormatCompatPolicyTests,SingleFileDirectoryParityTests,FixedWidthBulkUpdate/Delete/PatchTests,WritePathProfilerTests,FixedWidthInlineValueTests.Reopen_KeepsInlineAndOverflowValues.docs/sharpcoredb-known-issues.mdstill prints the documented fixed output for all five bound-value shapes (re-run live against this tree).CHANGELOG.mdgainsFixedandChangedentries under[Unreleased]— the strictness is user-visible.No API change, no format change, no durability or encryption behaviour touched.