refactor joins to use direct implementation or each type rather than composition of inner+anti joins #571
Conversation
🦋 Changeset detectedLatest commit: d7d8b48 The changes in this PR will be included in the next version bump. This PR includes changesets to release 13 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
More templates
@tanstack/angular-db
@tanstack/db
@tanstack/db-ivm
@tanstack/electric-db-collection
@tanstack/query-db-collection
@tanstack/react-db
@tanstack/rxdb-db-collection
@tanstack/solid-db
@tanstack/svelte-db
@tanstack/trailbase-db-collection
@tanstack/vue-db
commit: |
|
Size Change: -14 B (-0.02%) Total Size: 66.6 kB
ℹ️ View Unchanged
|
|
Size Change: 0 B Total Size: 1.18 kB ℹ️ View Unchanged
|
| * - `indexB: Index<K, V2>` - all right-side rows accumulated over time | ||
| * | ||
| * **Mass maps** track presence efficiently: | ||
| * - `massA/massB: Map<K, number>` - sum of multiplicities per key |
There was a problem hiding this comment.
Essentially, this map keeps a consolidated multiset. I had a version of the Index that internally keeps such a map instead of an array. I think it makes more sense to modify the index to keep a consolidated map, rather than having the index and then also keeping a map here. This is a concern of the index.
| * Operator that joins two input streams | ||
| * Helper to build delta index and mass map from messages | ||
| */ | ||
| function buildDelta<K, V>( |
There was a problem hiding this comment.
This is definitely an index concern and not a join concern. The index should keep the "mass" (btw i think this is a very bad name, it's just (consolidated) multiplicity) and the join operator should simply do index.addValue(key, [value, multiplicity]) when it's going through the messages. No need for buildDelta.
|
|
||
| // Precompute mode flags to avoid repeated string comparisons | ||
| const mode = this.#mode | ||
| const emitInner = |
There was a problem hiding this comment.
isn't emitInner always true?
There was a problem hiding this comment.
Ah, no it's false when we're doing an anti join. Perhaps const emitInner = mode !== 'anti' is more accurate.
| if (multiplicity !== 0) { | ||
| results.add( | ||
| [key, [value, null]], | ||
| retract ? -multiplicity : +multiplicity |
There was a problem hiding this comment.
I don't get the retract logic. If the key used to have multiplicity 0, then retract is true so we will add the new change to the index but with its negated multiplicity, why?
| const after = before + deltaMass | ||
|
|
||
| // Skip if presence doesn't flip (0->0, >0->different>0) | ||
| if ((before === 0) === (after === 0)) continue |
There was a problem hiding this comment.
Even if the presence doesn't flip, don't we need to update the indexes/mass index ?
Say, that before is 5 and the new delta has multiplicity -3. Then after is 2. So it is still visible and this won't update the index so in the index the key will still have multiplicity 5. Next time, if it receives a multiplicity of -3 the key will still not flip (even though it should at this point!) because it thinks the multiplicity is 5. This looks wrong to me?
| this.#mode = mode | ||
| } | ||
|
|
||
| run(): void { |
There was a problem hiding this comment.
I would introduce helper functions to simplify this method and that way we could also unit test the helper functions in isolation. This method contains 4 steps, each step could be extracted to a helper function.
This removes all the additional state that was needed for each of the composed joins, and the need for the consolidate step. Delivers a father 50% speedup over the index refactor PR #549 this is stacked on.