Find a starting arrangement for a linear join

[frankmcsherry]

This PR determines and implements an appropriate starting arrangement for a linear join, as opposed to using either the arrangement on no columns or implementing no arrangement. Previously, the first input would announce [] as its keys, and we would (incorrectly) use such an arrangement if we had one. Otherwise, and more commonly, the logic would announce no keys and form no arrangements.

The modified logic looks at the keys of the second input collection, and attempts to localize them all to the first input collection. These are then implemented as an arrangement, as to the best of our understanding they will have to be so implemented by rendering in any case. Better to do it here, as the arrangements can then be identified as common expressions and re-used across multiple joins.

This PR partially addresses an arrangement re-use issue found in left joins, where the left input could be reused, in principle, but would not be reused in practice because we would not pre-form and share the arrangements.

Motivation

Tips for reviewer

Checklist

• This PR has adequate test coverage / QA involvement has been duly considered.

• This PR evolves an existing $T ⇔ Proto$T mapping (possibly in a backwards-incompatible way) and therefore is tagged with a T-proto label.

• This PR includes the following user-facing behavior changes:

[frankmcsherry]

The failures all appear to be explain output, which I suspect we can update and then inspect visually. Probably a lot of cases of newly arranged inputs where there would be one constructed anyhow, and if that's the only fall out, seems better to be clear than not. Ofc, could also be big swings in plans for other reasons.

[ggevay]

A plan change that you might not like so much is that all 2-input joins will turn into Delta joins, no? Because we will put an ArrangeBy on both inputs in the first run of JoinImplementation, and then the next run will be like, great, everything is arranged in just the right way, let's do a delta join!

Do you think we should make some effort to avoid this? I'll try to list the pros and cons of Delta and Differential joins for the 2-input case:

• pro Delta:
  • (The usual Delta join advantage of avoiding arrangement creation for intermediate results is not present in the 2-input case.)
  • A Delta join might still have an edge in some cases for one-off queries and initial snapshots, because of Delta join start-up performance improvement #6856
• pro Differential:
  • Zig-zag join, i.e., avoiding a scan over any of the inputs in some cases. Does this actually negate the advantage of Delta join start-up performance improvement #6856 in all cases (with only 2 inputs)?
  • Simpler dataflow.

A simple fix would be to have an extra if statement inputs.len() == 2 to force a Differential join in the 2-input case? I know that we don't like special cases, but I would argue that this special casing would make some sense: Without cost-based optimization that would be counting arrangements, we need to somehow explicitly make the Optimizer know that the usual Delta join advantage of avoiding intermediate arrangements is not happening for 2 inputs.

[frankmcsherry]

I think the better fix, if we want it, is indeed forcing all two input delta joins to be differential joins! If that would cut down on plan churn here we could do that in this PR, but we could also follow up with that as it is a separate change (and we might want a separate node in the history explaining the extent of the change). Wdyt?

[ggevay]

I'd say it's fine to do it in this PR. The PR's code changes are not big at all. Also, a detailed commit msg and a code comment could explain the 2-input special casing.

[frankmcsherry]

I think it may be non-trivial, as there are two-input delta joins we want to keep: those for which there do not exist matching arrangements. The current moment where we select between the two doesn’t have enough information to know this. No rush to land this so we can talk it out; maybe there is a smart fix.

…

Sent from my iPhone

On Nov 16, 2022, at 06:46, Gábor E. Gévay ***@***.***> wrote:  I'd say it's fine to do it in this PR. The PR's code changes are not big at all. Also, a detailed commit msg and a code comment could explain the 2-input special casing. — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you authored the thread.

[ggevay]

for which there do not exist matching arrangements

The "not" here is a typo? So you mean that maybe the user explicitly created some index because she knows that for some reason Delta would work better in a specific case?

[frankmcsherry]

Not a typo! If I join on a and b, and have an index on a for the first input and one on b for the second input, I could apply a delta join without a new arrangement, but I could not do so for a differential join. Perhaps we don’t prefer the delta join in this case, but the option exists Edit from ggevay: The following is meant here (I think): Let's say we are doing the following join: `FROM t1, t2 WHERE t1.a = t2.a AND t1.b = t2.b`. We have an index on `a` for t1 and an index on `b` for t2. In this case, a Differential join can't do anything with the existing indexes, because it would need either an index on `a` for both inputs, or an index on `b` for both inputs. On the other hand, there is a tricky way to apply a Delta join: the first path streams t1's index, does index lookup with `t1.b = t2.b` in t2's index, the second path streams t2's index, does index lookup with `t1.a = t2.a` in t1's index. Additionally, both paths apply `t1.a = t2.a AND t1.b = t2.b` at the end of the path as an MFP.

