feat: compile one writer per parameter or column, and read a row's max cells from a source

[arthurschreiber]

Builds on #1777 and #1781. Targets master.

Problem

After #1781 every type writes a value through writeValue, and TVP and bulk load rows write cell by cell through validate followed by writeValue, with the column's declaration facts re-read per cell and every cell going through call sites that see a different type at each column. That is where the time goes now. And a max value can be read from a source as a parameter (#1777), but not as a cell of a TVP row or a bulk load row.

Change

compileWriter(facts, options) is a type's single write path. It returns (buffer, value) => void | AsyncIterable<void>: the writer validates one value through the type's validate and writes it (length prefix and data), with the fact-dependent choices (max or not, the collation, the scale, useUTC) made once when the writer is built. writeValue leaves the interface, and with it the second copy of every type's write logic. Nothing is inlined that validate already does. Callers:

• RpcRequestPayload writes each parameter through type.compileWriter(data, options)(buffer, data.value). resolve validated the value already; the writer validates it again, which every type's validate tolerates (VarChar, Char and Text take the buffer validate encoded as well as a string).
• tvp.ts compiles one writer per column per TVP and writes each row in a synchronous loop that returns as soon as a cell hands back a rest; finishRow drives that rest through writeRest and writes the cells after it, so a row without such a cell costs one call. BulkLoadPayload does the same per column.
• The generic compileWriter adapter in data-type.ts goes: every type compiles its own.

Streamed cells. A cell of a varbinary(max), nvarchar(max) or varchar(max) column in a TVP row or a bulk load row may now be an async iterable of chunks, read while the row is written, under the rules Request.addParameter already documents for such a parameter value. A column that is not max rejects a source the way it rejects any other non-value.

Behaviour

• Bytes are unchanged for every existing input. The per-type unit tests exercise the compiled writer against the same expected bytes as before.
• New: a max cell in a TVP row or a bulk load row may be an async iterable. Documented on addParameter and in the bulk load row-source section.
• New, small: the writers of VarChar, Char and Text accept an already encoded buffer for a cell, the way validate produces one. NChar, NVarChar and VarBinary no longer have unreachable branches for values validate rejects, and their unit tests feed valid values.
• A source read in several chunks is written as several PLP chunks, valid but not byte-identical to the single-chunk in-memory form, as for a parameter value already.

Validation

• test/unit/compiled-writers-test.ts: error parity with validate, a source rejected for a non-max column, the rest of a write returned for a max column, a TVP row and a bulk load row with streamed cells serializing exactly as with the cells in memory, chunks handed on while a cell is still being read, a failing cell source surfacing as the column's InputError for a TVP and failing the bulk load while closing its row source.
• test/integration/streaming-parameters-test.ts and test/integration/bulk-load-test.ts: a 100 KB varbinary(max) cell and a 70 KB nvarchar(max) cell, streamed in uneven chunks including empty ones across packet and flush boundaries, round-trip unchanged through a TVP procedure and through a bulk load, next to in-memory and null cells.
• Existing suites: unit 543 passing; parameterised statements, rpc, tvp, bulk load, streaming parameters, binary insert, prepare/execute and datatypes-in-results integration suites pass against SQL Server 2022 (331). Lint and typecheck clean.

Measurements

