Skip to content
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

Associated types for TraceReader #152

Open
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@frankmcsherry
Copy link
Member

frankmcsherry commented Mar 5, 2019

This PR introduces associated types Key, Val, Time, and R for TraceReader. These types were previously generic parameters, and infected all of the types related to trace management. Many types are now substantially simpler to express, because the TraceReader implementation drives these four "associated" types.

As an example, the arrange_named method, the core of arrangement, now has the following signature:

    fn arrange_named<Tr>(&self, name: &str) -> Arranged<G, TraceAgent<Tr>>;

whereas both Arranged and TraceAgent used to have various K, V, T, R generic type parameters.

The only downside so far is that due to the lack of "implied bounds", there are some places where we must repeat various type bounds that are "known" to be true. For example, we may need to write

where
    Tr: TraceReader<Time=G::Timestamp>+Clone+'static,
    Tr::Batch: BatchReader<Tr::Key,Tr::Val,G::Timestamp,Tr::R>+'static,
    Tr::Cursor: Cursor<Tr::Key,Tr::Val,G::Timestamp,Tr::R>+'static,

where the second two constraints are constraints of the TraceReader trait itself, and so should already hold by definition. So, eyes open for that.

cc @comnik @ryzhyk

@comnik

This comment has been minimized.

Copy link
Contributor

comnik commented Mar 5, 2019

This looks great! I have to hold out for a bit before trying it out tho, I just got a bunch of stuff stabilised...

@frankmcsherry

This comment has been minimized.

Copy link
Member Author

frankmcsherry commented Mar 5, 2019

No worries. It is pretty massively breaking, and wanted to get a read from you and @ryzhyk before doing anything, as it is probably pretty disruptive.

@ryzhyk

This comment has been minimized.

Copy link

ryzhyk commented Mar 5, 2019

Looks neat, those TraceReader types were getting out of control :) Will definitely try it out, but probably in a day or two (it is going to affect a lot of my code, so I need to allocate a good chunk of time to it).

@frankmcsherry

This comment has been minimized.

Copy link
Member Author

frankmcsherry commented Mar 5, 2019

One answer is certainly "this looks promising, but exhausting; please delay until next major release" which is certainly something I can do.

@ryzhyk

This comment has been minimized.

Copy link

ryzhyk commented Mar 5, 2019

Depends on how much rush you're in to release 0.9. If you think it can wait a couple of days, I'm more than happy to do it.

@ryzhyk

This comment has been minimized.

Copy link

ryzhyk commented Mar 8, 2019

@frankmcsherry, sorry, I'm still dealing with deadlines. Probably won't do it till next week.

@frankmcsherry

This comment has been minimized.

Copy link
Member Author

frankmcsherry commented Mar 8, 2019

No worries! I've got a lot to do too. No rush to land this.

comnik added a commit to comnik/declarative-dataflow that referenced this pull request Mar 8, 2019

@comnik

This comment has been minimized.

Copy link
Contributor

comnik commented Mar 8, 2019

So my stuff builds, tests pass, no problems there. Unfortunately my major touchpoint is the PrefixExtender-compatible index type, which maintains three different trace types (count, propose, validate).

This leads to some interesting ASCII art: comnik/declarative-dataflow@c7d8e8d#diff-b4aea3e418ccdb71239b96952d9cddb6R311

(Edit: I think this change is for the better, I don't mind the extra typing)

@ryzhyk

This comment has been minimized.

Copy link

ryzhyk commented Mar 18, 2019

My tests compile and run. Type signatures are much simpler now, but the where clauses in my function declarations are kind of gigantic now, as pointed out in the PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.