Add Threshold/Distinct variants maintaining retractions

[antiguru]

Fixes MaterializeInc/database-issues#2266.

Reduction operators could provide an optimization by not maintaining their output but rather a smaller set of data needed to compute their correct outputs.

Pull-up the CollectionBundle type during rendering a reduction dataflow. This would permit reductions to return collections instead of arrangements if they do not maintain their outputs.

[antiguru]

Pointing out a potential diff in behavior: Previously, all arrangements would be produced with a specific key_arity. With this change, that's only the case for reductions maintaining their outputs. Reductions returning a collection would not have the same bundle.

[frankmcsherry]

Can you talk me through this enum? A CollectionBundle can represent a single arrangement, though I suppose it has to have errors with it?

[frankmcsherry]

If this is just meant to be a minor local enum for the purposes of simplifying some reduce rendering, that's no worries, but I would put some doc comments on it explaining what the goal is (e.g. why not make it public and ship it to others, that the two variants reflect errors in one case and not the other, idk stuff like that.

[antiguru]

That's a good point, the current type could be confusing. A CollectionBundle itself might contain an arrangement. What I really intended here was to give reductions a choice to either return an arrangement or two collections. I'll see if I can change it to be clearer.

[frankmcsherry]

Pondering a bit: if it is just to make the match for reduce rendering easier, no worries but maybe doc as such. If it is meant to be a re-usable abstraction, we should workshop further.

[antiguru]

I changed the type to contain a single Collection instead of a CollectionBundle. This a) doesn't require it to mess with the err_collection, and b) it's leaner and easier to understand.

[frankmcsherry]

Seems directionally good. I left a few comments, then hit the review button, sorry.

The enum ArrangementOrBundle that might be local probably wants to be local, or wants for us to clean it up a bit. One option could be to take the error stream earlier, and require that all methods create a CollectionBundle.

I think we probably want to keep each variant of the distinct and threshold methods around, and lift the decision of which to call up to the plan. This is partly because we'd like to punt that sort of decision making upwards, and commit the rendering to specific and visible behavior (e.g., the plan should reveal which operators will make which arrangements).

[frankmcsherry]

This is a bit weird that for one of the variants it ignores the err_input argument. I worry that it will result in some future bug where someone imagines that it will concat that input in rather than drop it when you have an -Arrangement.

[antiguru]

Fixed, err_input is now used for both variants.

[frankmcsherry]

We can discuss further, but I think we probably want to keep both options around and allow the planner to distinguish (from prior experiences where different users want different things, and its easier to flip flop a plan around than the rendering behavior).

[antiguru]

Both variants are around now and can be selected through the plan.

[frankmcsherry]

Same comment as with distinct: given that these are not massive, we'd probably like both implementations and an enum in the plan that allows the planner to toggle between the two as it sees fit (and then we could determine the best defaults).

[antiguru]

Same here: both variants are around now and can be selected through the plan.

[frankmcsherry]

These look good to me. It does call out that we'll have to learn how to drive and test things that the coordinator does not produce, but I think we are on the path to that.

[antiguru]

Thanks for the review! I added more documentation.