Serialization only, consumed through Readable.from as makeRequest does, same machine, interleaved runs, Node 22, median per run with the range across runs. master is current master (with #1781).

Case	master	this PR
TVP, 200k rows as an array (int, nvarchar(50), bit)	3.3M rows/s	4.1M–4.3M
TVP, 200k rows as an array (int, nvarchar(50), int)	2.4M–3.2M	3.8M–4.4M
TVP, 200k rows from an async iterable	1.7M	1.9M
bulk load, 200k rows (int, nvarchar(50), float)	2.87M	2.99M
bulk load, 200k rows of 40 int and varbinary columns	0.65M	0.56M–0.63M
100 scalar parameters of 5 types per request	53k–55k req/s	50k–52k
one 1 MB varbinary(max) parameter	33k req/s	33k

The gain is per cell: no separate validate and write calls through the type object, no cell object, and one monomorphic call per column instead of call sites that see every type. The two-type wide bulk load row was already served well by a polymorphic call site and sits within noise of master, slightly below in these runs. Scalar requests pay for a closure per parameter and a second validation, about a microsecond per 100 parameters, which the round trip dwarfs; the shared-closure variant for fact-free types recovers half of that and costs the same again on rows, so the writers stay per column.

Not in this PR

• Stable declarations for a prepared or compiled request: with type, length, precision and scale fixed up front, a request's writers, TYPE_INFO and declaration string can be compiled once and reused. The pieces are in place; this PR does not add the API.

🤖 Generated with Claude Code

https://claude.ai/code/session_01Fxv5h4UMCGJEpjgCKcAxug

[claude]

Review: unify writeValue/writeValueStream, add compileWriter

Went through the diff in detail (data-type.ts, the three streaming data types, tvp.ts, bulk-load-payload.ts, rpcrequest-payload.ts) plus the new/updated tests. Overall this is a careful, well-tested refactor — the writeRest generator that centralizes the close-on-early-stop / error-identity rules is a nice consolidation of logic that used to be duplicated inline in RpcRequestPayload, and the new compiled-writers-test.ts does a good job cross-checking compileWriter output against validate + writeValue for both the migrated types and the adapter fallback.

Bug: stale reference to the removed writeValueStream method name

src/data-types/tvp.ts:238:

if (!Array.isArray(value.rows)) {
  throw new TypeError('A TVP whose rows are an async iterable can only be written through writeValueStream.');
}

writeValueStream no longer exists anywhere in the codebase (DataType.writeValue replaced it). This message is on the "legacy" generateParameterData path, so it's rarely hit, but if it ever fires the error text now points to a method deleted in this same PR. Should read writeValue, matching the sibling message two lines below ('A TVP cell read from a source can only be written through writeValue.') which was correctly updated.

Minor: duplicated error-message construction in tvp.ts

writeRow's catch block builds the per-cell error inline:

throw new InputError(`TVP column '${columns[k].name}' has invalid data at row index ${rowIndex}`, { cause: error });

while the new columnError(column, rowIndex, error) helper (added a few lines above, and used by finishRow) builds the exact same message. Worth having writeRow call columnError(columns[k], rowIndex, error) too, so the two call sites can't drift apart if the message ever changes.

Observation: intentional logic duplication between validate/writeValue and compileWriter

Int.compileWriter, and the non-max branches of VarChar/NVarChar/VarBinary.compileWriter, re-implement the same validation/encoding logic that already lives in validate/writeValue rather than delegating to them — that's presumably deliberate, to avoid the per-cell object allocation/dispatch overhead the PR description calls out. Reasonable trade-off, but it means these implementations can silently drift apart over time (e.g. a future fix to Int.validate's range check not mirrored in compileWriter). compiled-writers-test.ts covers this well today by asserting byte-for-byte and error-for-error parity between the two paths, so as long as that suite is kept in sync with future changes to these types it should catch regressions — just flagging the coupling since it isn't enforced by the type system.

Things that look solid

• writeRest's handling of the three distinct failure/termination paths (source throws, consumer throws in, consumer stops early) correctly preserves error identity vs. wrapping, and avoids double-closing the iterator (the done guard). This matches the behavior of the deleted inline version in RpcRequestPayload, just centralized and shared with the new row loops.
• The TVP writeRow/finishRow split (resuming a row's remaining cells after a streamed cell's rest completes) correctly re-derives the next pending cell via recursion into writeRow rather than looping imperatively, and skips the row-shape validation on resumption (from !== 0) since it already ran once for that row.
• Column-length handling in the new compileWriter implementations (column.length! <= this.maximumLength) is consistent with how writeTypeInfo/writeColumns already treat column.length for TVP metadata, so length: Infinity for a varchar(max)/nvarchar(max)/varbinary(max) TVP or bulk-load column behaves the same in both places.
• Good negative-path test coverage: rejecting an async-iterable cell for a non-max column, propagating a mid-stream source failure as the column's wrapped InputError (with the cause chain asserted), and confirming the row source's finally runs (i.e. it's actually closed) when a cell's source fails mid-bulk-load.

Nit

The Request.addParameter/connection.ts doc comments were correctly updated to mention that TVP/bulk-load max cells can now be async iterables — this is a user-facing capability addition (not just an internal refactor), so it may be worth a changelog/release-notes mention if this repo tracks one elsewhere.

No build/test tooling was run as part of this review (sandboxed environment) — this is a static read of the diff plus tracing through the referenced code paths, so please confirm CI is green before merge.

[claude]

Review

I read through the full diff plus the surrounding (unchanged) code in data-type.ts, plp-stream.ts, and the four migrated types to check the new compileWriter path for byte-for-byte parity with the existing validate + writeValue path, and to trace the async "cell read from a source" control flow through tvp.ts and bulk-load-payload.ts.

Overall

This is a clean, well-scoped perf change. The compileWriter/CellWriter abstraction is a sensible generalization of the writeValue contract from #1777 (resolve-once, write-many instead of per-cell validate + adapter dispatch), and it's backed by real byte-parity tests (compiled-writers-test.ts) rather than just behavioral assertions, which is exactly what you want when duplicating validation/serialization logic for speed. I traced each migrated type (Int, NVarChar, VarBinary, VarChar) against its validate/writeValue counterpart and didn't find a byte or error-message mismatch — the check order (null → type check → collation → codepage) is preserved everywhere it matters.

The TVP row-continuation logic (writeRow/finishRow in tvp.ts) is correct as far as I can tell: cells are processed strictly sequentially (a pending cell's rest is fully drained via writeRest before the next column's writer runs), so the per-column reused cell/closure state in the generic compileWriter adapter is never touched concurrently. The "two streamed cells in the same row" case is exercised by the new integration test (b and c both chunked), which is good — that's the case most likely to break if someone later tried to parallelize column writes.

Minor points

1. bulk-load-payload.ts:163 — writeRest(rest, (error) => error as Error) doesn't actually wrap the error with column context the way tvp.ts's columnError does; it's just an identity function with a type assertion (so a non-Error throw from a user's source stream passes through unchanged, assertion notwithstanding). The PR description already calls this out as a deliberate gap pending fix: wrap bulk load serialization errors in InputError #1772, so I'm not asking for it to be fixed here — just flagging it so it doesn't get lost, since right now a failing streamed bulk-load cell surfaces without the column name while the equivalent TVP failure does include it (as covered by the "surfaces a failing source as the column's InputError" test, which only covers the TVP path).

