Pre-arrange Differential introspection sources #22330
Conversation
antiguru
force-pushed
the
logging_prearrange
branch
from
79a4291 to
968186d
Compare
In a quick test, this reduces the number of rows feeding into the output as follows: * Batches: 70% * Records: 26% Tested with introspection debugging turned on. Signed-off-by: Moritz Hoffmann <[email protected]>
antiguru
force-pushed
the
logging_prearrange
branch
from
968186d to
ec8abd0
Compare
vmarcos
left a comment
vmarcos
left a comment
There was a problem hiding this comment.
LGTM! Minor comment below, totally optional (up to your preference!).
| let arrangement_batches = batches | ||
| .as_collection() | ||
| .mz_arrange_core::<_, RowSpine<_, _, _, _>>( | ||
| Exchange::new(move |_| u64::cast_from(worker_id)), |
There was a problem hiding this comment.
Not that this matters much, but the Exchanges at this position are merely defensive, right? In the previous version of the code, the data were just pipelined, so using a Pipeline pact here would also be an option.
There was a problem hiding this comment.
Yep, and thinking about it I'm not sure it makes a lot of sense! But I don't want to change it as part of this PR because there are other places that use the same construct, and it's better to change all at the same time.
The exchange would go away if/once #22384 lands because then we make it an invariant that log data gets processed locally.
There was a problem hiding this comment.
FWIW, this is where we decided to keep these exchanges: #18062 (comment).
But I also find Pipeline more readable here, as it conveys what invariants we believe to be true. When seeing the exchange without an explanation you might start to doubt your understanding of how events flow through these dataflows.
3 participants
In a quick test, this reduces the number of rows feeding into the output as follows:
Tested with introspection debugging turned on.
Motivation
Checklist
$T ⇔ Proto$Tmapping (possibly in a backwards-incompatible way), then it is tagged with aT-protolabel.