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

Update to latest Timely #519

Merged
merged 2 commits into from
Sep 9, 2024
Merged

Conversation

antiguru
Copy link
Member

@antiguru antiguru commented Sep 9, 2024

Bring in changes related to abomination and reference-counted addresses.

Signed-off-by: Moritz Hoffmann [email protected]

Bring in changes related to abomination and reference-counted addresses.

Signed-off-by: Moritz Hoffmann <[email protected]>
@@ -159,7 +159,6 @@ mod val_batch {
///
/// The `L` parameter captures how the updates should be laid out, and `C` determines which
/// merge batcher to select.
#[derive(Abomonation)]
Copy link
Member

Choose a reason for hiding this comment

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

Does this need a #[derive(Serialize, Deserialize)]? I don't know, and maybe we wont until we start to use it? :)

Copy link
Member Author

Choose a reason for hiding this comment

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

We cannot easily. Serde requires the GAT to be serialize/deserialize unless we tell it otherwise.

Copy link
Member

Choose a reason for hiding this comment

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

Gotcha. And .. no particular reason to want to serialize / deserialize these, outside perhaps of interest in some DD-based persistence? I guess I forget why the derive(Abomonation) was there in the first place, and perhaps there was no reason anymore?

@@ -720,7 +719,6 @@ mod key_batch {
///
/// The `L` parameter captures how the updates should be laid out, and `C` determines which
/// merge batcher to select.
#[derive(Abomonation)]
Copy link
Member

Choose a reason for hiding this comment

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

Same as above; removing a derived Abomonation without introducing Serialize, Deserialize. Idk the consequences.

@@ -250,8 +250,7 @@ mod val_batch {
///
/// The `L` parameter captures how the updates should be laid out, and `C` determines which
/// merge batcher to select.
#[derive(Abomonation)]
pub struct RhhValBatch<L: Layout>
pub struct RhhValBatch<L: Layout>
Copy link
Member

Choose a reason for hiding this comment

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

Same Q as elsewhere.

Copy link
Member

@frankmcsherry frankmcsherry left a comment

Choose a reason for hiding this comment

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

All looks good to me, except a few moments where we lost an Abomonation derive and did not replace it with a Serialize, Deserialize derive. That might be fine (perhaps it was unused?), but I wanted to flag them in case they are known oopsies.

Signed-off-by: Moritz Hoffmann <[email protected]>
@antiguru antiguru merged commit 9376560 into TimelyDataflow:master Sep 9, 2024
7 checks passed
@antiguru antiguru deleted the update_timely branch September 9, 2024 19:49
antiguru added a commit to MaterializeInc/differential-dataflow that referenced this pull request Sep 9, 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.

2 participants