2. Duplication risk — compileWriter re-implements the null/type/range checks that already live in each type's validate/writeValue. That's an inherent cost of the "resolve facts once" design and is mitigated well by the parity tests, but it does mean any future bug fix or edge-case change to Int/NVarChar/VarBinary/VarChar's validate or writeValue needs a matching update in compileWriter, with nothing but the test suite to catch drift. Might be worth a one-line comment at each compileWriter site pointing back at its validate/writeValue twin (some already have this, e.g. varchar's encode comment) so a future editor knows to keep them in sync.

3. Nit: in int.ts's compileWriter, value starts as unknown from the CellWriter signature but gets reassigned (value = Number(value)) and then cast with as number three times — works fine, but a local let n: number would read a little cleaner than the repeated casts. Not blocking.

Test coverage

Good — the unit suite checks byte parity and error parity per type (including the adapter path for Bit/Float/DateTime), rejection of a source for a non-max column, mid-row continuation, and source-failure propagation with source cleanup (the "closes the row source" bulk-load test is a nice touch). The integration tests round-trip a real streamed varbinary(max)/nvarchar(max) cell through both a TVP proc and a bulk load with uneven/empty chunks crossing packet boundaries, which is the scenario most likely to expose a PLP-chunking bug. I didn't spot a gap here.

Security

No concerns — this is purely wire-serialization logic; no new string interpolation into SQL, no new deserialization of untrusted input.

Performance

The measurements in the description are consistent with what the code does: one validate-shaped branch resolved per column instead of per cell, and a monomorphic call site per column instead of one call site fanning out over every column's type. Looks legitimate.

[claude]

Review

I read through the full diff (src/data-type.ts, data-types/{int,nvarchar,varbinary,varchar,tvp}.ts, bulk-load-payload.ts, request.ts/connection.ts docs, and the new compiled-writers-test.ts) plus the previous versions of the touched files to diff behavior, not just code shape. Overall this is a well-scoped, carefully engineered change — the compileWriter/CellWriter contract is a clean generalization of the existing resolve/writeValue split, the generic adapter (compileWriter in data-type.ts) is a sensible zero-cost fallback for un-migrated types, and the TVP/bulk-load row loops correctly thread the "pending cell" state through writeRow/finishRow so that only one cell's async rest is ever in flight per row. I did not find any correctness bugs.

Correctness spot-checks I ran mentally

