-
Notifications
You must be signed in to change notification settings - Fork 3
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
Operator for join #57
Conversation
@@ -38,6 +39,10 @@ export abstract class OperatorBase<O extends object> implements Operator { | |||
} | |||
|
|||
commit(v: Version) { | |||
if (v <= this.#lastCommit) { |
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.
How does this happen again?
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.
for operators with more than one input. We send the commit notification down the graph so it'll get a commit notification twice.
An alternative would be to not send commit
down the graph and instead have every effect
and view
register with the global materialite
instance if they're run in a given transaction. On commit, they'd get notified directly.
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.
Working on a separate PR that does not send commit
through the graph. Only effects
and views
need to know of commits
so they can call their listeners so it makes sense not to send commit
through the graph from a saving work perspective.
} | ||
} | ||
|
||
// export |
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.
remove
], | ||
]); | ||
items.length = 0; | ||
}); |
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.
please add some tests where newDifference is called more than once per stream and version.
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.
good catch. I need to take version
into account in the operator to handle this case.
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.
This was actually working fine. I wrote the test case incorrectly 😅. New test added for this case that actually does the correct thing.
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.
LGTM
updated to always iterate over the smallest collection in the outer loop. This caused our synthetic ids for |
} | ||
} | ||
} | ||
return ret; | ||
} | ||
|
||
#concatIds(idA: string | number, idB: string | number) { |
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.
It is generally better to use module level functions than private methods. Only use private methods when you need access to other (private) state.
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.
Still LGTM
Join is a bit verbose to test. 900 lines of tests 😅.
Next PR will expose
JOIN
to ZQL proper.Left
andRight
join can be done without much extra work (add a row to the result set, rather than skipping, if a match is not present in the left/right table).Commits shouldn't be reviewed in isolation.
Kept for history:
Fiddling with the best way to alias tables until then. E.g.,
vs
Or do all joins as nested joins 🤔 and the
where
/select
parts becomes a pathAlternate
nest join
ideas:I think the
nest
idea is cleanest.Drawbacks: