Introduce EnterTime, fix Negate #587
Conversation
Introduces the EnterTime trait, which allows streams to explain to differential how they can modify their timestamp component to match the inner time. Fixes the Negate trait so it can be implemented by downstream crates. Due to Rust's rules, the local type has to come first, so the `G` parameter has to come after the container type. Signed-off-by: Moritz Hoffmann <[email protected]>
Signed-off-by: Moritz Hoffmann <[email protected]>
There was a problem hiding this comment.
Nits on some doccomments, but a larger question about generic argument order for EnterTime and Negate, which feel like they should be flipped unless I misunderstand something.
Additional nit: I can't tell whether EnterTime should be redundant with timely's Enter trait, which does the same thing but doesn't change the container type. My sense is that EnterUpdates is maybe more what is intended, in that this is only meant to work for updates specifically. I'm not certain I understand whether this should be a trait on streams or a trait on containers; the stream logic seems to be mainly "bring the stream in, and change the container" and I can't think of other implementations.
| use timely::dataflow::scopes::Child; | ||
| use timely::progress::timestamp::Refines; | ||
|
|
||
| /// Extension trait for streams. |
There was a problem hiding this comment.
Nit, but doccomment could use some love.
| G: ScopeParent, | ||
| TInner: Refines<G::Timestamp>, | ||
| { | ||
| /// The containers in the output stream. |
There was a problem hiding this comment.
Similar nit: probably something better to say here. Perhaps
/// The type of container used to represent updates with `TInner` timestamps
| where | ||
| G::Timestamp: Lattice, | ||
| StreamCore<G, C>: crate::operators::Negate<G, C> + ResultsIn<G, C>, | ||
| StreamCore<G, C>: crate::operators::Negate<C, G> + ResultsIn<G, C>, |
There was a problem hiding this comment.
I think conventionally, we usually have scope parameters first and containers later (e.g. timely's Map trait, as the first one I found). I'm looking at my local DD repo, and it has Negate<G, C> in it, where G: Scope. I'm not sure I understand the flipping that is going on here.
| use timely::progress::timestamp::Refines; | ||
|
|
||
| /// Extension trait for streams. | ||
| pub trait EnterTime<C, G, TInner> |
There was a problem hiding this comment.
As with the Negate comment, I think G, TInner, C is the more common order; certainly the scope comes first in just about all timely traits iirc (both Map and Operator, that I just checked). Timely's Enter trait is Enter<G, T, C> fwiw.
|
Discussed a bit with @antiguru : one take is that these could/should be properties of containers rather than streams. Concern was raised that this may prevent flexibility about the mutability/ownership of the arguments, e.g. when |
2 participants
Introduces the EnterTime trait, which allows streams to explain to
differential how they can modify their timestamp component to match the
inner time.
Fixes the Negate trait so it can be implemented by downstream crates. Due
to Rust's rules, the local type has to come first, so the
Gparameter hasto come after the container type.
MaterializeInc/materialize#31980 shows how to implement the traits for local types.
Signed-off-by: Moritz Hoffmann [email protected]