Boilerplate for the implementation of n-way unions #491
Conversation
jorisdral
requested review from
dcoutts,
mheinzel,
recursion-ninja and
wenkokke
as code owners
jorisdral
force-pushed
the
jdral/n-way-unions
branch
from
d5c8ba4 to
ad5d621
Compare
jorisdral
force-pushed
the
jdral/n-way-unions-boilerplate
branch
from
3a05525 to
45cf87e
Compare
jorisdral
force-pushed
the
jdral/n-way-unions-boilerplate
branch
2 times, most recently
from
9936513 to
b8a040b
Compare
dcoutts
left a comment
dcoutts
left a comment
There was a problem hiding this comment.
Actually the "same session" code is wrong, but easy to fix and then I think easy to explain.
mheinzel
left a comment
mheinzel
left a comment
There was a problem hiding this comment.
Partial review, just smaller notes.
jorisdral
force-pushed
the
jdral/n-way-unions-boilerplate
branch
2 times, most recently
from
ad222ee to
44008f4
Compare
mheinzel
left a comment
mheinzel
left a comment
There was a problem hiding this comment.
Looks good, a few minor comments.
| -- INVARIANT: a table's identifier is never changed during the lifetime of | ||
| -- the table. | ||
| , tableId :: !Word64 |
There was a problem hiding this comment.
Could it even change, now that it's not in the TableState any more? Same for the session below.
There was a problem hiding this comment.
Technically yes, because it's a record type. There's nothing stopping us from internally constructing a Table m h -> Table m h function that changes tableId. It's a silly example, I admit :P I thought it would be at least useful to mention that we shouldn't change tableId at any point
| Left (i, j) -> throwIO $ ErrUnionsSessionMismatch i j | ||
| Right sesh -> pure sesh | ||
|
|
||
| traceWith (sessionTracer sesh) $ TraceUnions (NE.map tableId ts) |
There was a problem hiding this comment.
I think we generally trace before checking things like that session and config match. Then if the check fails, the log ends with the operation that failed.
There was a problem hiding this comment.
Problem is that we have no tracer until after we've checked the sessions, so before we've checked the sessions we can't do any tracing yet. We could take the first tracer we find in the set of tables, but then there is the risk that the tables' tracers belong to different sessions and then it's not guaranteed that all tables have the same tracer. What I've done is change the order of checks and tracing: check sessions, trace, check table configs. That should make the trace appear earlier than it did previously
There was a problem hiding this comment.
Alternatively: we make the user pass in a session and use that one's tracer, and then check that the passed in session is the same as the tables' sessions. But I think it's also fine how we do it now
We add a new `unions` function to the `Internal` module, which should eventually contain the full implementation of unions. For now, the function contains boilerplate for: * Checking that all input tables have the same configuration, and * Checking that all input tables are in the same session, and * Duplicating references to the table contents for each of the tables * Creating a new table with unioned table contents The unioning of table contents is not yet implemented and will follow in some later set of PRs.
We have to check table types before calling the internal `union`s code from the public API.
jorisdral
force-pushed
the
jdral/n-way-unions-boilerplate
branch
from
6ceb69e to
cb6827e
Compare
We add a new
unionsfunction to theInternalmodule, which should eventually contain the full implementation of unions. For now, the function contains boilerplate for:The unioning of table contents is not yet implemented and will follow in some later set of PRs.