• Shared mutable cell in the generic compileWriter adapter (data-type.ts): this reuses one ParameterData object across every cell of a column. That's only safe if the writer for a given column is never re-entered while a previous call's returned AsyncIterable rest is still being drained. I traced writeRow/finishRow (TVP) and the column loop in BulkLoadPayload and confirmed both stop advancing a row the moment a writer returns a rest, and don't move to the next row until that rest is fully drained via writeRest. So this holds even for a future type that gains streaming support without implementing its own compileWriter.
• isTextType[i] && value == null in BulkLoadPayload: this now checks the raw cell value instead of the previously-validated one (validation is now deferred into the compiled writer). This is only equivalent if type.validate(x) == null iff x == null for Text/Image/NText — I checked all three validate implementations and confirmed that invariant holds today, so behavior is unchanged. Worth keeping in mind if one of those three ever grows different null semantics.
• Legacy TVP.generateParameterData vs. streamed cells: correctly throws TypeError('A TVP cell read from a source can only be written through writeValue.') when a compiled writer returns a pending cell in that sync-generator code path, so the new streaming feature can't silently desync the legacy path.
• Error parity: compileWriter implementations for Int/NVarChar/VarBinary/VarChar throw the same TypeError messages as the corresponding validate, and the new unit tests assert this directly against validate + writeValue byte-for-byte, which is a good way to pin this down.
• Bulk load cell errors still aren't wrapped with column context (no InputError naming the column, unlike the TVP path) — this matches pre-PR behavior exactly (I diffed against the prior bulk-load-payload.ts), and the PR description already calls this out as deferred to fix: wrap bulk load serialization errors in InputError #1772, so it's not a regression.

Minor, non-blocking nits

• VarChar.compileWriter's non-max branch re-checks collation/codepage presence on every single cell call inside encode(...), even though column.collation is fixed for the whole column/all rows. Could be hoisted out of the returned closure and checked once at compile time (mirroring how the max branch already only needs isAsyncIterable/Buffer.isBuffer per cell). Very small win, purely a style/perf nit.
• NVarChar/VarBinary's legacy writeValue/generateParameterData still contain a value instanceof Buffer branch that validate (and now compileWriter) can never actually feed, since validate throws for non-strings/non-buffers as appropriate — pre-existing dead code, not introduced here, just noting it's still there if a future cleanup pass wants it.
• The "hands chunks on while the cell is being read" tests exercise a streamed cell that isn't the last column in the row; a variant with the streamed cell in the last column position (boundary where writeRow's continuation loop immediately falls out with from === len) would round out the boundary coverage, though the "serializes exactly as in memory" tests do exercise that shape for content correctness already.

Test coverage

The new test/unit/compiled-writers-test.ts is thorough — byte-for-byte parity against validate+writeValue for both migrated and adapter-backed types, explicit error-message parity, source-rejected-for-non-max-column checks, and TVP/bulk-load streaming behavior including a failing source closing the row iterator. Combined with the new integration tests (uneven/empty chunks crossing packet and flush boundaries) this gives good confidence in the byte-level behavior. I wasn't able to run the suite in this sandbox (no network access for npm ci), so this review is based on static analysis rather than an executed test run — worth having CI green before merge as usual.

Security

No new attack surface: values still go through the same validate logic (just resolved once per column instead of once per cell), and the streaming paths properly close the underlying async iterator on error/early-cancellation (traced through writeRest's finally/catch and BulkLoadPayload's close()/catch/finally), so a failing or abandoned row source shouldn't leak a hanging stream.

Nice work — this is a solid, low-risk perf change given how carefully the byte-parity and streaming-edge-case tests pin down behavior.

[claude]

Review: compile a writer per column, and read a row's max cells from a source

Nice piece of work — the compileWriter/writeRest split is a clean generalization of the parameter-resolution pattern from #1777, and it's backed by a strong byte-parity test suite (test/unit/compiled-writers-test.ts) that checks the compiled writers against validate + writeValue for both the migrated types and the generic adapter. I read through data-type.ts, the four migrated types (int, nvarchar, varbinary, varchar), tvp.ts, bulk-load-payload.ts, and the new/changed tests.

Code quality / best practices

• The compileWriter adapter fallback in src/data-type.ts (reusing a single ParameterData cell per column, mutating just .value) mirrors the pre-existing TVP cellsFor pattern, so every type gets a working (if not accelerated) writer for free. Good incremental-migration design — matches the PR's own stated goal of letting types opt in one at a time.
• tvp.ts's writeRow/finishRow split (return a PendingCell instead of driving the async rest inline) keeps the hot, fully-synchronous-row case allocation-light, which lines up with the measured perf gains.
• Minor nit: src/data-types/tvp.ts:130 has a stray comment // TvpColumnData above the writers[k](buffer, row[k]) call. It doesn't reference any type or symbol that exists elsewhere in the codebase (grepped — no hits) and reads like a leftover annotation from an earlier draft. Worth deleting for clarity.

Potential bugs

