Noah/refactor filtering by froyo-np · Pull Request #287 · AllenInstitute/vis

froyo-np · 2026-08-13T17:37:41Z

What

the initial implementation of filtering did not support some features (like referring to the .x or .y value of a vector column) that were critical for the actual filters we need in our use case. Making this all work required a bit of a rewrite, but its much more capable now, and the "andOpen" style syntax, which was confusing, is now replaced with something that is still a bit confusing, but better and you get more power from it.

How

use a builder-style expression system, rather than very fragile typescript template-string types.

Screenshots

This section is optional if there are no visible changes

If possible add screenshots of the visible additions in the UI.
If there are changes in the UI, add Before and After Screenshots for quick overview.
If there was a Figma design, add a link to that here as well.
Hint : Drag and Drop any images you want to add to the PR. Also you can create a gif of an interactive version and add that!

PR Checklist

approach, which resulted in any type intermediate values, ruining everything.

froyo-np · 2026-08-13T17:41:41Z

-export type VarName = `${letter}${string}`;
-// type Var<T extends VarName> = `$${T}`
-// I want a type error if the op type is a vOp and either operand is not...
-export type PredicateExpr<T extends ITable, K extends keyof T, Param extends VarName> = K extends string


I'd probably read this code in editor, rather than diff-view - its essentially a re-write of most of the fun parts.

gone are the complicated template-string style types.

replaced are shorter, templated builder-style functions, which infer their types from the parameters, which are usually very short strings.

…titute/vis into noah/refactor-filtering

lanesawyer

Handful of comments, looks to be working well, the demo is still functional!

