feat: decode inline Arrow IPC + warehouse-compat fallback#329
Open
jamesbroadhead wants to merge 31 commits into
Open
feat: decode inline Arrow IPC + warehouse-compat fallback#329jamesbroadhead wants to merge 31 commits into
jamesbroadhead wants to merge 31 commits into
Conversation
3 tasks
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
force-pushed
the
stack/arrow-3-inline-arrow-fix
branch
from
ca69f8e to
08c5486
Compare
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>
Collaborator
|
Some agentic review @jamesbroadhead Please take a look Xavier Review (v3 @
|
| 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:
- INLINE + ARROW_STREAM
executor.querycompletes and is billed (:391-396). - Multi-MiB
Buffer.from(attachment, "base64")runs synchronously on the Node event loop (:408). inlineArrowStash.put()returnsnull(:417) — the decoded bytes are thrown away.- A second
executor.queryruns withEXTERNAL_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_LINKSdirectly 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-50—Statement failed: ${errorMessage}bodypackages/appkit/src/stream/stream-manager.ts:288-308— SSE serializer forwardserror.messageverbatimpackages/appkit/src/plugins/analytics/analytics.ts:170-172—/arrow-resultwarehouse-path 404 returnserror.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
Recordbuild 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:30—data: z.array(z.record(z.string(), z.unknown())).optional()packages/appkit-ui/src/react/hooks/use-analytics-query.ts:189—AnalyticsSseMessage.safeParseruns on every SSE message afterJSON.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.parsefailure still leavesuseAnalyticsQuerystuck inloading(RECURRING from v1/v2) — outer catch at `packages/appkit-ui
10 tasks
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>
Contributor
Author
|
👋 Hi Mario — Claude here, replying on James's behalf. Pushed 🔴 critical — stash-full double-execution + divergent resultFixed at the root, not papered over.
The pinning test
Files: 🟠 high — raw warehouse / SDK error text reflected to clients (CWE-209)Done. Added Test pin: Files: 🟠 high — JSON_ARRAY fallback shape + heavy materializationBoth legs fixed:
File: 🟠 high — client
|
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>
Contributor
Author
|
Self-review (Xavier multi-model: GPT-5.4 + Gemini-3.1) on
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>
4 tasks
Contributor
Author
|
Claude here, on James's behalf. Quick status update since this has been quiet:
Mario's OOO. @pkosiec — would you be able to take a look when you get a chance? You've got the most context on this part of the package recently. Original review thread + Xavier findings are already in the comments above so no need to repost; main thing is whether the v3 blocker fixes look right to you. Thanks! |
Resolves 8 conflicts integrating main's warehouse-readiness streaming
with this branch's inline-Arrow format fallback.
decodeArrowAttachmentToRows (JSON_ARRAY fallback) fixes so decoded rows
match the warehouse's native JSON_ARRAY rendering:
- DECIMAL: apply scale ("123.45", not unscaled "12345")
- TIMESTAMP/DATE: ISO-8601 via Date#toISOString (drop Z for NTZ)
- STRING-as-JSON: parse to match _transformDataArray
- ARROW_STREAM with no attachment: fall through to EXTERNAL_LINKS
Also preserve errorCode/clientMessage through execute() in the route.
Validated e2e against a dogfood serverless warehouse: JSON_ARRAY
canonical + decode-fallback and ARROW_STREAM inline-stash paths all
match native rendering.
Co-authored-by: Isaac
Signed-off-by: MarioCadenas <MarioCadenas@users.noreply.github.com>
Signed-off-by: MarioCadenas <MarioCadenas@users.noreply.github.com> # Conflicts: # packages/appkit/src/type-generator/query-registry.ts # packages/appkit/src/type-generator/types.ts
Contributor
📦 Bundle size reportCompared against
|
| dist | raw | gzip |
|---|---|---|
| JS (runtime) | 727 KB (-5.6 KB) | 254 KB (-2.5 KB) |
| Type declarations | 275 KB (-6.3 KB) | 94 KB (-2.7 KB) |
| Source maps | 1.4 MB (-15 KB) | 473 KB (-5.8 KB) |
| Other | 11 KB | 3.7 KB |
| Total | 2.4 MB (-27 KB) | 825 KB (-11 KB) |
Per-entry composition (own code — deps external (as shipped))
| Entry | Initial (gz) | Lazy (gz) | Total (gz) | node_modules (min) | Own code (min) |
|---|---|---|---|---|---|
. |
79 KB | 2.5 KB | 81 KB | external | 258 KB |
./beta |
39 KB | 231 B | 39 KB | external | 117 KB |
./type-generator |
19 KB | 0 B | 19 KB | external | 54 KB |
Chunks:
| Entry | Chunk | Load | Size (gz) |
|---|---|---|---|
. |
index.js |
initial | 75 KB |
. |
utils.js |
initial | 4.0 KB |
. |
remote-tunnel-manager.js |
lazy | 2.5 KB |
./beta |
beta.js |
initial | 30 KB |
./beta |
databricks.js |
initial | 5.7 KB |
./beta |
service-context.js |
initial | 3.2 KB |
./beta |
client-options.js |
initial | 219 B |
./beta |
databricks.js |
lazy | 128 B |
./beta |
index.js |
lazy | 103 B |
./type-generator |
index.js |
initial | 19 KB |
@databricks/appkit-ui
npm tarball (packed): 295 KB (-11 KB) — gzipped download (dist + bin; excludes release-only docs/NOTICE).
| dist | raw | gzip |
|---|---|---|
| JS (runtime) | 350 KB (-19 KB) | 116 KB (-6.4 KB) |
| Type declarations | 201 KB (-4.2 KB) | 72 KB (-1.3 KB) |
| Source maps | 669 KB (-21 KB) | 218 KB (-6.8 KB) |
| CSS | 16 KB | 3.3 KB |
| Total | 1.2 MB (-43 KB) | 410 KB (-14 KB) |
Per-entry composition (consumer bundle — deps bundled, peerDeps external)
| Entry | Initial (gz) | Lazy (gz) | Total (gz) | node_modules (min) | Own code (min) |
|---|---|---|---|---|---|
./js |
4.3 KB | 49 KB | 54 KB | 208 KB | 12 KB |
./js/beta |
20 B | 0 B | 20 B | 0 B | 0 B |
./react |
428 KB | 49 KB | 476 KB | 1.3 MB | 163 KB |
./react/beta |
20 B | 0 B | 20 B | 0 B | 0 B |
Chunks:
| Entry | Chunk | Load | Size (gz) |
|---|---|---|---|
./js |
index.js |
initial | 4.2 KB |
./js |
chunk |
initial | 120 B |
./js |
apache-arrow |
lazy | 49 KB |
./js/beta |
beta.js |
initial | 20 B |
./react |
index.js |
initial | 426 KB |
./react |
tslib |
initial | 2.1 KB |
./react |
apache-arrow |
lazy | 49 KB |
./react/beta |
beta.js |
initial | 20 B |
…ry (#472) * refactor(appkit): direct Arrow IPC streaming + hardened result delivery Rework of the Arrow/JSON result-delivery path on top of #329's arrow foundation (base branch is #329 merged with current main). Delivery: - ARROW_STREAM queries stream the raw Arrow IPC bytes on the /query response body — no SSE, no server-side stash, no second /arrow-result request. The inline stash was process-local (broke horizontal scaling) and buffered whole payloads; it and its route are removed. - Capability fallback centralized in result-delivery.ts (errorCode-first classification): INLINE+ARROW_STREAM (Reyden) -> EXTERNAL_LINKS -> a structured ARROW_DELIVERY_UNSUPPORTED error; the JSON path retries as ARROW_STREAM and decodes server-side when the warehouse is inline-only. - EXTERNAL_LINKS chunks pipe response.body one network read at a time (peak memory = one read buffer). Multi-chunk results resolve every chunk's links via next_chunk_index in the request's identity context (OBO-correct) — previously only chunk 0 was streamed (silent truncation). - Real column names ride an X-Appkit-Arrow-Columns header (statement-id ref + /columns endpoint for wide schemas); the client relabels the positional col_N schema. Hardening (pre-merge review): - writeChunk no longer hangs on a client disconnect mid-backpressure — it races drain vs close/error so the stream unwinds and the upstream reader is cancelled instead of leaking. - Fail-fast first-byte timeout returns 503 WAREHOUSE_UNAVAILABLE on a stuck warehouse. - /columns resolves under the user identity then the service principal (fixes OBO wide-schema fallback). - Retry backoff is abortable; arrow errors forward the actionable clientMessage. Green: typecheck, 3220 tests + 1 skipped, biome, build + publint. Multi-chunk external, OBO+external, and wide-schema /columns are unit-tested only (no warm standard warehouse to live-verify). * fix(appkit): idle-timeout backpressure, OBO warehouse identity, writeChunk close guard Addresses three defects surfaced in external review of the Arrow rework: - Idle-timeout vs backpressure (arrow-stream-processor.ts): the per-chunk idle timeout was armed before reader.read() but stayed armed across the downstream `yield`, so a slow client backpressuring writeChunk past the timeout (60s) was mistaken for an upstream stall and aborted a healthy download. The timeout now covers only the read (armed before, cleared in a finally the instant it resolves). - OBO warehouse startup ran as the service principal (analytics.ts): the direct-binary Arrow path called getWorkspaceClient() + ensureWarehouseRunning bare in the handler body, so warehouse auto-start ran as the SP even for .obo.sql requests — unlike the SSE path, which runs readiness inside the executor (user) context. Readiness now runs via the request executor (asUser for OBO) through _ensureArrowWarehouseReady, matching the SSE path. - writeChunk hang on an already-closed socket (analytics.ts): if res was already destroyed/ended when writeChunk was entered, res.write returned false while drain/close/error had already fired, so the promise never settled and wedged the for-await loop + upstream reader. It now rejects up front when res.destroyed || res.writableEnded. Regression tests added: slow-consumer-between-reads (no false abort), already-closed writeChunk, and OBO readiness routed through the user executor. Green: typecheck, 3222 tests + 1 skipped, biome, build + publint. Signed-off-by: MarioCadenas <MarioCadenas@users.noreply.github.com> --------- Signed-off-by: MarioCadenas <MarioCadenas@users.noreply.github.com> Co-authored-by: MarioCadenas <MarioCadenas@users.noreply.github.com>
…charts The direct-streaming rework made `format="arrow"` charts fetch raw Arrow IPC bytes over HTTP (fetchArrowDirect), but the Playwright mock (setupMockAPI) only ever returned an SSE/JSON response. Arrow-format charts therefore decoded SSE text as Arrow, failed, and rendered empty — dropping the `bar-chart-apps_list` count below 3 and failing arrow-analytics.spec.ts "charts render with mock data (no empty states)". setupMockAPI now detects `format: "ARROW_STREAM"` in the request body and responds with real Arrow IPC bytes (tableFromJSON + tableToIPC, Content-Type application/vnd.apache.arrow.stream) built from the same mock data; array/object cells are stringified so Arrow can infer primitive column types (chart columns like name/totalSpend are unaffected). The JSON/SSE path is unchanged. Verified with the CI env (APPKIT_E2E_TEST, DATABRICKS_*=e2e-mock): arrow-analytics 4/4 and the full integration suite 24/24 pass. Signed-off-by: MarioCadenas <MarioCadenas@users.noreply.github.com>
useAnalyticsQuery validated SSE messages at runtime with `AnalyticsSseMessage.safeParse` (a zod schema value), pulling the entire zod library (~60 KB gz / ~303 KB min) into the `./react` initial bundle — a regression this branch introduced (main validates with a type-only import and no zod). The `shared` sideEffects hint removed zod from `./js`, but it can't help `./react` because the schema value is genuinely called. The SSE wire schema is intentionally loose (`data` is an optional array of unknown), so a structural check suffices — no schema validator needed to read our own same-origin server's messages. Replaced the `.safeParse` with a plain `parsed.type === "result"` check (missing/non-array data normalizes to []), matching main. Verified via esbuild metafile: zod is now 0 in the initial bundle of both ./react and ./js; apache-arrow stays lazy (197 KB). Typecheck, 16 hook tests, and biome pass. Signed-off-by: MarioCadenas <MarioCadenas@users.noreply.github.com>
…refresh Robustness pass on the arrow result-delivery path from a full-feature review. - Cache INLINE arrow like the JSON path: the INLINE+ARROW_STREAM attempt now runs through a caching executor (per-user TTL cache + retry), so an arrow chart isn't a fresh warehouse execution on every render. Only bounded (<=25 MiB) inline attachments are cached; EXTERNAL_LINKS carry expiring pre-signed URLs and are never cached. Uses cache.getOrExecute directly so the thrown errorCode the capability fallback classifies on survives. - Memoize warehouse arrow capability (inline vs external) per warehouse id, in process: a standard warehouse rejects INLINE arrow on every query, so after the first we skip that doomed probe and go straight to EXTERNAL_LINKS. - Re-mint expired chunk links mid-download: EXTERNAL_LINKS pre-signed URLs expire in <=15 min, so a large/slow result can outlast its earliest links. The streamer now re-resolves a chunk's link via getStatementResultChunkN (bound to the caller's identity context) before retrying a failed fetch, instead of retrying the same expired URL. - Loosen fallback classification: any capability-CODED rejection (INVALID_PARAMETER_VALUE / NOT_IMPLEMENTED) of INLINE+ARROW now falls back to EXTERNAL_LINKS, rather than requiring an exact message phrase — a Databricks message reword no longer 500s every standard warehouse. - Zero-row external_links ([]): treat an empty array as a zero-row result and synthesize an empty Arrow table, instead of routing to streamChunks([]) which rejected with a 500. - Quiet routine client disconnects: an aborted signal now tears down without an ERROR log whether it fires before or after headers (checked before the headers-sent branch), so unmounts/param-changes don't spam alerting. Fail-fast 503 ordering preserved. - e2e mock now mirrors a real warehouse: positional col_N schema + real names in the X-Appkit-Arrow-Columns header, so the client relabel path (the live bug) is exercised end-to-end rather than only unit-tested. Green: pnpm -r typecheck, full unit suite (3421 pass / 1 skip), biome, build + publint, playground e2e (arrow-analytics 4/4, full integration 24/24). Multi- chunk external, OBO+external, and wide-schema paths remain unit-tested only (no warm standard warehouse to live-verify). Signed-off-by: MarioCadenas <MarioCadenas@users.noreply.github.com>
The executeStatement and _pollForStatementResult catch blocks carried a verbatim-identical error-rethrow tail (AbortError passthrough, AppKitError passthrough, and SDK-errorCode-preserving ExecutionError wrap). Extract it into a private `_rethrowStatementError(error): never` helper. The `never` return type keeps control flow identical — both copies were the last statement in their catch. Behavior-preserving. Signed-off-by: MarioCadenas <MarioCadenas@users.noreply.github.com>
The +7.2% appkit tarball growth is the PR's own new source (arrow-schema.ts, result-delivery.ts, expanded analytics/client). apache-arrow is external and not in the tarball, so the increase is justified own-code — acknowledge it by updating the committed baseline. Signed-off-by: MarioCadenas <MarioCadenas@users.noreply.github.com>
Resolves the bundle-size-baseline.json conflict by regenerating the baseline from the merged tree (v0.44.0 recharts drop + this PR's inline-arrow source), so the committed baseline reflects the actual shipped dist. tsc clean on appkit + appkit-ui; size:compare reports no change. Signed-off-by: MarioCadenas <MarioCadenas@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Some warehouses return
ARROW_STREAM+INLINEresults as base64 Arrow IPC inresult.attachmentrather thanresult.data_array. Some refuseJSON_ARRAY+INLINEentirely and only supportARROW_STREAMforINLINE. Others (most classic + some serverless) do the opposite — they refuseARROW_STREAM+INLINEand requireEXTERNAL_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:
ARROW_STREAM,JSON_ARRAYcaller always gets JSON rows; anARROW_STREAMcaller always gets Arrow bytes), regardless of which disposition the warehouse actually accepted.Design
ARROW_STREAMrequests are handled by_handleArrowStreamQuery(plugins/analytics/analytics.ts) and stream the raw Arrow IPC bytes directly as the HTTP POST response body — no SSE, no server-side stash, no second request:executeStatement;deliverArrowBytes(plugins/analytics/result-delivery.ts) decodes it once and yields a zero-copyUint8Arrayview over the Buffer straight to the response. Content-type is the real binaryapplication/vnd.apache.arrow.stream.writeChunk.executor.queryruns per disposition — an INLINE rejection falls back to exactly one EXTERNAL_LINKS execution, never a redundant re-run of the same disposition.JSON_ARRAYrequests keep the existing SSE path (it also carries warehouse-readiness progress and cached rows). Column names for very wide Arrow schemas that don't fit in theX-Appkit-Arrow-Columnsresponse header are re-derived statelessly viaGET /columns/:statementId(no server cache).Wins from the direct-stream redesign vs. the earlier stash approach:
/arrow-resultround triparrow_inlinewire message is gone — the discriminated union rejects it as schema-invalidDisposition fallback
The plugin classifies an inline rejection (
classifyDispositionRejection/isCapabilityRejectioninresult-delivery.ts) into one of two signals. Classification is gated on a structurederrorCode(INVALID_PARAMETER_VALUE/NOT_IMPLEMENTED, or those substrings) plus a case-insensitive mention of "inline" — unrelated errors (auth, SQL, permission) are never reinterpreted as disposition mismatches and are re-thrown unchanged:needs-arrow-inline: warehouse only acceptsARROW_STREAMfor INLINE. If the caller asked forJSON_ARRAY,deliverJsonResultretries asARROW_STREAM + INLINEand decodes the Arrow IPC attachment to plain row objects server-side (decodeArrowAttachmentToRows) — the caller's JSON contract is preserved. Scalar values are stringified to match the warehouse's native JSON_ARRAY shape.external-links-unsupported/ needs-external: warehouse refusesARROW_STREAM + INLINE. AnARROW_STREAMcaller falls back toARROW_STREAM + EXTERNAL_LINKS.Per-warehouse capability is memoized on the Arrow path (
capabilityHint/onCapabilityResolved) so a known-external warehouse skips the doomed INLINE probe on subsequent queries. 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).Safety caps
Buffer.from(connectors/sql-warehouse/client.ts).tableFromIPC, then a 100k row cap before the row loop — both precede materialization so a large result can't block the event loop; past the cap it throws with guidance to re-issue asARROW_STREAM.MAX_EXTERNAL_CHUNK_FOLLOWS.Error sanitization
Raw warehouse/SDK error text no longer reaches clients.
AppKitError.clientMessagereturns a sanitized per-subclass default; the SSE serializer and the Arrow route sendclientMessage+ a structurederrorCode, while rawerror.messagestays in server logs only. The UI branches onerrorCode, not on message wording.Changes
connectors/sql-warehouse/arrow-schema.ts+client.ts): detectsresult.attachmentand decodes viaapache-arrow'stableFromIPC.plugins/analytics/result-delivery.ts,analytics.ts):deliverArrowBytes/deliverJsonResult/_handleArrowStreamQuerystream bytes on the response body and implement the bidirectional disposition fallback.decodeArrowAttachmentToRows): when retrying withARROW_STREAM, decode and stringify so the JSON caller still gets row objects.shared/src/sse/analytics.ts): single source of truth between server and client for the JSON path. Default format remainsJSON_ARRAY. Client-side deep per-row validation was removed from the browser hot path (zod is no longer bundled inappkit-ui).GET /columns/:statementId): stateless re-derivation for wide Arrow schemas.Tests
connectors/sql-warehouse/tests/arrow-schema.test.ts(new)connectors/sql-warehouse/tests/client.test.ts(new) — real base64 Arrow IPC captured from a live serverless warehouseplugins/analytics/tests/result-delivery.test.ts(new) — single-execution per disposition, both fallback directions, cap enforcementplugins/analytics/tests/analytics.test.ts— integration coverage for both fallback directions plus the no-fallback safety netplugins/analytics/tests/arrow-delivery.integration.test.ts(new)appkit-ui/src/react/hooks/__tests__/use-analytics-query.test.ts— arrow-stream + malformed-frame (loading cleared, stream aborted) testsshared/src/sse/analytics.test.ts(new)Test plan
Verified via automated tests on the current head:
ARROW_STREAMINLINE: attachment decoded and streamed on the response bodyARROW_STREAMINLINE → EXTERNAL_LINKS fallback when the warehouse rejects INLINEJSON_ARRAYdirect path: warehouse accepts INLINE + JSON_ARRAY → row data returned unchangedJSON_ARRAYfallback path: warehouse refuses INLINE + JSON_ARRAY but accepts INLINE + ARROW_STREAM → server decodes Arrow attachment to JSON rowsJSON_ARRAYno-fallback safety: rejection without a needs-arrow signal (or without the errorCode gate) does NOT trigger a retryloadingand aborts the stream (no strandedloading=true)arrow_inlineSSE message is rejected as schema-invalidStack
This was the third PR in a stack; both predecessors are on main:
test: backfill coverage for genie connector, service context, stream registry #327 — coverage backfill(merged)feat: rename AnalyticsFormat to API enum names with legacy aliases #328 —(merged)AnalyticsFormatrename to API enum namesFixes #242. Replaces #256.
This pull request was AI-assisted by Isaac.