join_traces uses timely's container builder - #478
Merged
antiguru merged 8 commits intoApr 30, 2024
Conversation
Demonstrate how `join_traces` could look like if the closure was to use internal instead of external iteration. This fails tests because the join's output is no more consolidated, which is the big gap of this change, and makes it a non-starter unless we have an alternative. Signed-off-by: Moritz Hoffmann <antiguru@gmail.com>
Signed-off-by: Moritz Hoffmann <antiguru@gmail.com>
Signed-off-by: Moritz Hoffmann <antiguru@gmail.com>
Signed-off-by: Moritz Hoffmann <antiguru@gmail.com>
antiguru
marked this pull request as ready for review
April 30, 2024 21:19
antiguru
changed the title
join_traces uses timely's container builder
frankmcsherry
self-requested a review
April 30, 2024 21:25
frankmcsherry
added a commit
that referenced
this pull request
Jul 8, 2026
* join: reshape JoinTactic to an iterator of output containers Replace the two-method JoinTactic (defer + fuel-metered work) with a single `prep(input0, input1, fresh, meet) -> Box<dyn Iterator<Item = CB::Container>>`. The tactic now maps two batch lists to an iterator of output containers and holds neither capabilities nor a fuel budget, mirroring ReduceTactic's data-to-data division of labor. The driver owns the two per-direction queues, pairs each iterator with its shipping capability, pulls under a split fuel budget, ships each yielded container, and drops a unit when its iterator goes dry. "Work remains" becomes driver-observable (an iterator has yet to yield None) rather than a fuel-sign protocol the tactic must self-report. Join closures now push into a bare `&mut EffortBuilder<CB>` rather than a timely `JoinSession`. The session is bound to a live operator output and is not locally constructible, so it could not back a self-contained iterator; the arbitrary-container capability #478 wanted comes from the ContainerBuilder, not the session, so a bare builder suffices. `JoinSession` is removed. The cursor tactic shares its logic closure across units via Rc<RefCell<_>> (each unit is a 'static iterator and cannot borrow the tactic), preserving the single-mutable-state semantics of one closure threaded through every match. Deferred becomes DeferredIter: next() plays the same merge-join loop forward, suspending at container boundaries instead of under a fuel budget. Open for review: - EffortBuilder's record counter is now vestigial; the type reduces to a bare give-providing sink, or could be removed for push_into on CB directly. - DeferredIter::next uses mem::take on extract (needs Container: Default), stealing the builder's recycled allocation; a stash-swap would avoid it. - logic's public sink type changed (JoinSession -> EffortBuilder<CB>); Mz exposure unverified. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * join: remove the now-vestigial EffortBuilder Its only remaining job was renaming `push_into` to `give`; the record counter that justified the wrapper went dead when the driver began metering fuel from shipped containers' `record_count()`. Join closures now receive `&mut CB` and call `push_into` directly. `EffortBuilder` is deleted. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * join: fix the columnar example's join sink caller The reachability example pushed into the join sink via `session.give(..)`, an inherent method on the removed `EffortBuilder`. The sink is now `&mut CB`, so it calls `push_into` directly and imports `timely::container::PushInto` in the `reachability` module (the old inherent `give` needed no trait in scope). Only surfaces under `--all-targets`, which is why `cargo build --workspace` missed it but CI did not. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Tidy some structure * join: parameterize JoinTactic by its output container, not the builder The trait used `CB` only as `CB::Container` (the iterator's Item); it never constructs or pushes into a builder. Parameterizing by the container `C` instead keeps the builder where it is actually used — a private concern of the cursor tactic, which now carries it as `CursorTactic<B0, B1, L, CB>` and implements `JoinTactic<B0, B1, CB::Container>`. Two things fall out. The `Container: Default` bound (needed only for the cursor tactic's `mem::take`) leaves the trait for that impl. And the driver, which only ships finished containers via `give_container`, no longer needs the caller's builder: it pins the operator output to `NoopBuilder<C>` (a builder for any `C`, no bounds) and requires only `C: Container`. This is the groundwork for tactics that produce containers without a push-builder (proxy `cross`, columnar bulk), which would otherwise carry a builder they never use. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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.
Switch
join_tracesto use timely's container builder, and add a container builder that consolidates in-place.