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

DNM: Revisiting ownership of key and value types #429

Merged
merged 7 commits into from
Nov 27, 2023

Conversation

frankmcsherry
Copy link
Member

@frankmcsherry frankmcsherry commented Nov 25, 2023

This PR works to decouple any relationship between the input data types D in collections, and the presented key and value types K and V surfaced by arrangements. There are otherwise a tight coupling of the two, often that D = (K, V), and certainly both K and V must be sized and ownable and all that. However, we would like to break that, to allow e.g. data of the form (Vec<u8>, String) and key and values respectively &[u8] and &str, which are less directly related.

We break these links whenever possible, ensuring that the Arrange trait expresses opinions only about the data types in input collections, and not at all with respect to the output types of formed traces (except that the timestamps must be comparable). The Join and Reduce trait implementations are now backed by methods join_trace and reduce_trace that are each driven by trace types T1 and T2, whose requirements are not to input types, but just that some may need to implement ToOwned in order to be captured and maintained for some duration. Elsewhere, we are forced to decorate previously naive types, traits, and methods with caveats like K: Sized or K: ToOwned<Owned = K> or similar implicit assumptions that are now made explicit.

@frankmcsherry frankmcsherry force-pushed the something_owned branch 4 times, most recently from 629458e to d3a50e7 Compare November 25, 2023 13:06
@frankmcsherry
Copy link
Member Author

The most recent commit adds support for Val being unsized, and only implementing ToOwned.

But also exciting, @antiguru, are the PreferredContainer trait and the Preferred<K,V,T,R> struct which implements Update and Layout and uses the containers preferred by the types (e.g. Vec<String> for String, and SliceContainer<u8> for [u8]). You can see an example of it in examples/spines.rs

@frankmcsherry
Copy link
Member Author

The most recent commit relaxes Arrange to speak of input types, rather than potentially unsized arrangement types. This .. makes sense because the Arrange trait itself has no opinions about the trace types to be used, which show up only in the fn arrange<Tr> methods themselves. At that point, constraints on the traces are introduced, but only in the form of what inputs are required. All constraints on output types, e.g. [u8] or potentially GATs, are absent from this trait, its methods, and its implementations (except that the timestamps need to line up).

@frankmcsherry
Copy link
Member Author

The most recent commit looks large, but it's almost all whitespace due to changing a tab indent level. They introduce functions join_trace and reduce_trace which are not part of a trait, and are generic in trace rather than key, val, etc. This is to present a minimally restrictive form of the functionality, partly to ensure we are able to write this only as a function of traces, rather than .. weird related types that might need owned and borrowed variants, etc. Nope! Or, mostly nope!

@frankmcsherry frankmcsherry force-pushed the something_owned branch 2 times, most recently from 829a09e to 787cc7a Compare November 27, 2023 13:38
@@ -97,7 +98,8 @@ mod val_batch {
#[derive(Abomonation, Debug)]
pub struct RhhValStorage<L: Layout>
where
<L::Target as Update>::Key: Default + HashOrdered
<L::Target as Update>::Key: HashOrdered,
<L::Target as Update>::KeyOwned: Default + HashOrdered,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This probably shouldn't be a requirement of the structure itself, but rather the implementations that need to push data in (Default so we can pad locations, and HashOrdered for actual owned insertions).

@frankmcsherry frankmcsherry merged commit df03c4e into TimelyDataflow:master Nov 27, 2023
1 check passed
This was referenced Oct 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant