perf(qwp): avoid repeated validation, row promises, and frame planning

[glasstiger]

Summary

• Reuse validated builder identifiers from the sender’s staged/published schema rather than rescanning on every row; exact-name checks avoid case-folded UTF-8 length mistakes and do not introduce a global unbounded cache.
• Avoid allocating a promise for every QWP row on the no-flush path while preserving the Promise/rejection contract of public row closers and writer.row().
• Reuse the measured ingress frame plan for encoding; keep symbol-dictionary rollback on failure.
• Also adds .agents/skills/review-pr/SKILL.md, a port of the review-pr review skill to Pi. It is tooling only and does not affect the published packages.

Validation

• pnpm format:check, pnpm typecheck, pnpm typecheck:test, pnpm eslint
• pnpm vitest run test/qwp/core.test.ts test/qwp/sender.test.ts (163 passed)
• Previously verified full non-integration suite (1097 passed), pnpm test:dist (39 passed), and pnpm typecheck:dist.
• On a local QuestDB nightly container, six interleaved 3M-row A/B pairs (BATCH=50000) against the same base commit: builder median 511,684 → 655,621 rows/s (+28.1%); row API median 590,226 → 667,622 rows/s (+13.1%). All runs verified row counts. The container was stopped afterward.

[glasstiger]

Review: perf(qwp): avoid repeated validation, row promises, and frame planning

Verdict: approve. I found no Critical or Moderate issues, and the test gate passes. This was a level 3 review.

• Head: e9eaf767 · Base: 68dfe19a
• Submodules: none.
• Changed files: the QWP ingress hot path, in packages/client-core/src/_qwp/sender.ts, _core/ingress.ts and ingress-session.ts. I treated these as high risk.

Critical

None.

Moderate

None.

What I checked

• Skipping name validation for known tables and columns. table(), addColumn and omitsNullish now skip validation for names they already hold. That is safe:
  • Every key in tablesByName was validated, either by table() or by compileWriterSchema via QwpTableBuffer.
  • Every non-empty .name in schema or publishedSchema was validated with the same readonly maxNameLength. That covers canonicalName, a writer's wireName, and names from releaseStagedRows.
  • The designated timestamp's empty name always goes back through validation.
  • Error types and messages are the same as base.
• at(), atNow(), row() and rows() behave as before. Every failure still rejects and never throws synchronously. Rows are discarded in the same cases as base, and a failing auto-flush still reaches the caller. The Node Sender wrappers and the pooled-client proxies still behave correctly.
• Frames encoded from a measured plan are byte-identical to base. Base already sized and wrote each frame from one plan, and re-planning at base gave the same result. Dictionary rollback ends in the same state because of the outer initialDictionarySize truncation.
• Tests catch regressions (mutation checks, run at head). I mutated each of these and a test failed every time:
  • at() returning SETTLED after starting a flush (10 tests failed)
  • rows() not awaiting a flush it started
  • table() caching names without validating them
  • key-only or empty-name matching in validateColumnName and in omitsNullish
  • dropping the dictionary rollback in encode()
  • dropping discardRow from at()
  • swallowing a failed auto-flush
• Head gates: typecheck, eslint and prettier are clean, and all 874 non-integration test/qwp tests pass.

Coverage

The test gate passes. No coverage gaps needed reporting: every changed behaviour is covered by the new or existing tests.

Summary

• Findings: none.

• Performance claim holds in direction. I ran the PR's own benchmarks/sender.bench.ts locally without a server, base versus head (hz):

  Benchmark	Base	Head	Change
  trades	40.0	49.9	+25%
  full dictionary	43.4	53.1	+22%
  delta dictionary, steady state	53.8	65.2	+21%
  sparse	28.1	31.7	+13%

• Tradeoff worth mentioning in the PR description. The null-value lookup in omitsNullish only speeds up a column the table already knows under exactly the spelling given, and in practice only all-lowercase names can match.

  Workload (20 null columns per row)	Base	Head	Change
  All-lowercase columns populated once	≈21	≈63	about 3× faster
  camelCase columns, or columns never populated	≈20.5–21.3	≈18.4–20.9	about 0–10% slower

  • The slowdown is about 8 ns per null cell, from two failed Map.get calls before the same validation as base. It mostly doesn't show up once network I/O is involved, so I did not report it as a finding.
  • The PR's own "validate nullish names and publish" bench is on that slower path, so it is flat: 9.38 at base, 9.22 at head.
  • I tried looking up by the case-folded key instead (qwpColumnNameKey). It was worse on every workload, so the current design is the better tradeoff.