• Nothing that looks like a functional regression. The compiled writers for Int, NVarChar, VarBinary, and VarChar are line-for-line consistent with their validate/writeValue counterparts (null handling, string/buffer type checks, PLP vs. fixed-length branch selection, error messages), and this is exercised directly by the new byte-parity tests.
• One asymmetry worth confirming is intentional: in tvp.ts, a cell (or its streamed "rest") that fails is wrapped via columnError into an InputError naming the column and row index, but the equivalent path in bulk-load-payload.ts:163 wraps a failing streamed cell with the identity function (error) => error as Error (no column attribution), and a synchronous writers[i](buffer, value) throw at line 160 isn't wrapped at all. Per the PR description this is deliberately deferred to fix: wrap bulk load serialization errors in InputError #1772, and it matches the pre-existing (unwrapped) behavior for bulk load, so it's not a regression — just flagging it so it doesn't get lost as a "known gap" once this merges ahead of fix: wrap bulk load serialization errors in InputError #1772.

Performance

• The core motivation (monomorphic per-column call site, declaration facts resolved once instead of per cell, no validate + separate writeValue allocation) is sound, and the reported numbers (TVP ~2.35M rows/s vs ~1.88M, bulk load wide rows ~0.7M vs ~0.51M) are consistent with removing exactly that per-cell overhead. Good that the benchmark table separates the "feat: stream max and table-valued parameters from async sources #1777 head" baseline from master so the incremental gain of this PR specifically is visible.

Security

• No concerns. This is purely a serialization-path refactor; no new external input parsing, no new trust boundaries. The max-column streaming reuses the same async-iterable contract (and the same collation/encoding checks) already established and reviewed for parameters in feat: stream max and table-valued parameters from async sources #1777.

Test coverage

• Strong: test/unit/compiled-writers-test.ts covers byte parity, error parity (including the adapter path for Bit/Float/DateTime), rejection of a streamed source on a non-max column, the rest-of-write contract for a max column, TVP/bulk-load row streaming (array and async sources), early-stop source closing, and error propagation/identity for a failing source.
• Integration tests (bulk-load-test.ts, streaming-parameters-test.ts) round-trip real varbinary(max)/nvarchar(max) cells against a live server with uneven, empty-including chunk boundaries that cross packet/flush thresholds — good, since that's exactly the class of bug (off-by-one chunk boundaries) that's easy to miss with purely in-memory unit tests.
• One gap: I didn't see a unit test asserting that a bulk-load row's failing cell (sync or streamed) surfaces as a bare error rather than an InputError, i.e. locking in the asymmetry noted above so a future change doesn't silently "fix" it in a way that conflicts with fix: wrap bulk load serialization errors in InputError #1772's planned wrapping. Not blocking, just a thought for whoever picks up fix: wrap bulk load serialization errors in InputError #1772.

Overall this looks solid and well-tested; I'd be comfortable with this merging as-is modulo the stray comment cleanup.

[codecov]

Codecov Report

❌ Patch coverage is 99.65870% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 82.88%. Comparing base (c14d9ea) to head (ae3c087).

Files with missing lines	Patch %	Lines
src/data-types/binary.ts	87.50%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1780      +/-   ##
==========================================
+ Coverage   82.49%   82.88%   +0.39%     
==========================================
  Files          95       95              
  Lines        4862     4915      +53     
  Branches      913      911       -2     
==========================================
+ Hits         4011     4074      +63     
+ Misses        575      570       -5     
+ Partials      276      271       -5     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[claude]

Review

I read through the diff (src/data-type.ts, src/data-types/{int,nvarchar,varbinary,varchar,tvp}.ts, src/bulk-load-payload.ts, src/connection.ts, src/request.ts) plus the new/updated tests, and traced the compileWriter contract end to end against the pre-existing validate + writeValue path. Overall this is a solid, well-scoped perf change with unusually thorough validation.

Code quality / design

