Skip to content

Use new merge batcher, remove ValOwned#26821

Merged
antiguru merged 5 commits into
MaterializeInc:mainfrom
antiguru:new_merge_batcher
May 6, 2024
Merged

Use new merge batcher, remove ValOwned#26821
antiguru merged 5 commits into
MaterializeInc:mainfrom
antiguru:new_merge_batcher

Conversation

@antiguru

@antiguru antiguru commented Apr 26, 2024

Copy link
Copy Markdown
Member

Rebase on current timely and differential:

This introduces a ConsolidatingContainerBuilder that consolidates its contents in-place, which eliminates the need for the ConsolidateBuffer. The PR thus removes this type.

Checklist

antiguru added 2 commits May 6, 2024 10:27
Signed-off-by: Moritz Hoffmann <mh@materialize.com>
Signed-off-by: Moritz Hoffmann <mh@materialize.com>
@antiguru antiguru force-pushed the new_merge_batcher branch from f802095 to 2880cf3 Compare May 6, 2024 16:11
Signed-off-by: Moritz Hoffmann <mh@materialize.com>
@antiguru antiguru force-pushed the new_merge_batcher branch from 339aae3 to 50b2100 Compare May 6, 2024 17:08
@antiguru antiguru requested a review from frankmcsherry May 6, 2024 17:11
@antiguru antiguru marked this pull request as ready for review May 6, 2024 17:11
@antiguru antiguru requested review from a team May 6, 2024 17:11
@antiguru antiguru requested a review from a team as a code owner May 6, 2024 17:11
Signed-off-by: Moritz Hoffmann <mh@materialize.com>

@frankmcsherry frankmcsherry left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good. A few comments, but otherwise good to go!

Comment thread src/compute/src/render/context.rs Outdated
T2: for<'a> Trace<
Key<'a> = T1::Key<'a>,
ValOwned = T1::ValOwned,
Val<'a> = T1::Val<'a>,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Future discussion point: is this the right constraint, or that V can be produced from T1::Val and accepted into T2?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

At the moment, we need a function to go from T1::Val -> V and T2::Val -> V. We could have two functions .. but it seems they're the same anyways, so the equality constraint doesn't seem to be too bad?

Comment thread src/compute/src/render/threshold.rs Outdated
Comment thread src/compute/src/server.rs
Signed-off-by: Moritz Hoffmann <mh@materialize.com>
@antiguru antiguru enabled auto-merge May 6, 2024 20:45
@antiguru antiguru merged commit 0f7906d into MaterializeInc:main May 6, 2024
@antiguru antiguru deleted the new_merge_batcher branch May 6, 2024 21:39
@antiguru antiguru mentioned this pull request May 7, 2024
5 tasks
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