• Minor scope note, not a finding: the PR also adds the unrelated .agents/skills/review-pr/SKILL.md tooling file. The description says so.

[glasstiger]

Review — level 3 (full pass)

Reviewed base 68dfe19a → head 5040c4e0. Verdict: approve. Nothing blocks; there is one small test-precision note.

Out of scope: .agents/skills/review-pr/SKILL.md and CLAUDE.md (tooling/docs only). Submodules: none.

Critical

None.

Moderate

None.

Minor

• Loose error checks in a new test. "rejects rather than throws from at() and atNow()" (test/qwp/sender.test.ts:781,798,802) uses a bare rejects.toThrow(), so any error satisfies it. The actual rejections at head are microsecond timestamp must be a safe integer (line 781) and QWP sender is closed (lines 798, 802). Matching on those messages, e.g. /safe integer/ and /closed/, would keep an unrelated failure from passing the test.
• PR description. The CLAUDE.md workflow note ("do not watch CI unless asked") isn't mentioned in the summary.

What was verified

• Frames are byte-identical to base. The same data went through the browser sender at both revisions: 3 flushes × 25 mixed rows over two tables (delta-dictionary symbols, nullable doubles and multi-byte UTF-8 strings, longs, a sparse symbol column, timestamps). This ran with Gorilla on and off, at byte caps of 300 / 700 / 2000 / 1 MiB; the smaller caps force the size-based splitting path (up to 13 frames). The SHA-256 of all sent frames matched in all 8 configurations.

• The new tests catch what they claim to. Nine mutations in a scratch worktree each failed at least one test:

  • removing the over-long-name check that runs before case-folding;
  • skipping validation on a case-insensitive-only match, in both validateColumnName and the nullish path;
  • dropping the empty-name guards;
  • removing the dictionary restore when a measured encode fails;
  • making at() / atNow() throw instead of reject;
  • dropping the await on a started flush in rows();
  • making row() non-async.

• No behaviour change for callers.

  • Every error path in at(), atNow(), writer.row() and rows() still rejects rather than throws. Rows are discarded as in base, and auto-flushes start at the same point.
  • The skip-revalidation shortcut is sound: every place that stores a table or column name validates it first against the same per-sender maxNameLength.

• Checks at head: test/qwp/ 887/887 pass; typecheck, eslint and prettier are clean.

• Performance. One local run of benchmarks/sender.bench.ts, so the numbers are indicative:

  Benchmark	Change vs base
  trades	+20%
  wide	+12%
  sparse	+6%
  full dictionary	+18%
  delta dictionary (cold)	+17%
  delta dictionary (steady state)	+16%
  never-populated nullish columns	no change (3 runs each)

  The nullish result is expected. A never-populated column is never in the schema, so the shortcut can't apply; that benchmark guards against a slowdown rather than measuring a speedup.

Summary

• Test gate: passes, with no coverage gaps to report.
• Findings: 0 Critical, 0 Moderate, 2 Minor.
• Tradeoff: at() / atNow() now return one shared, already-resolved promise when no flush starts, so they settle a microtask or two earlier. Nothing in the repo or the documented contract depends on promise identity or tick count.

[glasstiger]

Review — level 3

Verdict: approve. Reviewed 68dfe19a → 94f53bf6. No admitted Critical, Moderate, or Minor findings; no in-diff or out-of-diff breakage. The test gate passes with 0 admitted coverage gaps.

Validation in isolated worktrees: all 1,111 tests passed (using the unchanged interop fixture), along with 39 test:dist tests, typecheck, typecheck:test, typecheck:dist, ESLint, and formatting checks. Base and head produced identical bytes for four mixed ingress frames (full/delta dictionaries, Gorilla on/off). The primary checkout was not modified.

Tradeoff: never-populated null columns now incur extra schema lookups. Two short in-process benchmark comparisons showed differences too small to establish a material regression. The local sender benchmarks supported the performance improvement's direction, but I did not reproduce the PR's live-server throughput figures.

Submodules: none changed.