• The compileWriter contract is clean: it composes naturally with the existing writeValue/writeRest streaming machinery from feat: stream max and table-valued parameters from async sources #1777, and the fallback adapter in data-type.ts:238-252 means every type keeps working even before it gets a native implementation. The PendingCell/finishRow state machine in tvp.ts correctly handles multiple streamed cells in the same row (verified by tracing writeRow's from parameter through finishRow's loop).
• Minor DRY nit: tvp.ts:132 inlines the same InputError message that columnError() (line 67-69) already builds, just for the non-streamed path in writeRow. Worth reusing columnError there too, e.g. catch (error) { throw columnError(columns[k], rowIndex, error); }.
• The four native compileWriter implementations (Int, NVarChar, VarBinary, VarChar) each re-implement the validation logic that already lives in validate()/writeValue() for that type (e.g. int.ts:53-71 vs 73-95 are near-identical). This is presumably deliberate — the whole point is to avoid the extra call/object per cell — and it's well-guarded by compiled-writers-test.ts's byte/error-parity assertions against validate + writeValue. Still, it's worth flagging for future maintainers: a change to one of validate/writeValue for these types needs a matching change to compileWriter, and only the parity test will catch a miss.
• tvp.ts:129 has a stray // TvpColumnData comment carried over from the pre-PR code; it previously sat next to a writeValue(column.type, buffer, cell, options) call and made a bit more sense as a type-annotation marker. Now that the call is writers[k](buffer, row[k]), the comment doesn't obviously refer to anything — consider dropping it or replacing it with something meaningful.

Potential bugs