…

Sent from my iPhone

On Nov 16, 2022, at 08:27, Gábor E. Gévay ***@***.***> wrote:  for which there do not exist matching arrangements The "not" here is a typo? So you mean that maybe the user explicitly created some index because she knows that for some reason Delta would work better in a specific case? — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you authored the thread.

[frankmcsherry]

Hi folks! After a long battle with explain outputs, this is probably ready for review. The commits that say nothing about explain outputs are the ones worth looking at, and they are pretty concise. The remaining diffs are mostly indentation changes, and adding ArrangeBy to things that require arrangements at render time.

[philip-stoev]

Item No 1 . This query:

CREATE TABLE t1 (f1 DOUBLE PRECISION, f2 DOUBLE PRECISION NOT NULL);

CREATE TABLE t2 (f1 DOUBLE PRECISION, f2 DOUBLE PRECISION NOT NULL);

EXPLAIN
SELECT *
FROM (SELECT MIN (f1) AS f1 FROM t2) AS a1
JOIN (SELECT f1, COUNT (  * ) FROM t1 GROUP  BY  1) AS a2
ON (a1.f1 = a2.f1 );

succeeds in this branch but panics on the prior commit as follows:

thread 'coordinator' panicked at 'assertion failed: shuffle[proj] < new_input_arity + map.len()', src/expr/src/linear.rs:834:13
stack backtrace:
   0: rust_begin_unwind
             at /rustc/897e37553bba8b42751c67658967889d11ecd120/library/std/src/panicking.rs:584:5
   1: core::panicking::panic_fmt
             at /rustc/897e37553bba8b42751c67658967889d11ecd120/library/core/src/panicking.rs:142:14
   2: core::panicking::panic
             at /rustc/897e37553bba8b42751c67658967889d11ecd120/library/core/src/panicking.rs:48:5
   3: mz_expr::linear::MapFilterProject::permute
   4: mz_compute_client::plan::join::JoinClosure::build
   5: mz_compute_client::plan::join::JoinBuildState::extract_closure
   6: mz_compute_client::plan::join::JoinBuildState::add_columns
   7: mz_compute_client::plan::join::linear_join::LinearJoinPlan::create_from
   8: mz_compute_client::plan::Plan<T>::from_mir_stack_safe
   9: mz_compute_client::plan::Plan<T>::from_mir_inner::{{closure}}
  10: mz_compute_client::plan::Plan<T>::from_mir_inner
  11: mz_compute_client::plan::Plan<T>::from_mir
  12: mz_compute_client::plan::Plan<T>::finalize_dataflow
  13: mz_adapter::coord::sequencer::<impl mz_adapter::coord::Coordinator<S>>::sequence_explain_plan::{{closure}}
  14: tracing_core::dispatcher::with_default

Is this expected? If yes, can we add this as an .slt test case? If not related to this branch, I will file a separate ticket. Please advise.

[frankmcsherry]

Yes, that crash looks like the behavior that this references

Previously, the first input would announce [] as its keys, and we would (incorrectly) use such an arrangement if we had one.

[frankmcsherry]

Or rather, it is hard to know without cracking open the query. But one defect that was fixed was the incorrect use of [] as the appropriate arrangement for the first input to a join, and it presented with the same error. If this PR happens to fix a related lingering error on main of the same flavor, great! (recently the code started to trust its input about which arrangements to use more, and this has surfaced cases where the inputs were not correct).

I'm not going to mess with CI any more for this PR, but we can add it later if you like!

[philip-stoev]

From the randomized testing:

• I used 2 grammars, one which creates TPC-*-like 3-way joins and another which creates deeply nested derived tables with aggregates and other ornaments
• No wrong results in this branch
• No panics specific to this branch (2 panics in main, one is Item No 1 the other one is filed separately)
• The plan changes all belong to the two groups already seen in the SLT and under discussion:
  • extra Arrangement node in plan
  • differential vs. delta join

[ggevay]

Looks good!

Btw. the current MIR explain output is not showing start_keys. I’ll open a separate PR for that.

[frankmcsherry]

Thanks folks; I appreciate the reviews and the patience!