lanesawyer · 2026-08-13T20:38:23Z


 ```ts
-.given({
+const { table, clause, select, all } = given({


Yeah, I like this approach a bit better. Definitely more verbose, but not detrimentally so.

lanesawyer · 2026-08-13T21:13:53Z

-                : Params & { [k in Param]: TsType<Ts[O][F]> }
-        >(this.ctx, [...this.predicates, { OP: 'and', pred }]);
+/* oxlint-disable no-console, typescript/no-explicit-any*/
+const entries = <T extends {}>(r: T): ReadonlyArray<[keyof T, T[keyof T]]> => Object.entries(r) as any;


Hm, I don't love the as any force casting here and in gen.ts but I'm not seeing a really straightforward other option that I've liked as I've played aroudn with it.

yeah - the way it works out is essentially Object.entries(r) as any as ReadonlyArray<[keyof T, T[keyof T]]>
mostly this was me struggling with both lodash entries and Object.entries types given my very fragile type stuff going on here - I'll try and get rid of the cast, but to be fair, its pretty contained - the any cant escape!

ok - this was noah having Typescript fever - I've removed the nonsense here and now lots of stuff lower down no longer needs silly casting.

lanesawyer · 2026-08-13T21:15:40Z

-        .build(device, 'testing');
+    const { all, any, column, table, select, clause } = given(tableLayout).from('edges');
+
+    const filter = select('$index')


Demo still works and shows a table of read data!

lanesawyer · 2026-08-13T21:19:59Z

+                );
+                return Q.shader;
+            },
+            build: (device: GPUDevice, label: string) => {


build and buildIndex are very similar, maybe extract a helper function? Or do we expect them to diverge more substantially over time?

they differ only in that one returns a function which requires an elements buffer to be passed, full of indexes. I think I can wrestle the typechecker... these are only separate so that the inferrence of that requirement works out - I'll give it a shot.

lanesawyer · 2026-08-13T21:30:35Z

+                // ok now get the results and compare them!
+                Promise.all([resolve.mapAsync(GPUMapMode.READ), resolveCount.mapAsync(GPUMapMode.READ)])
+                    .then(() => {
+                        const count = new Uint32Array(resolveCount.getMappedRange());


This variable is unused. Should it be used? Otherwise delete!

lanesawyer · 2026-08-13T21:51:49Z

+            const runner = (args: Indexed extends true ? RunIndexedFilterArgs<Ts> : RunFilterArgs<Ts>) => {
+                const { enc, parameters, sets, timestampWrites } = args;
+                // here, we zero out the result counters
+                // TODO - consider not doing this - if we didnt do that:


A TODO worth solving now?

lanesawyer · 2026-08-13T21:57:28Z

+                const bindings = indexed
+                    ? (args as RunIndexedFilterArgs<Ts>).sets.map((s, i) => {
+                          return device.createBindGroup({
+                              layout: pipe.pipeline.getBindGroupLayout(1),
+                              entries: [
+                                  { binding: 0, resource: s.resultCounter },
+                                  { binding: 1, resource: s.results },
+                                  { binding: 2, resource: s.elements },
+                                  ...mapTablesToBindings(s.tables, safeLookups),
+                              ],
+                          });
+                      })
+                    : args.sets.map((s, i) => {
+                          return device.createBindGroup({
+                              layout: pipe.pipeline.getBindGroupLayout(1),
+                              entries: [
+                                  { binding: 0, resource: s.resultCounter },
+                                  { binding: 1, resource: s.results },
+                                  ...mapTablesToBindings(s.tables, safeLookups),
+                              ],
+                          });
+                      });


Could simplify this a bit, the only difference is the { binding: 2, resource: s.elements }, bit. Don't know that I love having the indexed && "elements" in s check though, could obfuscate a little bit what's happening, since folks might not look deeply into entries and not understand the indexed versus not indexed take.

Up to you, just saw a little bit of code duplication that could be avoided in a few different ways!

const bindings = args.sets.map((s) => device.createBindGroup({ layout: pipe.pipeline.getBindGroupLayout(1), entries: [ { binding: 0, resource: s.resultCounter }, { binding: 1, resource: s.results }, ...(indexed && "elements" in s ? [{ binding: 2, resource: s.elements }] : []), ...mapTablesToBindings(s.tables, safeLookups), ], }), );

lanesawyer · 2026-08-13T21:58:26Z

+                for (let i = 0; i < sets.length; i++) {
+                    const s = sets[i]!;
+                    const bg1 = bindings[i]!;
+                    // console.log('running filter-->',s.results.label)


Can remove, or potentially do a logger.debug here so we could turn these on later if we choose.

I don't think our logger gets stripped from the build though so no-ops would be annoying for performance... Maybe something for us to look into later so we can feel more free to sprinkle debug logs around.

lanesawyer · 2026-08-13T22:00:36Z

+                            inputs.push(buf);
+                            return buf;
+                        })
+                ) as BufferTables<Ts>; // TS cant tell, mapValues I think does erase the info...


mapValues does have a generic parameter but I'm having trouble making it happy...

Yeah, also dug into this one, not finding a working variant of mapValues that solves this problem -- all of them erase key-typing in one way or another. 😞

lanesawyer · 2026-08-13T22:01:01Z

+    ArrayBufferTables,
 } from './types';
-import * as wgh from 'webgpu-utils';
+import * as lo from 'lodash';


Oooh maybe we move to lodash-es here too soon, didn't realize we had only done that in one repo so far.

yeah I'd love to - ye olde lodash imports break the build regularly unless I do absolutely wacky stuff.

Replacement PR here! #288

We can merge yours first and I can come clean up the remainder later, or you can review and merge that and start using in this PR. Either way!

field - its more readable even!

for query building helpers

Jarbuckle

Just a couple things, looking good overall!

Jarbuckle · 2026-08-13T17:48:09Z

-    ${outputStructDecl}
+    ${decl}


Looks like this got reverted somehow 😅

fixed, sorry about that...

Jarbuckle · 2026-08-14T20:00:10Z

+                            inputs.push(buf);
+                            return buf;
+                        })
+                ) as BufferTables<Ts>; // TS cant tell, mapValues I think does erase the info...


Yeah, also dug into this one, not finding a working variant of mapValues that solves this problem -- all of them erase key-typing in one way or another. 😞

Jarbuckle · 2026-08-14T20:04:27Z

+                        for (let i = 0; i < expected.buffer.byteLength; i++) {
+                            if (dv.getUint8(i) !== ex.getUint8(i)) {
+                                logger.error('filter validation failed at byte: ', i);
+                                failBytes += 1;
+                            }
+                        }
+                        if (failBytes === 0) {
+                            logger.info('validation success');
+                        }
+                    })


How many comparisons is this doing, on average? 🤔 And do we need to know the number of bytes that failed? Also, genuinely curious if we want to keep going if we get failures? Would that be a filter algorithm issue, a hardware issue, both, neither?

the idea of this is a smoke-test. it will certainly fail if you ask it to filter more than 64 rows of data, so at worst, this will fail after 512 checks. I've re-written this to return a copy of the result buffer in the failing case, and it now stops at the first failed byte.

in the validate, stop validating at the first failed byte, return the copy for the user to determine what went wrong

lanesawyer

Changes make sesne, thanks for the test, and the demo still runs. I think any other pain points can be ironed out again as we hit them while making further progress on Connectomics!

lanesawyer · 2026-08-14T22:57:42Z

Simple but effective at proving the output looks as expected! Thanks!

lanesawyer · 2026-08-14T22:58:41Z

+    ArrayBufferTables,
 } from './types';
-import * as wgh from 'webgpu-utils';
+import * as lo from 'lodash';


Replacement PR here! #288

We can merge yours first and I can come clean up the remainder later, or you can review and merge that and start using in this PR. Either way!

froyo-np added 4 commits August 13, 2026 08:18

initial refactor to support a bunch more stuff...

55fe06d

update, fight with lodash, fix demo... seems to work

97431bd

fmt

4133cac

heavy refactor to avoid type erasure of internal classes of previous

e277bda

approach, which resulted in any type intermediate values, ruining everything.

froyo-np requested a review from a team as a code owner August 13, 2026 17:37

froyo-np requested review from Jarbuckle, TheMooseman and lanesawyer and removed request for lanesawyer August 13, 2026 17:37

Merge branch 'main' into noah/refactor-filtering

8aa43d9

froyo-np commented Aug 13, 2026

View reviewed changes

froyo-np added 4 commits August 13, 2026 11:01

wording updates

58644cd

lint

9f6014c

fmnt

17445f2

Merge branch 'noah/refactor-filtering' of https://github.com/AllenIns…

975e3a7

…titute/vis into noah/refactor-filtering

lanesawyer reviewed Aug 13, 2026

View reviewed changes

froyo-np added 6 commits August 13, 2026 16:41

excellent PR feedback, much nonsense removed

393396b

a better approach to determining the expected type of a swizzled vector

ea0ef87

field - its more readable even!

fmt

0243876

lint

9236c74

get rid of logger in favor of a returned validation value, and add tests

144c777

for query building helpers

fmt

1403589

Jarbuckle reviewed Aug 14, 2026

View reviewed changes

froyo-np added 2 commits August 14, 2026 14:12

struct decl not decl... not sure how that happened...

ed531f3

in the validate, stop validating at the first failed byte, return the copy for the user to determine what went wrong

fmt

1141392

lanesawyer approved these changes Aug 14, 2026

View reviewed changes

froyo-np merged commit f0e45cb into main Aug 17, 2026
5 checks passed

froyo-np deleted the noah/refactor-filtering branch August 17, 2026 17:32

Uh oh!

Conversation

froyo-np commented Aug 13, 2026

What

How

Screenshots

PR Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lanesawyer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jarbuckle left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lanesawyer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants