Skip to content

feat: decode inline Arrow IPC + warehouse-compat fallback#329

Open
jamesbroadhead wants to merge 19 commits into
mainfrom
stack/arrow-3-inline-arrow-fix
Open

feat: decode inline Arrow IPC + warehouse-compat fallback#329
jamesbroadhead wants to merge 19 commits into
mainfrom
stack/arrow-3-inline-arrow-fix

Conversation

@jamesbroadhead
Copy link
Copy Markdown
Contributor

@jamesbroadhead jamesbroadhead commented Apr 29, 2026

Summary

Some warehouses return ARROW_STREAM + INLINE results as base64 Arrow IPC in result.attachment rather than result.data_array. Some refuse JSON_ARRAY + INLINE entirely and only support ARROW_STREAM for INLINE. Others (most classic + some serverless) do the opposite — they refuse ARROW_STREAM + INLINE and require EXTERNAL_LINKS. The previous code path silently returned empty results in all of these cases.

This PR makes the analytics plugin work across all three warehouse shapes by:

  • handling both INLINE and EXTERNAL_LINKS dispositions for ARROW_STREAM,
  • stashing inline Arrow IPC server-side and serving it via the existing /arrow-result route,
  • falling back between dispositions based on what the warehouse signals it accepts,
  • preserving the caller's requested format end-to-end (a JSON_ARRAY caller always gets JSON rows; an ARROW_STREAM caller always gets Arrow bytes), regardless of which disposition the warehouse actually accepted.

Design

Earlier iterations of this PR sent inline Arrow bytes over SSE (base64 inside JSON inside SSE framing) using an arrow_inline message type. That was reworked: inline payloads are now stashed server-side and delivered through the same /arrow-result/<jobId> endpoint that already serves EXTERNAL_LINKS results. The SSE control channel only ever carries { type: "arrow", statement_id }; the id is either a real warehouse statement id or a synthetic inline-<uuid>, and the server demultiplexes.

Wins from the redesign:

  • Keeps multi-MiB blobs off SSE
  • One client code path for both INLINE and EXTERNAL_LINKS
  • Real binary content-type (application/vnd.apache.arrow.stream) instead of base64-in-JSON-in-SSE
  • The retired arrow_inline wire message is gone — the discriminated union rejects it as schema-invalid

Stash properties (see inline-arrow-stash.ts):

  • Drain-on-read (no replay path)
  • TTL-bounded, evicted on put/take
  • Per-user keyed
  • Memory-bounded with rejection on overflow — callers fall back to EXTERNAL_LINKS rather than evicting in-flight entries
  • Caveat: process-local. Multi-replica deployments need sticky sessions or a shared store (out of scope)

Disposition fallback

The plugin classifies an inline rejection into one of two signals (case-insensitive, requires INVALID_PARAMETER_VALUE / NOT_IMPLEMENTED plus mention of "inline" — unrelated errors aren't reinterpreted as disposition mismatches):

  • needs-arrow: warehouse says it only accepts ARROW_STREAM for INLINE ("Inline disposition only supports ARROW_STREAM format."). If the caller asked for JSON_ARRAY, retry as ARROW_STREAM + INLINE and decode the Arrow IPC attachment to plain row objects server-side — the caller's JSON contract is preserved. Scalar values are stringified to match the warehouse's native JSON_ARRAY shape.
  • needs-json: warehouse says it only accepts JSON_ARRAY for INLINE ("The format field must be JSON_ARRAY when the disposition field is INLINE.", "ARROW_STREAM is not supported with INLINE disposition"). If the caller asked for ARROW_STREAM, retry as ARROW_STREAM + EXTERNAL_LINKS.

For ARROW_STREAM callers, the stash-full case also falls back to EXTERNAL_LINKS. Explicit format requests are honored — no auto-downgrade across format boundaries (a JSON caller never gets Arrow bytes; an Arrow caller never gets JSON rows).

Changes

  • Server-side Arrow IPC decoding (connectors/sql-warehouse/arrow-schema.ts + client.ts): detects result.attachment and decodes via apache-arrow's tableFromIPC. apache-arrow@21.1.0 added as a server dependency.
  • Inline Arrow stash (plugins/analytics/inline-arrow-stash.ts): the bounded, TTL'd, drain-on-read, per-user keyed store described above.
  • Unified /arrow-result route (plugins/analytics/analytics.ts): single endpoint serves both warehouse statement ids and synthetic inline-* ids; differentiation is by id prefix.
  • Disposition classifier + bidirectional fallback (plugins/analytics/analytics.ts): replaces the prior heuristic, which case-sensitively required both INLINE and ARROW_STREAM in the error message and never matched the actual wire wordings.
  • JSON_ARRAY server-side Arrow→rows decoder: when retrying with ARROW_STREAM, decode and stringify so the SSE payload still looks like {type: "result", data: [...rows]}.
  • Zod-validated SSE wire protocol (shared/src/sse/analytics.ts): single source of truth between server and client. Default format remains JSON_ARRAY.

Tests

  • connectors/sql-warehouse/tests/arrow-schema.test.ts (new, 514 lines)
  • connectors/sql-warehouse/tests/client.test.ts (new, 383 lines) — real base64 Arrow IPC captured from a live serverless warehouse
  • plugins/analytics/tests/inline-arrow-stash.test.ts (new, 116 lines)
  • plugins/analytics/tests/analytics.test.ts (+770) — integration coverage for both fallback directions (warehouse refuses JSON_ARRAY + INLINE → server retries as ARROW_STREAM + INLINE and decodes; warehouse refuses ARROW_STREAM + INLINE → falls back to EXTERNAL_LINKS), plus stash-full fallback and the no-fallback safety net
  • appkit-ui/src/react/hooks/__tests__/use-analytics-query.test.ts — arrow-IPC SSE tests (this PR) merged with the stable-params tests already on main from fix(appkit-ui): stabilize useAnalyticsQuery params reference to avoid infinite refetch #321
  • shared/src/sse/analytics.test.ts (new, 87 lines)

Full suite: 2,674 tests, all green on the current head.

Stack

This was the third PR in a stack; both predecessors are now on main:

Test plan

Verified via automated tests on the current head:

  • useAnalyticsQuery reads ARROW_STREAM results via /arrow-result for both real warehouse statement ids and synthetic inline-* ids
  • JSON_ARRAY direct path: warehouse accepts INLINE + JSON_ARRAY → row data returned unchanged
  • JSON_ARRAY fallback path: warehouse refuses INLINE + JSON_ARRAY but accepts INLINE + ARROW_STREAM → server decodes Arrow attachment to JSON rows; SSE wire carries {type: "result"}, not {type: "arrow"}
  • JSON_ARRAY no-fallback safety: rejection without a needs-arrow signal does NOT trigger a retry
  • ARROW_STREAM + INLINE → EXTERNAL_LINKS fallback when the warehouse rejects INLINE
  • ARROW_STREAM + INLINE → EXTERNAL_LINKS fallback when the inline stash is full
  • Inline-stash invariants: drain-on-read, TTL eviction, per-user keying, reject-on-overflow leaves prior entries intact
  • Retired arrow_inline SSE message is rejected as schema-invalid and never triggers a local decode

Live smoke test against three e2-dogfood warehouses (SELECT 1 AS one, 'hello' UNION ALL SELECT 2, 'world'):

Warehouse JSON_ARRAY caller (default) ARROW_STREAM caller
serverless A (refuses JSON_ARRAY + INLINE) ✅ rejected on first attempt, fallback retries as ARROW_STREAM + INLINE, decoded to [{"one":"1","greeting":"hello"},{"one":"2","greeting":"world"}] ✅ INLINE + ARROW_STREAM accepted directly, attachment decoded
serverless B ✅ INLINE + JSON_ARRAY accepted directly ✅ INLINE rejected with needs-json, falls back to EXTERNAL_LINKS
classic ✅ INLINE + JSON_ARRAY accepted directly ✅ INLINE rejected with needs-json, falls back to EXTERNAL_LINKS

All three warehouses now return identical row shapes for the same default useAnalyticsQuery call.

Fixes #242. Replaces #256.

This pull request was AI-assisted by Isaac.

Renames the client-side analytics format model from "JSON"/"ARROW" to
"JSON_ARRAY"/"ARROW_STREAM" to match the Statement Execution API enum
verbatim — no more local-name to API-name translation.

Pure mechanical rename. No behavior change. Internal type values only;
the lowercase user-facing values passed to useChartData ("json", "arrow",
"auto") are unchanged.

Carved out of #256 (#327 is layer 1, this is layer 2). The actual
inline-Arrow-IPC + warehouse-fallback fix sits on top of this in layer 3.

Note: this is a breaking change for any direct consumer of
useAnalyticsQuery passing explicit format: "JSON" or "ARROW" — they will
need to update to "JSON_ARRAY" / "ARROW_STREAM". Consumers using
useChartData (lowercase "json"/"arrow"/"auto") are unaffected.

Co-authored-by: Isaac
Widen AnalyticsFormat to also include the pre-rename "JSON" and "ARROW"
spellings, both marked @deprecated with a JSDoc note describing the
removal condition (no consumer on appkit/appkit-ui < 0.33.0). Add a
normalizeAnalyticsFormat helper and call it at the analytics route
handler entry point so all downstream code (cache key, format
branching, formatParameters) continues to operate on the canonical
"JSON_ARRAY" | "ARROW_STREAM" values.

InferResultByFormat is widened to also match "ARROW" so callers
passing the legacy spelling still get TypedArrowTable<...> inferred.

This lifts the breaking-change carve-out from the rename, so callers
of useAnalyticsQuery({ format: "JSON" | "ARROW" }) keep working with
only an IDE deprecation hint.

Signed-off-by: James Broadhead <jamesbroadhead@gmail.com>
Serverless warehouses return ARROW_STREAM + INLINE results as base64 Arrow
IPC in result.attachment rather than result.data_array. The previous code
path discarded inline data for any ARROW_STREAM response (designed for
EXTERNAL_LINKS), so these warehouses silently returned empty results.

This commit makes the analytics plugin work across classic and serverless
warehouses by handling both dispositions for ARROW_STREAM, decoding inline
Arrow IPC attachments server-side, and falling back to JSON_ARRAY when a
warehouse rejects ARROW_STREAM + INLINE.

Changes
- Inline Arrow IPC decoding (new arrow-schema.ts) via apache-arrow's
  tableFromIPC, producing the same row-object shape as JSON_ARRAY
  regardless of warehouse backend. apache-arrow@21.1.0 added as a server
  dep.
- Format fallback: ARROW_STREAM + INLINE requests automatically fall back
  to JSON_ARRAY if a classic warehouse rejects them. Explicit format
  requests are respected without fallback.
- Zod-validated SSE wire protocol for /api/analytics/query (shared schema
  between server and client; malformed payloads surface a clear error
  instead of silent undefined).
- Default remains JSON_ARRAY for compatibility.

Stack: layer 3 of 3 carved from #256.
- #327 — coverage backfill (layer 1)
- #328 — AnalyticsFormat rename to API enum names (layer 2)
- (this PR) — the actual fix

Fixes #242

Co-authored-by: Isaac
@jamesbroadhead jamesbroadhead force-pushed the stack/arrow-3-inline-arrow-fix branch from ca69f8e to 08c5486 Compare May 11, 2026 16:38
Six issues surfaced by GPT 5.4 xhigh + Gemini 3.1 Pro parallel review
followed by an adversarial debate round (reviewer: GPT, critic: Gemini,
meta: Claude Opus).

1. Raise SSE event-size cap from 8 MiB to 12 MiB on both server
   (streamDefaults.maxEventSize) and client (connectSSE.maxBufferSize).
   The inline Arrow attachment cap (MAX_INLINE_ATTACHMENT_BYTES) stays
   at 8 MiB *decoded*; base64 encoding + JSON + SSE framing inflate that
   to ~10.6 MiB on the wire, so 12 MiB leaves enough headroom for legal
   8-MiB-decoded payloads to traverse the buffer.

2. Empty `data_array: []` is truthy, so zero-row ARROW_STREAM responses
   skipped empty-table synthesis and fell through to the JSON row
   transform — callers requesting Arrow got [] JSON rows. Length-check
   explicitly.

3. The arrow-fix commit dropped lowercase legacy "json" / "arrow" from
   DataFormat / resolveFormat(), silently breaking existing useChartData
   callers passing those spellings. Restore them as @deprecated aliases
   on the DataFormat union; resolveFormat() normalizes them to the
   canonical "JSON_ARRAY" / "ARROW_STREAM" return values.

4. The JSON_ARRAY -> ARROW_STREAM retry in DESCRIBE QUERY only fired on
   thrown exceptions. Some warehouses signal the rejection as
   `status.state === "FAILED"` instead. Extract the rejection-matcher
   helper and retry on both paths before degrading the typegen result
   to `unknown`.

5. analytics.test.ts:946 asserted `format: "JSON"` returns 400, but the
   route now accepts "JSON" as a legacy alias (normalized to
   JSON_ARRAY). Use a truly unsupported value ("CSV") so the test still
   exercises the malformed-format path.

6. Restore `zod: 4.3.6` to @databricks/appkit dependencies. main has it;
   the rebase conflict-resolution accepted the branch's older deps list
   which lacked it. appkit imports `zod` directly from several files
   (analytics.ts, agent tools, tests).

Co-authored-by: Isaac
Signed-off-by: James Broadhead <jamesbroadhead@gmail.com>
The original commit added zod@3.23.8 to shared for the new SSE wire
protocol schema. With zod restored on appkit at 4.3.6 (matching main),
the workspace now had two different zod majors resolving in different
packages — a latent peer-dep / type-incompatibility foot-gun even
though the schema itself was already cross-major-compatible.

Bump shared's zod to 4.3.6 so the whole workspace lands on one major.
The schema's two-arg `z.record(z.string(), z.unknown())` form is the
zod 4 spelling, so no functional change is needed; drop the now-stale
"keeps it valid under either major" comment.

Signed-off-by: James Broadhead <jamesbroadhead@gmail.com>
Signed-off-by: James Broadhead <jamesbroadhead@gmail.com>

# Conflicts:
#	packages/appkit-ui/src/react/hooks/types.ts
#	packages/appkit-ui/src/react/hooks/use-chart-data.ts
#	packages/appkit/src/plugins/analytics/analytics.ts
#	packages/appkit/src/plugins/analytics/types.ts
Restoring zod@4.3.6 to appkit and bumping shared's zod from 3.23.8 to
4.3.6 left the lockfile out of sync with package.json, breaking CI's
pnpm install --frozen-lockfile step on every job. Regenerate the
lockfile so both specifier entries match the manifests.

Signed-off-by: James Broadhead <jamesbroadhead@gmail.com>
The merge with main left two separate import statements for "./types" —
one for the type-only specifiers and a duplicate value import of
normalizeAnalyticsFormat. Biome rejected this as both an
organize-imports failure and a noRedeclare error. Merge them into a
single mixed type/value import.

Signed-off-by: James Broadhead <jamesbroadhead@gmail.com>
The merge resolution in client.ts dropped the logger.error call from
the executeStatement catch block — main has it, our pre-merge branch
had it, the resolved version lost it. Without that line the
"error log redaction" tests fail because the connector no longer
surfaces the failure message to the log spy.

Restore the call. Test plan: the two sql-warehouse.test.ts redaction
tests pass locally; behavior matches the comment "executeStatement's
catch ... is the single point that logs (gated on isAborted)".

Signed-off-by: James Broadhead <jamesbroadhead@gmail.com>
…e SSE message

Address Mario's design feedback: SSE is for short control messages, not
bulk binary. Inline Arrow IPC payloads from serverless warehouses no
longer ride the SSE channel as base64; they are stashed server-side and
fetched out-of-band through the existing /arrow-result/:jobId endpoint
with the canonical application/vnd.apache.arrow.stream content-type.

Wire protocol
- Discriminated union shrinks from three variants to two: the
  arrow_inline message type is gone. Both INLINE and EXTERNAL_LINKS
  ARROW_STREAM responses now flow as a single `arrow` message whose
  statement_id discriminates dispatch: warehouse-issued ids hit the
  warehouse path, synthetic "inline-<uuid>" ids hit the stash. The
  client sees one path.

Server
- New InlineArrowStash: TTL'd (10 min), bounded-memory (256 MiB),
  drain-on-read, per-user-keyed map of decoded Arrow IPC bytes. Stash
  key is the request's user id (or "global" for SP contexts) and is
  symmetric between put and take.
- AnalyticsPlugin holds one stash instance and uses it in two places:
  - _executeWithFormatFallback decodes result.attachment once, puts the
    bytes in the stash, and emits an arrow message with the synthetic
    id. Bulk bytes never traverse SSE.
  - _handleArrowRoute prefix-dispatches on the jobId: "inline-" drains
    the stash and serves with application/vnd.apache.arrow.stream + a
    no-store cache header; other ids fall through to the existing
    warehouse-fetch path unchanged.
- Connector's MAX_INLINE_ATTACHMENT_BYTES raised from 8 MiB to 25 MiB
  (the Databricks API hard cap on INLINE) since the SSE event-size
  budget no longer constrains it.

Client
- useAnalyticsQuery loses the arrow_inline branch and the local base64
  decoder. Both inline and external-links responses fetch through
  /api/analytics/arrow-result/:id; the prefix branch lives server-side.
- The dead client-side MAX_INLINE_ATTACHMENT_BYTES guard goes away.

SSE buffers
- streamDefaults.maxEventSize: 12 MiB -> 1 MiB
- connectSSE.maxBufferSize:    12 MiB -> 1 MiB
SSE now carries only short JSON control messages (result rows, arrow
envelope with statement id, error frames). Multi-MiB caps are no longer
needed and would mask buffer regressions.

Tests
- New InlineArrowStash unit tests (TTL eviction, max-bytes LRU, drain-
  on-read, per-user scoping).
- Reworked the route's "emits arrow_inline" test into a stash + arrow-
  message assertion: the SSE payload must not contain the base64 bytes
  or the arrow_inline type literal, and the decoded bytes must be in
  the stash keyed by the same synthetic id.
- New /arrow-result tests cover the inline path: success drain, 410 on
  unknown id, 410 on user mismatch.
- Client tests rewritten to assert both warehouse and inline-prefixed
  ids fetch through the same /arrow-result URL with no local decoding.
- Shared schema tests assert the retired arrow_inline type no longer
  parses.
- The /arrow-result content-type for warehouse hits stays application/
  octet-stream (no behavior change there).

Signed-off-by: James Broadhead <jamesbroadhead@gmail.com>
Four findings surfaced by the GPT pass on the reworked PR:

1. ARROW_STREAM cache replay returned drained inline-* ids (HIGH).
   The previous code capped the cache TTL at 10 min for ARROW_STREAM,
   which made sense for EXTERNAL_LINKS pre-signed URLs that expire in
   ~15 min but is broken for inline ids: the stash drains on the first
   /arrow-result fetch, so any cache hit replays an id whose bytes are
   gone and reliably 410s. Bypass cache entirely for ARROW_STREAM (TTL
   = 0); JSON_ARRAY responses still cache normally.

2. Stash evict-on-fit invalidated already-issued ids (MEDIUM).
   The earlier `evictUntilFits` dropped the oldest entries when a new
   payload would push total bytes past `maxBytes`, but those oldest
   entries had ids that were already in flight to clients. Replace
   eviction with rejection: `put()` now returns `string | null` and the
   caller falls back to EXTERNAL_LINKS when the stash is full. Every id
   we hand out stays valid until naturally drained or expired.

3. Aborted stream still decoded + stashed (MEDIUM).
   If the client cancels the SSE between query completion and stash
   write, we still decoded the base64 attachment and held the bytes
   until TTL eviction. Re-check `signal.aborted` before decode/put so
   canceled streams exit cleanly.

4. Empty result message wrote `undefined` to the hook's state (LOW).
   The wire schema makes `data` optional; an empty result set may omit
   it. Normalize the missing case to `[]` so consumers can rely on
   `data` being either `null` (no message yet) or a value of the
   inferred result type.

Also documents the process-local-memory constraint on the stash in its
docstring: a `GET /arrow-result/inline-*` that lands on a different
replica than the original SSE request will 410. Multi-replica
deployments need sticky sessions or a shared external store, neither in
scope for this PR.

Tests:
- `inline-arrow-stash`: replaced the eviction test with a rejection
  test that asserts `put()` returns null when the stash is full and
  that previously-issued ids remain takeable.
- `useAnalyticsQuery`: new test asserts an empty result message
  normalizes to [].

Signed-off-by: James Broadhead <jamesbroadhead@gmail.com>
Signed-off-by: James Broadhead <jamesbroadhead@gmail.com>
- agents.ts had two unused imports that biome's noUnusedImports rule
  flags as errors in CI. Drop them; behavior unchanged.
- inline-arrow-stash.test.ts: introduce a mustPut() helper that asserts
  the non-null contract for successful puts, so the new
  `put(): string | null` return type does not poison every downstream
  take() call with a string-vs-string|null TS error.
- Minor formatter touch-ups picked up by biome --write.

Signed-off-by: James Broadhead <jamesbroadhead@gmail.com>
Resolves conflict in
packages/appkit-ui/src/react/hooks/__tests__/use-analytics-query.test.ts
by combining the PR's arrow-IPC SSE message tests with the
useStableParams refetch tests from #321 (now on main). The merged
file uses a single connectSSE spy that both captures the onMessage
handler (for the arrow tests) and counts invocations (for the
stable-params tests). All 10 tests pass.

Signed-off-by: James Broadhead <jamesbroadhead@gmail.com>
When the inline Arrow stash refuses a new entry (put returns null),
the route must retry the statement with EXTERNAL_LINKS instead of
emitting a useless inline- id. The stash itself was unit-tested
already; this adds the integration test through the /query route.

Signed-off-by: James Broadhead <jamesbroadhead@gmail.com>
…arehouses

Some serverless warehouse variants only support ARROW_STREAM for the
INLINE disposition — JSON_ARRAY + INLINE is rejected with 'Inline
disposition only supports ARROW_STREAM format.' Before this change,
every default useAnalyticsQuery call against such a warehouse failed.

The plugin now classifies inline rejections into 'needs-arrow' vs
'needs-json' signals. For a JSON_ARRAY caller hitting needs-arrow, the
plugin retries as ARROW_STREAM + INLINE and decodes the Arrow IPC
attachment back to plain row objects server-side, keeping the caller's
JSON_ARRAY contract intact (scalar values stringified to match the
warehouse's native JSON_ARRAY shape).

The existing ARROW_STREAM + INLINE → EXTERNAL_LINKS path now uses the
same classifier with the 'needs-json' signal. Matching is
case-insensitive and handles the real warehouse error wordings rather
than the case-sensitive 'INLINE' + 'ARROW_STREAM' substring pair the
old heuristic required, which never matched the actual wire errors.

Verified live against three e2-dogfood warehouses: one that refuses
JSON_ARRAY + INLINE, one other serverless, and one classic — all three
now produce identical JSON row output for the same SELECT.

Signed-off-by: James Broadhead <jamesbroadhead@gmail.com>
@MarioCadenas
Copy link
Copy Markdown
Collaborator

Some agentic review @jamesbroadhead Please take a look

Xavier Review (v3 @ 0f5022fb) — request changes

Three reviewer personas (correctness, security, performance) all returned request changes. Findings re-verified against the live worktree at PR head 0f5022fb. The critical stash-full double-execution issue from the v2 review (2026-05-13) persists into v3 and is now pinned by an explicit test.

Reviewer Verdict Findings
correctness request changes 5 (1 high, 3 medium, 1 low)
security request changes 8 (1 high, 5 medium, 2 low)
performance request changes 8 (1 critical, 3 high, 2 medium, 2 low)

Blockers

🔴 [critical] Stash-full path doubles warehouse work AND can return a divergent result — pinned by test as intentional

File: packages/appkit/src/plugins/analytics/analytics.ts:391-396, 408-425, 452-458
Test asserting current behavior: packages/appkit/src/plugins/analytics/tests/analytics.test.ts:1074-1081

Flagged by all three reviewers. In the cap-overflow path:

  1. INLINE + ARROW_STREAM executor.query completes and is billed (:391-396).
  2. Multi-MiB Buffer.from(attachment, "base64") runs synchronously on the Node event loop (:408).
  3. inlineArrowStash.put() returns null (:417) — the decoded bytes are thrown away.
  4. A second executor.query runs with EXTERNAL_LINKS (:452-458) — billed again, and for any non-deterministic SQL (CURRENT_TIMESTAMP, RAND, late-arriving rows) the second result can differ from the discarded first one.

The new test pins this as deliberate — so this needs an explicit product decision before merging.

Suggestion (any of):

  • Reserve stash capacity before executing.
  • Reuse the already-decoded buffer for this request via a one-shot sidestep endpoint.
  • Switch from reject-on-overflow to safe LRU eviction with in-flight refcount.
  • Route to EXTERNAL_LINKS directly when stash utilization is near cap.

If divergent-result + double-billing is the accepted tradeoff, please document it on the SSE/route contract and add a counter so it's monitorable.

🟠 [high] Warehouse / SDK error text still reflected verbatim to clients (RECURRING — 5 PRs deep: #310, #197, #255, #329 v1/v2/v3)

Files:

  • packages/appkit/src/errors/execution.ts:48-50Statement failed: ${errorMessage} body
  • packages/appkit/src/stream/stream-manager.ts:288-308 — SSE serializer forwards error.message verbatim
  • packages/appkit/src/plugins/analytics/analytics.ts:170-172/arrow-result warehouse-path 404 returns error.message (inline misses use a fixed string at :133-135)

Anyone reading the SSE stream or hitting /arrow-result for a missing warehouse id sees raw Databricks wording — statement fragments, object names, correlation IDs, internal paths. CWE-209.

v3 added structured errorCode propagation (good — already consumed by _classifyInlineRejection), but the human message still embeds raw upstream text.

Suggestion: Map upstream failures to a small set of stable, client-safe messages; keep full detail server-side only. UI should branch on errorCode, not message.

🟠 [high] JSON_ARRAY "needs-arrow" fallback may diverge from native JSON_ARRAY shape + heavy server-side materialization

File: packages/appkit/src/plugins/analytics/analytics.ts:373-382 (retry+decode), :591-625 (decodeArrowAttachmentToRows); shape comment at :607-620

Flagged by both correctness AND performance:

  • Correctness: warehouse-native JSON_ARRAY stringifies scalars and represents nested fields as JSON strings; the fallback path stringifies scalars but explicitly leaves nested values "as-is" (acknowledged by code comment at :607-620). Callers see different shapes depending on which path executed.
  • Performance: per-row Record build is O(rows × cols) CPU + allocation on the Node main thread. A 100k×50 fallback can cost hundreds of ms of blocking time + GC churn on top of the first failed INLINE attempt.

Suggestion: Align serialization with _transformDataArray / documented JSON_ARRAY semantics (e.g. JSON.stringify nested values), and cap row count for the fallback materializer with an explicit error past the cap.

🟠 [high] Client safeParse keeps O(rows × keys) Zod validation for large JSON results (RECURRING from v1/v2)

Files:

  • packages/shared/src/sse/analytics.ts:30data: z.array(z.record(z.string(), z.unknown())).optional()
  • packages/appkit-ui/src/react/hooks/use-analytics-query.ts:189AnalyticsSseMessage.safeParse runs on every SSE message after JSON.parse

10k–1M rows = hundreds of ms to seconds of main-thread TBT after JSON.parse.

Suggestion: Loose schema for result.data (z.array(z.unknown())), or branch on type === "result" before deep validation.

Other notable findings

  • [medium] JSON.parse failure still leaves useAnalyticsQuery stuck in loading (RECURRING from v1/v2) — outer catch at `packages/appkit-ui

Addresses Mario's v3 review on #329 — 1 critical + 3 high findings:

- 🔴 critical: stash-full double-execution + divergent result. The
  ARROW_STREAM INLINE fallback used to discard the already-decoded
  bytes when the stash was at cap and re-run the same statement on
  EXTERNAL_LINKS — that path billed the warehouse twice and could
  return a divergent result for non-deterministic SQL. The stash now
  has a second pool ("overflow") sized at `maxOverflowBytes` (default
  same as `maxBytes`). When the regular pool is full, decoded bytes
  spill into overflow rather than being thrown away. Only when both
  pools are at cap does the route surface `INLINE_ARROW_STASH_EXHAUSTED`
  — single execution, never silent double-billing. Memory is bounded
  above by `maxBytes + maxOverflowBytes`.

- 🟠 high: warehouse / SDK error text reflected verbatim to clients
  (CWE-209). Added `clientMessage` to AppKitError with a sanitized
  default per subclass, and a stable `errorCode` field on the SSE
  error payload. Stream-manager and the /arrow-result 410/404
  responses now emit `clientMessage` (sanitized) over the wire; raw
  upstream wording stays in server logs only. UI can branch on
  `errorCode` (e.g. `INLINE_ARROW_STASH_EXHAUSTED`) instead of
  substring-matching the human string.

- 🟠 high: JSON_ARRAY fallback shape divergence + heavy materialization.
  `decodeArrowAttachmentToRows` now `JSON.stringify`s nested values
  (List / Struct / Map) to match what the warehouse emits natively
  under JSON_ARRAY — apache-arrow's typed values expose `toJSON()`,
  so the output shape matches. Added a hard 100k-row cap on the
  fallback materializer; past the cap surfaces
  `RESULT_TOO_LARGE_FOR_JSON_FALLBACK` with guidance to re-issue
  with ARROW_STREAM, rather than blocking the Node event loop for
  hundreds of ms.

- 🟠 high: client `safeParse` O(rows × keys) Zod validation. Loosened
  `AnalyticsResultMessage.data` to `z.array(z.unknown())` — the
  per-row shape is already enforced at the source by the typed
  `makeResultMessage` builder, so the deep client-side validation
  was pure cost. TS-level type narrows back to
  `Record<string, unknown>[]` for callers.

Also a medium from the review:
- `JSON.parse` failure in `useAnalyticsQuery` no longer strands the
  hook in `loading=true` — the outer catch now clears loading and
  surfaces a user-facing error.

Co-authored-by: Isaac
Signed-off-by: James Broadhead <jamesbroadhead@gmail.com>
@jamesbroadhead
Copy link
Copy Markdown
Contributor Author

jamesbroadhead commented May 21, 2026

👋 Hi Mario — Claude here, replying on James's behalf.

Pushed 4afce1f6 addressing every blocker in your v3 review. Closed #390 as superseded — the overflow pool subsumes that PR's putWait half-measure, so there's no value in keeping it open.

🔴 critical — stash-full double-execution + divergent result

Fixed at the root, not papered over. InlineArrowStash now has a second pool ("overflow") sized at maxOverflowBytes (default same as maxBytes). When the regular pool is at cap, the already-decoded bytes spill into overflow rather than being thrown away. Memory is bounded above by maxBytes + maxOverflowBytes.

  • ✅ Single executor.query call — never re-runs on EXTERNAL_LINKS.
  • ✅ No discarded multi-MiB decode work.
  • ✅ No double-billing.
  • ✅ No divergent-result possibility (the bytes the client gets are the bytes the warehouse returned the first time).
  • ✅ Only when both pools hit cap does the route surface INLINE_ARROW_STASH_EXHAUSTED — clean error, never silent re-execution.

The pinning test … falls back to EXTERNAL_LINKS when the inline stash is full from v3 has been replaced with two tests:

  • … spills the already-decoded bytes into the stash overflow pool when the regular pool is full — single execution, no double-billing
  • … surfaces a stable error when both regular and overflow pools are exhausted — never silently double-bills

Files: packages/appkit/src/plugins/analytics/inline-arrow-stash.ts, analytics.ts:391-430, tests/inline-arrow-stash.test.ts, tests/analytics.test.ts.

🟠 high — raw warehouse / SDK error text reflected to clients (CWE-209)

Done. Added clientMessage to AppKitError (sanitized default per subclass) and a new errorCode field on the SSE error payload (SSEError interface). The stream-manager broadcaster now sends clientMessage over the wire — raw error.message stays in server logs only. Same treatment for the /arrow-result 410/404 paths. UI branches on errorCode (INLINE_ARROW_STASH_EXHAUSTED, NOT_IMPLEMENTED, etc.).

Test pin: should send a sanitized error event when handler throws — raw error text is kept server-side only asserts the raw Error.message (with a fake /internal/path + corrId=…) never appears anywhere on the SSE wire.

Files: packages/appkit/src/errors/base.ts, errors/execution.ts, stream/stream-manager.ts:287-340, stream/sse-writer.ts, stream/types.ts, plugins/analytics/analytics.ts:170-180.

🟠 high — JSON_ARRAY fallback shape + heavy materialization

Both legs fixed:

  • Shape: nested values (List / Struct / Map) now go through JSON.stringify(v). apache-arrow's typed Vector values expose toJSON(), so the output matches the warehouse-native JSON_ARRAY shape (nested-as-JSON-string). Previous comment about "round-tripping would mismatch" is no longer accurate now that we lean on toJSON().
  • Performance: added a hard JSON_ARRAY_FALLBACK_MAX_ROWS = 100_000 cap. Past the cap, the materializer throws RESULT_TOO_LARGE_FOR_JSON_FALLBACK with guidance to re-issue with format: "ARROW_STREAM" — instead of blocking the Node event loop for hundreds of ms.

File: packages/appkit/src/plugins/analytics/analytics.ts:591-660.

🟠 high — client safeParse O(rows × keys) Zod validation

Loosened AnalyticsResultMessage.data from z.array(z.record(z.string(), z.unknown())) to z.array(z.unknown()). The per-row shape is already enforced at the source by the typed makeResultMessage builder, so the deep client-side validation was pure cost. TS-level type narrows back to Record<string, unknown>[] for consumers via an Omit + intersection so call sites are unchanged.

File: packages/shared/src/sse/analytics.ts.

🟢 medium — JSON.parse failure stranded useAnalyticsQuery in loading=true

Fixed. The outer catch in the SSE message handler now sets loading=false and surfaces "Unable to load data, please try again" so consumers can render a retry affordance. New test pins the behavior.

Files: packages/appkit-ui/src/react/hooks/use-analytics-query.ts:260-269, __tests__/use-analytics-query.test.ts.


Truncated review tail: your comment ends mid-sentence at "outer catch at packages/appkit-ui" — looks like Xavier's output hit a size limit. The tally in the header table (correctness 5, security 8, performance 8 = 21 total) implies ~16 more medium/low findings beyond the blockers detailed above. Could you re-post the rest, or kick off a Xavier v4 run against 4afce1f` so the remaining items show up? Happy to roll a follow-up commit on top of this one as soon as I can see them.

Validation on this commit: full appkit suite (2,096 tests) + full appkit-ui suite (292 tests) green; tsc --noEmit clean on appkit, appkit-ui, shared; biome check clean on changed files.

Second iteration on #329 after running Xavier multi-model review against
4afce1f. Eleven findings (1 critical, 4 high, 6 medium), all addressed:

🔴 critical — `decodeArrowAttachmentToRows` regression: bare
   `JSON.stringify` on Arrow nested values throws on `bigint` (the spec
   doesn't serialize it) and produces broken shapes for `Uint8Array` /
   `Date`. Added `nestedJsonReplacer` that stringifies bigint, base64s
   `Uint8Array`, and ISO-strings `Date`. Top-level `Date` / `Uint8Array`
   columns get the same treatment in their own branches so we never
   reach the nested fallback for them.

🟠 high — single-payload throw threshold was `maxBytes +
   maxOverflowBytes`, but a payload can only land in *one* pool (pools
   do not split). Fixed to `Math.max(maxBytes, maxOverflowBytes)`.

🟠 high — overflow pool inherited the regular 10-min TTL, which kept
   transient bytes around for the full duration of the regular pool's
   lifetime and amplified cross-user memory pressure in multi-tenant
   deployments. Split into a separate `overflowTtlMs` (default 30s) —
   overflow exists only to bridge the immediate `/arrow-result` fetch.

🟠 high — overflow defaulted to the same size as `maxBytes`, doubling
   the worst-case memory headroom. Dropped default to `maxBytes / 4`;
   overflow only needs to absorb transient spillover, not hold long-term
   state. Combined with the shorter TTL, the practical memory delta vs
   pre-overflow code is small.

🟠 high — JSON_ARRAY fallback row cap ignored cell size (10 rows × 10MB
   cells = OOM under the 100k-row cap). Added a 64MB byte cap on the
   decoded attachment, checked before `tableFromIPC` so we never even
   allocate the table for an oversize payload.

🟢 medium — `gc()` ran O(N) on every `put`. Throttled to
   `gcMinIntervalMs` (default 5s). Tests that drive the clock past TTL
   on a sub-throttle scale opt out via `gcMinIntervalMs: 0`.

🟢 medium — `table.getChild(name)` was being called for every cell,
   walking the schema fields on every lookup. Hoisted into a
   `[name, vector]` array resolved once before the row loop. Real win
   at 100k × 50 columns.

🟢 medium — `useAnalyticsQuery` dropped the new structured `errorCode`
   from the SSE error payload, so consumers could not actually branch
   on the stable identifier the previous commit added. Added
   `errorCode: string | null` to `UseAnalyticsQueryResult` and plumbed
   it through from `parsed.errorCode`.

🟢 medium — outer `catch` in the SSE handler used to clear loading on a
   parse failure but leave the stream open, re-firing the catch on every
   subsequent malformed message. Now also calls
   `abortController.abort()` so the consumer can retry cleanly.

Validation: 2,097 appkit + 292 appkit-ui tests pass; tsc clean on
appkit / appkit-ui / shared; biome clean on changed files.

Co-authored-by: Isaac
Signed-off-by: James Broadhead <jamesbroadhead@gmail.com>
@jamesbroadhead
Copy link
Copy Markdown
Contributor Author

Self-review (Xavier multi-model: GPT-5.4 + Gemini-3.1) on 4afce1f6 surfaced 11 follow-ups against my own commit. All addressed in 28fdb928.

# Severity Finding Fix
1 🔴 critical decodeArrowAttachmentToRows regression: bare JSON.stringify throws on bigint and emits broken shapes for Uint8Array / Date New nestedJsonReplacer (bigint → string, Uint8Array → base64, Date → ISO); top-level Date / Uint8Array handled explicitly so they never reach the nested fallback
2 🟠 high Single-payload throw used maxBytes + maxOverflowBytes, but a payload only lands in one pool Threshold is now Math.max(maxBytes, maxOverflowBytes)
3 🟠 high Overflow inherited the 10-min regular TTL — long-lived cross-user memory pressure Separate overflowTtlMs (default 30s); overflow exists only to bridge the immediate /arrow-result fetch
4 🟠 high Overflow defaulted to maxBytes, doubling worst-case memory headroom Default dropped to maxBytes / 4
5 🟠 high JSON_ARRAY fallback row cap ignored cell size (10 × 10MB rows = OOM) Added 64MB byte cap on the decoded attachment, checked before tableFromIPC
6 🟢 medium gc() ran O(N) on every put Throttled to gcMinIntervalMs (default 5s); tests opt out with 0
7 🟢 medium table.getChild(name) walked schema for every cell Hoisted into [name, vector] pairs once before the row loop
8 🟢 medium Hook dropped the new structured errorCode from SSE error payloads Added errorCode: string | null to UseAnalyticsQueryResult; populated from parsed.errorCode
9 🟢 medium Outer SSE catch cleared loading but left the stream open Now also calls abortController.abort()

Validation: 2,097 appkit + 292 appkit-ui tests pass; tsc clean on appkit / appkit-ui / shared; biome clean on changed files.

Three follow-ups from self-review on the inline-Arrow path:

- **Telemetry**: `InlineArrowStash.put()` now returns `{ id, pool }` so
  callers can label outcomes without re-introspecting the stash. The
  analytics plugin emits two instruments (lazy-init since `this.telemetry`
  isn't bound at construct time):
    - `analytics.inline_arrow_stash.put.count{result}` — counter with
      label "regular" | "overflow" | "exhausted"
    - `analytics.inline_arrow_stash.put.bytes{result}` — histogram of
      accepted payload sizes
  Operators can now drive capacity-tuning dashboards off `result="overflow"`
  spill rate and `result="exhausted"` rejection rate without inferring
  state from response codes.

- **Type cleanup**: `AnalyticsResultMessage` had a Zod-inferred type
  wrapped in `Omit<…, "data"> & { data?: ... }` to narrow `data` to
  `Record<string, unknown>[]` for callers. Replaced with a hand-written
  interface alongside the Zod schema, with an explicit "keep in sync"
  comment. Same runtime behavior, dramatically easier to read.

- **Format-path extraction**: `_executeWithFormatFallback` was a 120-line
  function handling two distinct format paths inline. Split into:
    - `_executeJsonArrayPath` — JSON_ARRAY with ARROW_STREAM retry
    - `_executeArrowStreamPath` — ARROW_STREAM with EXTERNAL_LINKS fallback
    - `_stashAndEmitInline` — the cap/overflow/exhaustion logic that both
      paths share, pulled into one named method
  `_executeWithFormatFallback` is now a thin dispatcher. Each method's
  failure modes are documented in its docstring so the contract is
  visible without reading the body.

Validation: 2,098 appkit + 292 appkit-ui tests pass; tsc clean on
appkit / appkit-ui / shared; biome clean on changed files.

Co-authored-by: Isaac
Signed-off-by: James Broadhead <jamesbroadhead@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Analytics plugin incompatible with ARROW_STREAM-only warehouses

2 participants