zql [2/n] - Hook Replicache up to ZQL #30
Conversation
| r.mutate.initE1({id: '5', str: 'y'}), | ||
| ]); | ||
|
|
||
| expect(view.value).toEqual([{id: '5'}, {id: '4'}, {id: '3'}]); |
There was a problem hiding this comment.
It looks like we are copying the values here, to create objects with only the selected fields. I'm concerned about doing this for perf reasons. I would actually expect this test to fail, because of the extra str field in view.value.
There was a problem hiding this comment.
Possible I'm overthinking this. LMK what you think. FYI @arv
There was a problem hiding this comment.
Let's get the desired semantics first and measure the performance before we start making compromises.
| r.mutate.initE1({id: '5', str: 'y'}), | ||
| ]); | ||
|
|
||
| expect(view.value).toEqual([{id: '5'}, {id: '4'}, {id: '3'}]); |
There was a problem hiding this comment.
Let's get the desired semantics first and measure the performance before we start making compromises.
| // This lookup of old was due to seeing `oldValue` values that did not match | ||
| // what was stored on the client when working on Repliear. | ||
| // `oldValue` not matching what is on the client is problematic | ||
| // if there are derived sources with orderings that depend on fields outside the id. | ||
| // E.g., | ||
| // If a derived source is sorted by `modified_time` | ||
| // and the `oldValue` provided has a different `modified_time` we'll fail to remove | ||
| // the correct value from the derived source. |
There was a problem hiding this comment.
I'm wondering what is going on here? We need to track this down. We made watch non experimental on main and if there is a serious bug like this maybe we should keep it experimental.
There was a problem hiding this comment.
Dax has reported something like this to me, but he didn't ever have time to make a reproduction.
Looking at the code, neither Greg nor I can imagine any way this can happen. If you have a reproduction let's fix it.
There was a problem hiding this comment.
Thank you very much for noticing/flagging this @arv - totally missed it.
There was a problem hiding this comment.
This was several months ago that I saw the behavior. I'll see if I can put together a consistent repro though.
There was a problem hiding this comment.
I wonder if this could happen when changes are batched. Perhaps there is some bug where multiple updates to the same key get smooshed together as one experimentalWatch update and the oldValue for some reason is not the one that's currently in Replicache, but one of the other ones that got smooshed together. Just a random guess.
There was a problem hiding this comment.
Correction. watch is still experimental but I would still like to figure this out.
There was a problem hiding this comment.
One potential issue here is that await tx.get('x') might not be === to await tx.get('x') because cache eviction
| if (lVal === rVal) { | ||
| return 0; | ||
| } | ||
| if (lVal === null || lVal === undefined) { |
There was a problem hiding this comment.
why include undefined?
Is it to support optional properties?
There was a problem hiding this comment.
Yes, Rails / Zod allows undefined for optional properties.
A user could request to sort on an optional property.
| if (rVal === null || rVal === undefined) { | ||
| return 1; | ||
| } | ||
| if (lVal < rVal) { |
There was a problem hiding this comment.
Are we detecting non comparable values at any layer. It would be good if it could be enforced by typescript since we want this to be forward compatible with zero.
There was a problem hiding this comment.
We are not. We could do this in TypeScript in the orderBy method of EntityQuery. There we can exclude any field types that are not comparable.
src/zql/ivm/source/set-source.ts
Outdated
| @@ -120,12 +119,15 @@ export abstract class SetSource<T> implements Source<T> { | |||
| this._materialite.addDirtySource(this.#internal); | |||
| return this; | |||
| } | |||
|
|
|||
| get(key: T): T | null { | |||
There was a problem hiding this comment.
Can we use undefined for non present keys. That would be more consistent with everything else.
tantaman
changed the title
| return this.#tree.get(key); | ||
| get(key: T): T | undefined { | ||
| const ret = this.#tree.get(key); | ||
| if (ret === null) { |
There was a problem hiding this comment.
I see this is from a different layer... I guess it is fine to use null if it does not leak to any public APIs
There was a problem hiding this comment.
We can pull in that other layer down the line. It is pretty much just a Treap and PersistentTreap.
| } | ||
|
|
||
| #onReplicacheDiff = (changes: ExperimentalNoIndexDiff) => { | ||
| this.#materialite.tx(() => { |
There was a problem hiding this comment.
I'm wondering if the materialite.tx should be around all of the diffs (for all of the source tables) for an underlying Replicache commit. I think achieving this would require a deeper integration with Replicache.
There was a problem hiding this comment.
Well actually since we dispatch all the diffs synchronously, I think we can achieve it without deeper integration, if we have a version of materialite.tx with explicit materialite.txStart/materialite.txEnd. If a tx is not open #onReplicacheDiff, we do materialite.txStart, and enqueue a microtask to do materialite.txEnd.
There was a problem hiding this comment.
I'm wondering if the materialite.tx should be around all of the diffs (for all of the source tables) for an underlying Replicache commit.
yep, it should. Good catch.
There was a problem hiding this comment.
Good catch. That might also explain some odd behaviors I'm seeing.
Takes events from
replicache.experimentalWatchand sends them to ZQL to maintain queries.See
replicache-context.test.tsfor examples.