• I specifically checked the bulk-load-payload.ts change where value used to be the validated value (via c.type.validate(...)) before the isTextType[i] && value == null null-pointer check, and is now the raw, unvalidated value. This looked like a possible behavior change at first, but Text/Image/NText's validate() (the only three types in isTextType) all return null iff the raw value is == null, so the check is equivalent either way — no bug found there.
• I also checked whether column.length for TVP columns (tvp.ts's writersFor) is resolved the same way as bulk load columns (which get resolveLength applied in BulkLoad.addColumn per the recent refactor: resolve parameter lengths for all types that can resolve one #1771 fix). TVP columns still take column.length as given by the caller with no resolveLength fallback — but this matches pre-existing behavior (confirmed against origin/master), so it's not a regression introduced here, just a pre-existing gap this PR doesn't need to fix.
• The reused-cell-object adapter in compileWriter (data-type.ts:245) is safe under the current sequential invariants (a row's cells, and rows themselves, are always fully written/drained before the next call), but it's worth being aware this would become unsafe if a caller ever invoked the same compiled writer concurrently for two cells before the first's rest (if any) was drained. Not an issue today since none of the adapter-only types (Bit, Float, DateTime, etc.) accept streamed values.

Performance

The benchmarks in the PR description are convincing and the approach (resolve declaration facts once per column instead of per cell, monomorphic call site per column) is the right lever for this workload. No concerns.

Security

No new attack surface — this is internal wire-serialization code operating on already-validated/typed values; no injection or memory-safety concerns spotted.

Test coverage

This is the strongest part of the PR. compiled-writers-test.ts checks byte-for-byte and error-for-error parity against validate + writeValue for both migrated and adapter-fallback types, plus source-rejection-on-non-max-columns, multi-cell TVP streaming, and bulk-load failure/close semantics. The integration tests exercise real round-trips with uneven/empty chunks across packet and flush boundaries. I don't see gaps here.

Nice work — nothing blocking, just the two small nits above (DRY on the TVP error construction, and the stray comment) if you want to tidy before merge.

[claude]

Review

Nice piece of work: the compileWriter/writeRest split is a clean way to get per-column monomorphism without special-casing streamed cells at every call site, and the new test/unit/compiled-writers-test.ts (byte-parity and error-parity vs validate + writeValue, plus streaming round-trips with uneven/boundary-crossing chunks and source-cleanup-on-failure) gives solid confidence that "bytes are unchanged for every existing input" actually holds. I traced through writeRow/finishRow in tvp.ts and the loop in bulk-load-payload.ts, and the resumable-cell bookkeeping (index + rest, drained before moving on) looks correct, including the rowIndex/from handling across chunk-yield boundaries.

A few things worth a look:

1. Bulk load validation errors now surface as InputError, not the raw error (behavior change)
Before this PR, BulkLoadPayload called c.type.validate(...) outside the try/catch, so a bad value (e.g. an invalid date) threw the raw TypeError straight out of the bulk load. Now validate runs inside the compiled writer, which is called inside the try that wraps everything as InputError (Column X could not be serialized, with the original error as .cause) -- reflected in the updated test/integration/bulk-load-test.ts assertions (TypeError changed to InputError with a TypeError cause).

This brings bulk load in line with how TVP/parameter validation errors are already wrapped, which looks like a genuine improvement, but it is a visible error-type change for any consumer catching a specific error class out of a bulk load. Given the PR own follow-up note ("#1772 wraps the bulk load per-cell write in an InputError naming the column; once it lands, that wrap belongs around the compiled writer call...") it would be worth double-checking whether this wrapping is meant to land here now or is expected to arrive via #1772, and either way calling it out explicitly in the changelog/release notes as a behavior change, not just a refactor.

2. Duplicated validation/serialization logic (maintainability)
Int.compileWriter, NVarChar.compileWriter, VarChar.compileWriter, and VarBinary.compileWriter each reimplement the same checks already present in validate() (and the PLP-writing logic already in writeValue()), rather than delegating. That is presumably deliberate for the monomorphic call-site performance win described in the PR, and the new byte-parity unit tests do guard against drift, but it does mean any future tweak to Int.validate range-check message (for example) has to be mirrored by hand in compileWriter, with only a test to catch it if someone forgets. A short comment at each compileWriter site pointing back at the paired validate/writeValue it must stay in sync with could help (a couple already do this, e.g. varchar compileWriter comment -- could be more consistent across the four).

3. Minor nits in tvp.ts

• writeRow catch block builds InputError inline (TVP column ... has invalid data at row index ...) instead of calling the columnError() helper defined a few lines above and used in finishRow -- small DRY opportunity so the two messages cannot silently diverge.
• The // TvpColumnData comment (carried over from the pre-refactor code) now sits right before rest = writers[k](buffer, row[k]); with no clear referent nearby -- looks like a vestigial marker at this point; consider dropping it or re-anchoring it.

Other areas checked, no issues found:

• The shared reused cell object in compileWriter generic fallback (data-type.ts) is safe under current call sites because every caller fully drains any returned rest via writeRest before invoking the writer again for that column -- worth keeping in mind if a future call site ever pipelines multiple in-flight cells for the same compiled writer, since that invariant is not enforced by the types.
• The isTextType && value == null check in BulkLoadPayload was moved to run on the raw value instead of the validated value -- verified this is equivalent since validate() for Text/Image/NText returns null if and only if the raw value is null/undefined, so no behavior change there.
• No security concerns -- this is pure binary-protocol serialization, no string-built queries or untrusted-input execution paths.
• Performance rationale and measurements in the PR description are consistent with the code changes (per-column writer compilation, no per-cell object allocation for the migrated types).

Overall: solid refactor with good test coverage; the main thing worth confirming before merge is whether the InputError-wrapping behavior change for bulk load validation errors (point 1) is intended to ship in this PR.

[claude]

Review

I read through the full diff (src/data-type.ts, src/data-types/{int,nvarchar,varchar,varbinary,tvp}.ts, src/bulk-load-payload.ts, plus the new/updated tests) and cross-checked the new code against the existing writeRest/streaming pattern from #1777 (rpcrequest-payload.ts). Overall this is a clean, well-scoped perf change with strong test coverage.

Code quality / best practices

• The compileWriter/CellWriter contract is a sensible extension of the existing resolve/writeValue split, and the fallback in compileWriter() (data-type.ts) correctly adapts any type without its own compileWriter via a single reused ParameterData cell. Verified this is safe because the only writeValue implementations that ever return an async "rest" (NVarChar, VarChar, VarBinary, TVP) all have their own compileWriter, so the reused cell object is never captured by a still-running async generator.
• Minor DRY nit: in tvp.ts, writeRow's initial per-cell loop builds the InputError message inline (`TVP column '${columns[k].name}' has invalid data at row index ${rowIndex}`) which duplicates columnError() a few lines above, used only by finishRow. Could just call columnError(columns[k], rowIndex, error) there too.
• The // TvpColumnData comment above rest = writers[k](buffer, row[k]); predates this PR and reads a bit stale now that the surrounding code no longer builds a ParameterData-shaped cell inline — not something this PR needs to fix, but worth a pass next time that area is touched.

Potential bugs

• Nothing that looks incorrect. Specifically checked:
  • Byte-for-byte parity between each compileWriter and its validate+writeValue pair (including error ordering — e.g. VarChar's encode() checks "not a string" before "no collation", matching validate's order).
  • tvp.ts's row-resumption logic (writeRow(..., from) / finishRow) for rows with more than one streamed max cell — the index-resume trick correctly picks up after each streamed cell and handles multiple streamed cells in the same row (exercised by the tests that stream both a varbinary(max) and an nvarchar(max) cell in the same row).
  • Resource cleanup: a failing source cell correctly closes the row source in both BulkLoadPayload and via writeRest's iterator.return() handling.
• Behavioral change worth flagging explicitly (already documented in the PR description and covered by updated tests, so not asking for a change, just noting it for visibility): a BulkLoad row whose cell fails validation now surfaces as InputError ("Column 'X' could not be serialized", with the original error as .cause) instead of the raw validation error, because validate now runs inside the same try/catch as writeValue via the compiled writer. Previously only writeValue failures were wrapped; validate failures propagated raw. Since InputError extends TypeError, instanceof TypeError checks still pass, but code that inspects error.message directly for a bulk-load row validation failure will now see a different message. This makes bulk-load error reporting consistent with how TVP row errors were already reported, which seems like a net improvement — just worth a mention in release notes if this project calls out user-facing error-shape changes.

Performance

• Design matches the stated goals: compileWriter resolves column facts once per column instead of per cell, and the array-of-rows path stays fully synchronous except for the (rare) streamed-cell case, avoiding a promise per row for the common case. The benchmarks in the description look consistent with that.

Security

• No concerns — this is purely an internal serialization-path refactor; no new external input parsing or trust boundary changes.

Test coverage

• Strong. compiled-writers-test.ts covers byte parity, error parity (including the adapter-fallback types Bit/Float/DateTime), rejecting a source on a non-max column, multi-chunk sources, and both TVP- and bulk-load-row streaming (including mid-read cancellation and source-failure/close-propagation cases). Integration tests round-trip large payloads with uneven/empty chunks crossing packet and flush boundaries for both TVP and bulk load. No gaps identified.

No blocking issues found from this read-through; the DRY nit above is optional polish.

[claude]

Review

This is the last of a multi-PR series (#1777, #1781, this one) that replaces writeValue with compileWriter, so every type now compiles a per-column/per-parameter closure that captures its declaration facts once and validates+writes each cell in one call. The design is sound, and the commit history shows the tricky correctness issues (error identity across for await/yield*, early-close semantics, surrogate-pair splitting, row/column-count desync) were already found and fixed in earlier iterations of this branch. Measurements back up the perf claims for TVP/bulk-load rows.

Bug: leftover duplicated/misindented comment in varchar.ts

src/data-types/varchar.ts, in compileWriter's narrow (non-max) branch, has this:

return (buffer, raw) => {
  // A buffer is the value as `validate` encoded it, for a parameter
  // resolved before the request is written.
  // A buffer is the value as `validate` encoded it, for a parameter
  // resolved before the request is written.
  const value = Buffer.isBuffer(raw) ? raw : VarChar.validate(raw, collation);
    if (value == null) {

The comment is duplicated verbatim, and the indentation of const value and the lines after it drifts between 6 and 8 spaces relative to the rest of the block. Looks like a merge/edit artifact left over from the many master merges on this branch. It doesn't affect behavior, but it will trip eslint's indent rule and should be cleaned up to one copy of the comment at consistent 8-space indentation — matching the identical, correctly-formatted comment a few lines below in the max branch of the same function.

Design notes (not blocking)

Deliberate double-validation on the RPC scalar path: RpcRequestPayload now calls compileWriter(...)(buffer, parameter.data.value) where parameter.data.value already went through resolve/validate once, so the compiled writer validates it again. This is explicitly called out in the PR description and covered by the 40-type byte-identity test; for VarChar/Char/Text the writer special-cases an already-encoded Buffer to avoid re-encoding, so it looks correctly handled. Just flagging it since running validation twice per scalar parameter is a non-obvious behavior change worth being explicit about.

TVP and bulk-load cells skip resolve entirely: the compiled writer validates raw row values directly, which is correct (no double validation there) and is what gives most of the perf win.

writeRow/finishRow in tvp.ts re-check row shape (array-ness, length) only when resuming at from === 0. Since row.length doesn't change across resumed calls this is fine, but it is easy to misread; a short comment noting why the check doesn't repeat on resume would help future readers.

Test coverage

Coverage looks thorough: compiled-writers-test.ts covers validation-error parity with validate, rejecting a streamed source on a non-max column, TVP/bulk-load row parity between in-memory and streamed cells, chunk hand-off during a slow read, and error propagation including source-close-on-failure. Integration tests round-trip streamed max cells across packet/flush boundaries for both TVP and bulk load. I did not run the suite in this environment, but the described results (543 unit / 331 integration passing) line up with the scope of the diff.

Minor

bulk-load-payload.ts and tvp.ts both build their per-column writers with the same columns.map((c) => c.type.compileWriter({ length: c.length, scale: c.scale, precision: c.precision, collation: ... }, options)) shape. Not necessary to change, just noting the duplication in case a shared helper in data-type.ts is wanted later.

Overall: solid refactor; the one concrete issue is the duplicated comment/indentation in varchar.ts.