-
Notifications
You must be signed in to change notification settings - Fork 0
Points feature filter (MVP step 2) + responsive off-thread loads #81
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
be99d92
d8b155e
6b75923
a0f52ea
3a30d3c
da33a61
6791559
54239ad
5d05f78
4032b1e
02864df
a5cf0d0
659ba5f
8d76252
49d71af
701ccba
b724e29
7bfe88e
2c47ff2
8da98bd
f912998
95bbdc2
ea33678
07feccd
03984d8
9bd92f8
078c176
5692d26
c131406
fda46f6
ab7afaa
e2de7d2
6d5919b
c1b7444
ea26292
dd290db
814ab22
c878e55
174b3c6
ee58000
ed87b18
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,119 @@ | ||
| # parquet-wasm limitations (and what we'd ideally have) | ||
|
|
||
| **Status:** notes, 2026-07-07. Context for the points feature-filter perf work | ||
| (see [points MVP roadmap](plans/points-mvp-and-roadmap.md)). | ||
|
|
||
| We vendor [`parquet-wasm`](https://github.com/kylebarron/parquet-wasm) for all | ||
| browser parquet decoding (`packages/core/src/parquetWasmLoader.ts`, | ||
| `packages/core/vendor/parquet-wasm/`). It works well, but its API shape forces a | ||
| specific trade-off for large transcripts `points.parquet` files, and it's worth | ||
| recording precisely what it can and cannot do so we can evaluate alternatives | ||
| (different bindings, a patched build, or hand-rolled footer parsing) later. | ||
|
|
||
| ## The capability we have | ||
|
|
||
| The normalized surface we rely on (`ParquetModule` in `parquetWasmLoader.ts`): | ||
|
|
||
| - `readParquet(fileBytes, { columns?, limit?, offset? })` — decode a **complete | ||
| parquet file** buffer, with column **projection during decode**. | ||
| - `readParquetRowGroup(schemaBytes, rowGroupBytes, rowGroupIndex, { columns? })` | ||
| — decode a **single row group** from its bytes + the file's schema bytes, again | ||
| projecting columns during decode. This is what makes per-row-group range reads | ||
| possible (`readParquetRowGroupBytesByGroupIndex` fetches | ||
| `[rowGroup.fileOffset(), rowGroup.compressedSize()]`). | ||
| - `readMetadata(footerBytes)` → row-group metadata exposing **only** | ||
| `numRows()`, `fileOffset()`, `compressedSize()` per row group | ||
| (`ParquetWasmRowGroupMetadata`). | ||
|
|
||
| ## The limitation that bites | ||
|
|
||
| **There is no way to fetch or decode an individual column chunk.** Consequences: | ||
|
|
||
| 1. **No projected *fetch*.** Column projection (`{ columns }`) happens only | ||
| *during decode*; the bytes handed to `readParquet` / `readParquetRowGroup` | ||
| must be the **whole file** or the **whole row group** — i.e. *all* columns. | ||
| For a 14-column Xenium `transcripts` file, building the feature catalog or the | ||
| per-row feature codes (which need one string column) still downloads every | ||
| column's bytes. | ||
| 2. **No column-chunk offsets in the metadata.** `ParquetWasmRowGroupMetadata` | ||
| does not expose per-`ColumnChunk` `file_offset` / `total_compressed_size` / | ||
| `data_page_offset` / `dictionary_page_offset`. Without those we cannot compute | ||
| the byte ranges of just the columns we want, so we cannot issue projected | ||
| range reads even manually. | ||
| 3. **Row-group bytes are not relocatable.** Per | ||
| [kylebarron/parquet-wasm#804](https://github.com/kylebarron/parquet-wasm/issues/804) | ||
| ("How to read a single row group batch, given only the row group bytes and the | ||
| schema bytes"), the footer's byte offsets are absolute to the original file, | ||
| so you cannot hand-concatenate a subset of column chunks into a synthetic row | ||
| group and decode it — the offsets no longer line up. `readParquetRowGroup` | ||
| sidesteps this by taking `schemaBytes` separately and the *contiguous* row-group | ||
| bytes, but that means the whole (all-column) row group must be fetched. | ||
|
|
||
| The practical upshot: on a large transcripts file the cost is dominated by | ||
| **decoding** the feature/geometry columns, and the only lever we have to keep the | ||
| UI responsive is to move that **decode** off the main thread — *not* to fetch | ||
| less. We still fetch whole row groups (all columns) via async range reads, but | ||
| the CPU-heavy decode runs in the points worker. See the off-thread | ||
| geometry+features decode in `VPointsSource.loadPoints` / | ||
| `decodeGeometryWithFeaturesFromPayload`. | ||
|
|
||
| ## Runtime probe (2026-07-07): what the Vitessce build actually exposes | ||
|
|
||
| The `.d.ts` types `readMetadata` as `unknown` and our `ParquetModule` wrapper only | ||
| surfaces `numRows/fileOffset/compressedSize`, but the underlying wasm object | ||
| exposes **more** than the wrapper. Introspecting the live object: | ||
|
|
||
| - `ParquetMetaData`: `fileMetadata()`, `numRowGroups()`, `rowGroup(i)`, `rowGroups()`. | ||
| - `RowGroupMetaData`: `numColumns()`, `column(j)`, `columns()`, `numRows()`, | ||
| `totalByteSize()`, `compressedSize()`, `fileOffset()`. | ||
| - `ColumnChunkMetaData`: `filePath()`, `fileOffset()`, `columnPath()`, | ||
| `encodings()`, `numValues()`, `compression()`, `compressedSize()`, | ||
| `uncompressedSize()`. | ||
| - **`ColumnChunkMetaData.statistics()` does NOT exist** — `col.statistics` is | ||
| `null`. So per-column-chunk **min/max are not reachable** even though the parquet | ||
| footer contains them (pyarrow reads them fine). | ||
|
|
||
| So the situation is more nuanced than "no column info": | ||
|
|
||
| - **Column-chunk *offsets* ARE available** (`column(j).fileOffset()` + | ||
| `compressedSize()` + `columnPath()`). A projected byte range per column is | ||
| computable. What still blocks a projected *fetch* is #804: `readParquetRowGroup` | ||
| needs the *contiguous* row-group bytes, and hand-concatenating a subset of column | ||
| chunks breaks the footer offsets — so we can compute the ranges but not feed a | ||
| sparse buffer back in for decode. | ||
| - **Column *statistics* are NOT available.** This blocks the **feature-primary | ||
| index** (skipping row groups whose feature range doesn't overlap the selected | ||
| genes): we'd need per-row-group `feature_name_codes` min/max to pick the ~3 of | ||
| 245 row groups a gene lives in, and the wasm won't give them. Reading them via | ||
| first/last-row reads (`loadParquetRowGroupColumnExtent`) fetches the *whole* | ||
| row group each time — fetching the entire 449 MB file just to build the index. | ||
| Getting stats efficiently needs one of: (a) a JS parse of the footer's Thrift | ||
| `FileMetaData` for `Statistics.min/max_value`; (b) an alternative metadata reader | ||
| (e.g. hyparquet, pure-JS, exposes row-group column stats); (c) extending the | ||
| vitessce/parquet-wasm build to surface `.statistics()`; or (d) a sidecar | ||
| per-row-group feature index emitted by the writer. | ||
|
|
||
| ## What we'd ideally have | ||
|
|
||
| Roughly in priority order for our use case (points/transcripts): | ||
|
|
||
| 1. **Column-chunk offsets in the metadata** — expose `ColumnChunkMetaData` | ||
| (`file_offset`, `total_compressed_size`, `data_page_offset`, | ||
| `dictionary_page_offset`) so we can compute per-column byte ranges. This alone | ||
| unlocks projected fetching. | ||
| 2. **A row-group decode that accepts a *sparse* set of column-chunk buffers** | ||
| (chunk bytes + which column + within-row-group offset), so we can fetch only | ||
| the columns we project and decode them without the whole row group. This is | ||
| exactly the ask in #804. | ||
| 3. **Dictionary-page-only reads** — for categorical columns (e.g. `feature_name` | ||
| as `dictionary<int16>`), the distinct values live in per-row-group dictionary | ||
| pages. Reading just those would build a feature catalog from a few KB per row | ||
| group instead of decoding the full column. | ||
| 4. **An async/range-read reader with random row-group access** that manages byte | ||
| sourcing itself (custom store), so we don't shuttle raw bytes across the | ||
| worker boundary. | ||
|
|
||
| We're open to evaluating alternative bindings or extending/patching the vendored | ||
| build to get (1)–(3); (1) is the highest leverage and smallest change. Until | ||
| then, off-thread decode (fetch-all-columns, decode-projected-off-thread) is the | ||
| pragmatic ceiling. | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -133,5 +133,19 @@ strategy — not a big-bang rewrite of the god-hook. | |
| escape hatch and "all on" is the least surprising.) | ||
| 2. Worker-backed catalog: dictionary-from-metadata fast path vs. same-bytes | ||
| off-thread decode. (Status doc P0 recommends metadata fast path first.) | ||
| **Finding (2026-07-06, live on a real Xenium `transcripts`):** the current | ||
| worker catalog path (`readParquetWorkerPayload` with `fullPartsForFallback` → | ||
| `scanParquetFeatureCatalogInWorker`) fetches the **entire** parquet file | ||
| before scanning, whereas the main-thread path does a *projected* single-column | ||
| range read of just the feature column. For a transcripts element with **no | ||
| `{feature_key}_codes` column** (so the cheap row-group *dictionary-page* scan | ||
| can't run), enabling the worker regressed catalog build from ~20s to >150s. | ||
| The request timeout in `pointsWorkerClient` (added this cycle) makes a silent | ||
| worker fall back safely, but does **not** fix this — the fetch is before the | ||
| worker call. **Next perf task:** give the worker a *projected/dictionary-only* | ||
| payload path (fetch only the feature column, or read dictionary pages) so | ||
| worker-offload is a win, not a regression — only then enable the worker in the | ||
| demo for catalog building. Until then the demo keeps the (blocking but faster) | ||
| main-thread path. | ||
|
Comment on lines
+136
to
+149
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win Update the roadmap status now that implementation work is underway. This section documents live behavior from the current worker implementation, which conflicts with the header’s “implementation not started” status. Change the status to “in progress” or explicitly scope that status to the roadmap plan rather than the implementation. 🤖 Prompt for AI Agents |
||
| 3. Engine submodule placement and one-object-vs-per-type facade — deferred to the | ||
| decomposition plan's open questions. | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,97 @@ | ||
| # Points — pre-merge punch-list & redesign backlog | ||
|
|
||
| Purpose: draw a clean line under the `points-feature-filter` PR before a larger | ||
| redesign. Everything here is either **fix-before-merge** (cheap, durable, or | ||
| stops a regression / stops the UI lying) or **defer-to-redesign** (entangled with | ||
| the state model, so patching now is throwaway). | ||
|
|
||
| ## Root cause the redesign targets | ||
|
|
||
| Most of the "wrong points / wrong stats" issues are one problem, not many: | ||
| `PointsEntry` (in `PointsDataEngine.ts`) is an **imperative mutable record** whose | ||
| fields are flipped with side effects mid-flight — `matchingLoading`, | ||
| `partialResult`, the atomic-swap on `ensureLoaded`, `reconcileRowCodes`, | ||
| `onProgress` mutating `loading` in place. That state is read through the | ||
| **monolithic `useLayerData`** and kept reactive only via `'use no memo'` escape | ||
| hatches. The decision of *which points to show* and *what the stats say* is spread | ||
| across those mutation sites, so it's ad-hoc and easy to get subtly wrong. | ||
|
|
||
| The redesign — **break up `useLayerData`** and **spike Effect / TanStack Query** | ||
| scoped to this runtime — is what fixes the *class*. Individual selection/stats | ||
| bugs below marked "defer" are downstream of it: fix them there, with an explicit | ||
| state model, not by patching mutations here. | ||
|
|
||
| --- | ||
|
|
||
| ## Fix-before-merge | ||
|
|
||
| | # | Item | Where | Kind | Note | | ||
| |---|------|-------|------|------| | ||
| | F1 | **Deselected features reappear while a covering scan streams** | `useLayerData` getLayers partial overlay | render-breaking | The partial overlay draws the buffer with **no selection filter**, unlike the settled matched layer (which passes `featureCodes` + `preloadedFeatureCodes`). Deselect a feature whose scan is still in flight → engine keeps that scan ("covered"), its partial keeps the deselected rows, overlay shows them until settle. **Introduced by this PR.** Cheap fix: pass the same filter props to the overlay (the partial's own `featureCodes` are available). Or revert the overlay. | | ||
| | F2 | **Delete dead `pointsRenderer.ts`** | `vis/.../renderers/pointsRenderer.ts` | hygiene | `renderPointsLayer` + its interfaces have **zero importers** (superseded by `PointsLayer`). Safe delete; leaves a cleaner starting line. | | ||
| | F3 | **Stop the summary line lying** | `PointsLayerPanel.ShowMatchingPoints` (`t.loaded`) | cosmetic (wrong number) | `t.loaded` is the covered-batch size, not the count matching the current selection, so "Loaded all N …" is often wrong. *Proper* fix needs the engine to count selection-matched rows = redesign. For merge: make the line honest cheaply (show a number that's actually right, or drop the misleading clause). | | ||
| | F4 | **Resolve the working tree** | `PointsDataEngine`, `PointsFeatureFilterPanel`, `PointsLayerPanel`, `models/index`, `pointsRenderer` (all uncommitted) | hygiene | Includes dangling notes (`// how do I get the engine from the context?`). Commit-or-revert each so the branch is coherent. | | ||
|
|
||
| **Undecided (cheap either way):** | ||
|
|
||
| - **U1 — overlay compositing.** Today the partial is a *separate sub-layer on top | ||
| of* the base (resident / prior matched), so during a scan you see both. Your | ||
| call: keep base+partial, or show partial-only during a scan. Small render | ||
| change in getLayers; orthogonal to F1 (F1 is about *filtering* the partial, | ||
| this is about *whether the base also draws*). Fine to defer. | ||
|
|
||
| --- | ||
|
|
||
| ## Defer-to-redesign | ||
|
|
||
| Each notes *why* it's coupled to the state-model / decode rework. | ||
|
|
||
| - **D1 — Mutable `PointsEntry` state model → Effect / TanStack Query.** The root | ||
| above. In-code smells already flagged: `PointsDataEngine.ts` `// I'm a bit iffy | ||
| about this ambient stateful thing` (onProgress), `// given ongoing problems with | ||
| agent debugging, inclined to more purity. Might consider using | ||
| Effect?`, `// there will be various mutating side-effects on entry…`. | ||
| - **D2 — Break up `useLayerData`.** The monolith the engine threads through; also | ||
| the reason for the `'use no memo'` hatches (`PointsFeatureFilterPanel`, | ||
| `ShowMatchingPoints`). A properly reactive state layer retires the hatches. | ||
| - **D3 — Progressive *initial* load (`loadPoints`).** Currently one-shot (bulk | ||
| fetch + single worker decode); making it progressive needs a per-part decode | ||
| loop **and** a general engine "partial resident" slot (the partial mechanism is | ||
| matching-specific today). The engine rework owns this. `pointsScanChunkProgress` | ||
| is already the reusable producer helper when we get there. | ||
| - **D4 — Progressive / active feature stats before the full catalog scan | ||
| completes.** Today stats only appear once the whole-dataset catalog settles; | ||
| there's real use in showing progressive/active counts. Tied to D3 (progressive | ||
| catalog build) and the stats state model (F3's proper fix). | ||
| - **D5 — Tiled (Morton) viewport-driven loading.** The tiled path isn't exercised; | ||
| viewport-driven load is a major feature and exactly the kind of demand-driven | ||
| state the new model should own (Morton tiling is still "dark" per the roadmap). | ||
| - **D6 — Worker contention with multiple layers.** Multiple point layers share | ||
| one worker; the engine keys by element and assumes single-demand-per-element. | ||
| Multi-layer sharing / a work queue belongs with the engine redesign. | ||
| - **D7 — GeoArrow encoding.** Unexplored; a decode-path spike, not this PR. | ||
| - **D8 — Streaming cancellation semantics.** The generators have no `AbortSignal` | ||
| threaded to the worker, and an abandoned manual `.next()` loop won't clean up. | ||
| Fine while consumers drain; design it with the new state layer. | ||
| - **D9 — Remove `'use no memo'` hatches (stable-snapshot option).** Give the | ||
| engine stable-identity snapshot accessors so `useSyncExternalStore` tracks the | ||
| value directly and the compiler stops needing an opt-out. Part of D1/D2. | ||
| - **D10 — Progressive-overlay visibility logic + flashing.** F1 fixed the | ||
| deselected-feature-lingering slice, but *which* points show during a partial | ||
| load still has logic problems, and it **flashes badly**: every notify rebuilds | ||
| the partial buffer into a fresh `PointsRenderResource` (new identity each | ||
| chunk), so deck tears down and recreates the `__partial` layer per step instead | ||
| of updating it in place. The real fix is a stable growing GPU buffer (preallocate | ||
| to cap, append, bump a draw count via `updateTriggers`) rather than a | ||
| rebuilt-per-chunk resource — which is the same append-buffer work noted for D3 | ||
| and the `pointsScanChunkProgress` O(chunks²) concat. Owned by the engine + | ||
| render redesign; the current overlay is a spike, not the destination. | ||
|
|
||
| --- | ||
|
|
||
| ## Suggested merge line | ||
|
|
||
| Do **F1–F4** (+ decide **U1**), confirm no regression vs `main`, tests + types | ||
| green. That yields a merged state that is *correct, honest, coherent, and | ||
| non-regressed* — without trying to make the selection logic *right*, which rides | ||
| the redesign (D1/D2). Everything in **Defer** stays untouched. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Clarify that column offsets alone are not sufficient for projected fetching.
The runtime probe above says column offsets are already available, yet the current decoder still requires contiguous row-group bytes. Reword this item as exposing offsets through the normalized wrapper—a prerequisite for a future sparse reader, not something that independently unlocks projected fetching.
Suggested wording
Also applies to: 100-107
🤖 Prompt for AI Agents