Skip to content

Commit 45cd71b

Browse files
antiguruclaude
andcommitted
doc: tighten the columnar Get/Mfp-output comments
Give the missing-push-bound rationale a single owner, drop the comparisons against the row-based builder, and cut the test doc to what the asserts do not already state. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 05b5272 commit 45cd71b

1 file changed

Lines changed: 12 additions & 33 deletions

File tree

src/compute/src/render/context.rs

Lines changed: 12 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -766,9 +766,8 @@ impl<'scope, T: RenderTimestamp> CollectionBundle<'scope, T> {
766766
for<'a> BatchCursor<Tr>:
767767
Cursor<Key<'a>: ExtendDatums, Val<'a>: ExtendDatums, Time = T, Diff = mz_repr::Diff>,
768768
<<BatchCursor<Tr> as Cursor>::KeyContainer as BatchContainer>::Owned: PartialEq,
769-
// The builder accepts whatever `logic` gives it, so the push bound lives at the `give`
770-
// call site rather than here. This lets a caller push borrowed records into a columnar
771-
// builder that has no owned-tuple `Push`.
769+
// No push bound here: it lives at `logic`'s `give` call site, so a caller can push
770+
// borrowed records into a columnar builder that has no owned-tuple `Push`.
772771
DCB: ContainerBuilder,
773772
// `logic` receives the key and value already decoded into a `DatumVecBorrow`. The decode
774773
// (and its arena/`DatumVec`) lives in the per-activation closure below, so it is scoped to
@@ -1015,15 +1014,12 @@ impl<'scope, T: RenderTimestamp> CollectionBundle<'scope, T> {
10151014
if mfp_plan.is_identity() && !has_key_val {
10161015
let key = key_val.map(|(k, _v)| k);
10171016
return match key {
1018-
// Unarranged identity: hand the edge straight through, so a
1019-
// columnar producer stays columnar without a `ColumnarToVec` hop.
1017+
// Unarranged identity hands the edge straight through, so a columnar
1018+
// producer stays columnar.
10201019
None => self
10211020
.collection
10221021
.clone()
10231022
.expect("The unarranged collection doesn't exist."),
1024-
// Keyed identity reads an existing arrangement, which is
1025-
// row-based. Wrap it as a `Vec` edge. `as_specific_collection`
1026-
// stays the consumer leaf.
10271023
Some(key) => {
10281024
let (oks, errs) = self.as_specific_collection(Some(&key), config_set);
10291025
(CollectionEdge::Vec(oks), errs)
@@ -1048,13 +1044,9 @@ impl<'scope, T: RenderTimestamp> CollectionBundle<'scope, T> {
10481044
// Wrap in an `Rc` so that lifetimes work out.
10491045
let until = std::rc::Rc::new(until);
10501046

1051-
// The ok output is built into a `Column`, so this producer emits the
1052-
// columnar edge. `ConsolidatingColumnBuilder` folds within-batch
1053-
// duplicates, matching the row-based `ConsolidatingContainerBuilder`
1054-
// this replaced. It stages owned `(Row, T, Diff)` tuples to consolidate
1055-
// in place, so the records are given owned; `mfp_plan.evaluate` already
1056-
// produces a fresh owned `Row` per result, so this is a move into
1057-
// staging, not a new allocation.
1047+
// `ConsolidatingColumnBuilder` folds within-batch duplicates. It consolidates in
1048+
// place, so records are given owned, which costs nothing here because
1049+
// `mfp_plan.evaluate` already produces a fresh `Row` per result.
10581050
let (stream, errors) = self.flat_map::<ConsolidatingColumnBuilder<Row, T, Diff>, _>(
10591051
key_val,
10601052
max_demand,
@@ -1161,9 +1153,7 @@ impl<'scope, T: RenderTimestamp> CollectionBundle<'scope, T> {
11611153
.try_into()
11621154
.expect("must fit");
11631155
bucketed = true;
1164-
// Temporal bucketing consumes and produces a `Vec` edge, so
1165-
// decode here. This is the sanctioned leaf decode where a
1166-
// `Vec`-internal operator meets the columnar edge.
1156+
// Temporal bucketing is `Vec`-internal, so decode here.
11671157
CollectionEdge::Vec(T::maybe_apply_temporal_bucketing(
11681158
oks.into_vec().inner,
11691159
as_of.clone(),
@@ -1456,8 +1446,6 @@ where
14561446
ok_output: &mut OutputBuilderSession<'_, C::Time, DCB>,
14571447
err_output: &mut OutputBuilderSession<'_, C::Time, ECB<C::Time>>,
14581448
) where
1459-
// The push bound lives at `logic`'s `give` call site, not here, so a caller can push
1460-
// borrowed records into a columnar builder that has no owned-tuple `Push`.
14611449
DCB: ContainerBuilder,
14621450
L: FnMut(
14631451
C::Key<'_>,
@@ -1742,24 +1730,17 @@ mod tests {
17421730
updates
17431731
}
17441732

1745-
/// A `Get -> ArrangeBy` chain carries the columnar arm end to end. A
1746-
/// non-identity MFP drives `as_collection_core` down its columnar producer
1747-
/// path, and feeding that edge into the arrange input keeps the columnar
1748-
/// passthrough, so no `ColumnarToVec` sits on the arrange path.
1749-
///
1750-
/// The producer output is checked against the projected input. Arrange
1751-
/// correctness itself is covered by `arrange_collection_arms_agree`; here we
1752-
/// only assert the variant survives the hand-off.
1733+
/// Arrange correctness itself is covered by `arrange_collection_arms_agree`; this only
1734+
/// asserts the columnar variant survives the hand-off.
17531735
#[mz_ore::test]
17541736
fn get_arrange_by_carries_columnar_end_to_end() {
17551737
let rows = vec![
17561738
(Row::pack_slice(&[Datum::Int64(1), Datum::Int64(10)]), 0u64),
17571739
(Row::pack_slice(&[Datum::Int64(2), Datum::Int64(20)]), 1),
17581740
(Row::pack_slice(&[Datum::Int64(1), Datum::Int64(10)]), 1),
17591741
];
1760-
// Project away column 1; the output row carries only column 0. A
1761-
// projection is non-identity, so `as_collection_core` takes the columnar
1762-
// producer path rather than the identity passthrough.
1742+
// A projection is non-identity, so `as_collection_core` takes the producer path
1743+
// rather than the identity passthrough.
17631744
let mfp = MapFilterProject::<LirScalarExpr>::new(2)
17641745
.project(vec![0])
17651746
.into_plan()
@@ -1783,8 +1764,6 @@ mod tests {
17831764
let (edge, _errs) =
17841765
bundle.as_collection_core(mfp, None, Antichain::new(), &config_set);
17851766
let producer_is_columnar = matches!(edge, CollectionEdge::Columnar(_));
1786-
// Tee the producer output for a content check, then feed the
1787-
// original edge into the arrange input.
17881767
let produced = edge.clone().into_vec().inner.capture();
17891768
let (_arranged, _arrange_errs, passthrough) =
17901769
CollectionBundle::<Timestamp>::arrange_collection(

0 commit comments

Comments
 (0)