Rework container builder to use push into#569
Merged
antiguru merged 4 commits intoTimelyDataflow:masterfrom Jun 12, 2024
Merged
Rework container builder to use push into#569antiguru merged 4 commits intoTimelyDataflow:masterfrom
antiguru merged 4 commits intoTimelyDataflow:masterfrom
Conversation
frankmcsherry
approved these changes
Jun 11, 2024
Member
frankmcsherry
left a comment
frankmcsherry
left a comment
There was a problem hiding this comment.
Looks great, with some comments about potentially optional code, and some clarity / moments where I was confused as a reader.
Previously, `ContainerBuilder` had `push` and `push_container` functions, which were odd in the presence of the `PushInto` trait. This change removes the functions and instead relies on a `PushInto` implementation. As a bonus, this removes several `SizableContainer` bounds, which are now up to the caller to enforce should they push into a capacity-based container builder. Specifically, it adds the following new types or APIs: * ContainerBuilder: remove `push` and `push_container`. Replaces the fromer with a `PushInto` implementation, and moves the latter into a function on `CapacityContainerBuilder`. All uses of `give_container` are now special-cased to said builder. Other builders can accept containers through their `PushInto` implementation. * A `BufferingContainerBuilder` that only accepts complete buffers. Could back out that change because it's similar (but not equal!) to the capacity-based variant. * core::Input learns `new_input_with_buider` that allows the user to specify a customer builder. Existing APIs should function unchanged and use the capacity-based builder. * core::SharedStream uses the buffering container builder. * core::to_stream gains a `ToStreamBuilder` trait that allows the user to specify the builder. `ToStream` uses that but fixes the builder to the capacity-based variant. Signed-off-by: Moritz Hoffmann <antiguru@gmail.com>
Signed-off-by: Moritz Hoffmann <antiguru@gmail.com>
antiguru
force-pushed
the
container_builder_input
branch
from
b1fde26 to
e639eb1
Compare
Signed-off-by: Moritz Hoffmann <antiguru@gmail.com>
frankmcsherry
approved these changes
Jun 12, 2024
Member
frankmcsherry
left a comment
frankmcsherry
left a comment
There was a problem hiding this comment.
Looks good. Merge at your discretion!
Signed-off-by: Moritz Hoffmann <antiguru@gmail.com>
antiguru
added a commit
to TimelyDataflow/differential-dataflow
that referenced
this pull request
Jun 13, 2024
This fixes a potential bug in the consolidating container builder where it would not clear buffers that were exposed to the user. There is no obligation for the user to call clear, so call it when recycling. Signed-off-by: Moritz Hoffmann <antiguru@gmail.com>
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Previously,
ContainerBuilderhadpushandpush_containerfunctions,which were odd in the presence of the
PushIntotrait. This change removesthe functions and instead relies on a
PushIntoimplementation. As abonus, this removes several
SizableContainerbounds, which are nowup to the caller to enforce should they push into a capacity-based
container builder.
Specifically, it adds the following new types or APIs:
pushandpush_container. Replaces the fromerwith a
PushIntoimplementation, and moves the latter into a functionon
CapacityContainerBuilder. All uses ofgive_containerare nowspecial-cased to said builder. Other builders can accept containers
through their
PushIntoimplementation.BufferingContainerBuilderthat only accepts complete buffers. Couldback out that change because it's similar (but not equal!) to the
capacity-based variant.
new_input_with_buiderthat allows the user tospecify a customer builder. Existing APIs should function unchanged and
use the capacity-based builder.
ToStreamBuildertrait that allows the user tospecify the builder.
ToStreamuses that but fixes the builder to thecapacity-based variant.
Signed-off-by: Moritz Hoffmann antiguru@gmail.com