opt: don't drop LeftJoin filter during join ordering

[DrewKimball]

opt: use RelExpr instead of ColumnID for reorderjoins relation map

The reorderjoins opttester directive previously maintained a map from
the first column ID of each base relation to the relation's label in
the output. This could cause a panic for relations that didn't output
any columns. This patch changes the map to use the relations themselves
as keys, which prevents the panic.

opt: don't drop LeftJoin filter during join ordering

This patch fixes a bug in the join reordering logic that can lead to
incorrect results due to a dropped filter and incorrect conversion of
a left join to an inner join. The bug can occur when the join tree
contains an inner join with a left join as an input, where the inner
join has two separate conjuncts in its ON condition that reference
both inputs of the left join. Additionally, the inner join filters
must not filter NULL values from the right side of the left join
(or alternatively null-rejection rules must be disabled).

The incorrect transformation looks something like this:

(INNER JOIN xy (LEFT JOIN ab (INNER JOIN uv wz ON v = w) ON b = v) ON a = x AND u = x)

=>

(INNER JOIN ab (INNER JOIN xy (INNER JOIN uv wz ON v = w) ON u = x) ON a = x)

Notice how xy has been "pushed" into the right side of the left
join and the left join's b = v filter (and the left join itself)
dropped in the process.

To understand what causes the bug, it is necessary to understand three
points about the join reordering algorithm:

1. Cross products are never introduced in the enumerated plans. So, for
   two sub-plans, a join is only considered between them if there is an
   applicable edge between those sub-plans.
2. The original paper associates each join with exactly one edge in the
   hypergraph that encodes "reorderability" properties.
3. The JoinOrderBuilder departs from the paper by associating each
   inner join conjunct with a hypergraph edge. This allows each
   conjunct to be independently reordered from the others. See the
   Special handling of inner joins section in the JoinOrderBuilder
   comment for more details.

(1) combined with (2) implies that a reordered join tree is only
considered if every edge in the hypergraph could be applied to form joins
in the join tree. This allows the original algorithm to prevent invalid
orderings by making just a single edge inapplicable. However, because
of (3) the same is no longer true for the JoinOrderBuilder. In the
example given above, the left join fails the applicability check,
indicating an invalid plan. However, the inner join's a = x filter
passes the check and ends up replacing the left join. This prevents
the the check in (1) from catching the invalid plan.

This patch fixes the bug by keeping track of the edges that should
be applied somewhere in each join tree based on the TES of each edge.
This is then compared against the actual edges that are applied in
the construction of the join tree. If the edge sets aren't equal,
the plan is invalid and cannot be added to the memo. This allows the
JoinOrderBuilder to recover the property that an inapplicable edge
invalidates an enumerated plan.

Fixes #90761

Release note (bug fix): Fixed a bug existing since 20.2 that could
cause incorrect results in rare cases for queries with inner joins
and left joins. For the bug to occur, the left join had to be in
the input of the inner join and the inner join filters had to
reference both inputs of the left join, and not filter NULL values
from the right input of the left join. Additionally, the right input
of the left join had to contain at least one join, with one input not
referenced by the left join's ON condition.

[cockroach-teamcity]

This change is 

[msirek]

Nice fix! I tried to poke holes in it, but it seems consistent with the paper. The key rule which must hold true seems to be the isSubsetOf check in setApplicableEdges, which determines when an edge must be consumed by a given join tree of vertexes:

cockroach/pkg/sql/opt/xform/join_order_builder.go

Lines 588 to 596 in 59eea8f

	// setApplicableEdges initializes applicableEdges with all edges that must show
	// up in any join tree that is constructed for the given set of vertexes. See
	// checkAppliedEdges for how this information is used.
	func (jb *JoinOrderBuilder) setApplicableEdges(s vertexSet) {
	applicableEdges := edgeSet{}
	for i := range jb.edges {
	if jb.edges[i].tes.isSubsetOf(s) {
	applicableEdges.Add(i)
	}

Since the tes is updated from the conflict detection rules, it seems like at least one combination of left and right subsets of vertexes must always be legal, so we'll never run into a case where we have applicable edges for a join tree, but could not build joins for that join tree.

Reviewed 2 of 2 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @DrewKimball, @mgartner, and @rytaft)

---

pkg/sql/opt/xform/join_order_builder.go line 873 at r2 (raw file):

			panic(errors.AssertionFailedf("expected existing join plan"))
		}
		jb.addToGroup(op, right, left, joinFilters, selectFilters, jb.plans[union])

I couldn't find anything wrong with the fix, but I noticed that when the non-commuted join is redundant we may attempt to add the commuted join to the memo multiple times. A possible improvement could be to check the number of group members when the join is redundant, and don't attempt to add the commuted join again if there are already 2 group members.

Another alternative if to make jb.addToGroup return a boolean indicating whether or not it added a new join to the group (or found a cached match in the memo). If none was added, then bypass the call to jb.callOnAddJoinFunc since we're not actually adding anything to the memo.

[DrewKimball]

Thanks for taking the time to be so thorough, I appreciate it!

which determines when an edge must be consumed by a given join tree of vertexes

I like that phrasing, I'll have to think about whether I can incorporate it into the documentation somehow.

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @mgartner, @msirek, and @rytaft)

---

pkg/sql/opt/xform/join_order_builder.go line 873 at r2 (raw file):

Previously, msirek (Mark Sirek) wrote…

I couldn't find anything wrong with the fix, but I noticed that when the non-commuted join is redundant we may attempt to add the commuted join to the memo multiple times. A possible improvement could be to check the number of group members when the join is redundant, and don't attempt to add the commuted join again if there are already 2 group members.

Another alternative if to make jb.addToGroup return a boolean indicating whether or not it added a new join to the group (or found a cached match in the memo). If none was added, then bypass the call to jb.callOnAddJoinFunc since we're not actually adding anything to the memo.

When you say the commuted join would be added multiple times, do you mean when successive ancestor joins are matched? For a given join that's being reordered I wouldn't expect the same join (reordered or otherwise) to be added to the memo more than once. Or do you just mean in the output of the reorderjoins test directive?

[msirek]

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @DrewKimball, @mgartner, and @rytaft)

---

pkg/sql/opt/xform/join_order_builder.go line 873 at r2 (raw file):

Previously, DrewKimball (Drew Kimball) wrote…

When you say the commuted join would be added multiple times, do you mean when successive ancestor joins are matched? For a given join that's being reordered I wouldn't expect the same join (reordered or otherwise) to be added to the memo more than once. Or do you just mean in the output of the reorderjoins test directive?

I didn't trace out exactly when the attempt to add the duplicate join was made, but I'm guessing it's when 2 given vertexes that were in a previously-planned subset of vertexes are also in the current subset. Here's the test case I was looking at:

CREATE TABLE t (a INT, b INT, c INT, d INT, PRIMARY KEY(a));

SELECT *
FROM t AS t1
JOIN t AS t2
  LEFT JOIN (t AS t3 JOIN t as t6 ON true LEFT JOIN t as t7 ON t7.a = t3.c and t6.d = t3.d)
    JOIN t AS t4
      JOIN t AS t5
      ON true
    ON t4.a = t3.a AND t5.d = t3.d
  ON t2.b > t4.b
ON t1.a = t4.a AND t1.c = t2.c AND t5.b = t2.c AND t2.d = t3.d
;

[DrewKimball]

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @mgartner, @msirek, and @rytaft)

---

pkg/sql/opt/xform/join_order_builder.go line 873 at r2 (raw file):

Previously, msirek (Mark Sirek) wrote…

I didn't trace out exactly when the attempt to add the duplicate join was made, but I'm guessing it's when 2 given vertexes that were in a previously-planned subset of vertexes are also in the current subset. Here's the test case I was looking at:

CREATE TABLE t (a INT, b INT, c INT, d INT, PRIMARY KEY(a));

SELECT *
FROM t AS t1
JOIN t AS t2
  LEFT JOIN (t AS t3 JOIN t as t6 ON true LEFT JOIN t as t7 ON t7.a = t3.c and t6.d = t3.d)
    JOIN t AS t4
      JOIN t AS t5
      ON true
    ON t4.a = t3.a AND t5.d = t3.d
  ON t2.b > t4.b
ON t1.a = t4.a AND t1.c = t2.c AND t5.b = t2.c AND t2.d = t3.d
;

Oh, I see what you mean now, that's a good point. The commuted version of each descendent join should have already been handled by the time we get to an ancestor join. I think it goes even further - if a sub-plan only contains vertexes that were in a descendent join, all possible reorderings for that set should already have been introduced. I think we could avoid the redundancy by just skipping those vertex sets for which the original join tree had sub-plans.

[rytaft]

great find!

Reviewed 2 of 2 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: complete! 2 of 0 LGTMs obtained (waiting on @DrewKimball, @mgartner, and @msirek)

---

pkg/sql/opt/xform/testdata/rules/join_order line 2967 at r2 (raw file):

# The 't2.b > t4.b' filter should not be dropped.
reorderjoins disable=RejectNullsLeftJoin

Is it possible to also construct a test case where you don't need to disable any rules? I wonder if this bug can actually appear in practice?

[DrewKimball]

Reviewable status: complete! 2 of 0 LGTMs obtained (waiting on @mgartner, @msirek, and @rytaft)

---

pkg/sql/opt/xform/testdata/rules/join_order line 2967 at r2 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

Is it possible to also construct a test case where you don't need to disable any rules? I wonder if this bug can actually appear in practice?

I'll see if I can make it happen.

[DrewKimball]

Reviewable status: complete! 2 of 0 LGTMs obtained (waiting on @mgartner, @msirek, and @rytaft)

---

pkg/sql/opt/xform/testdata/rules/join_order line 2967 at r2 (raw file):

Previously, DrewKimball (Drew Kimball) wrote…

I'll see if I can make it happen.

Adding an OR t4.a IS NULL filter does it:

# The 't2.b > t4.b' filter should not be dropped.
opt format=hide-all
SELECT 1
FROM t90761 AS t1
JOIN t90761 AS t2
  LEFT JOIN t90761 AS t3
    JOIN t90761 AS t4 ON true
  ON t2.b > t4.b
ON (t1.a = t4.a OR t4.a IS NULL) AND t1.c = t2.c;
----
project
 ├── inner-join (cross)
 │    ├── inner-join (cross)
 │    │    ├── inner-join (hash)
 │    │    │    ├── scan t90761 [as=t1]
 │    │    │    ├── scan t90761 [as=t2]
 │    │    │    └── filters
 │    │    │         └── t1.c = t2.c
 │    │    ├── scan t90761 [as=t4]
 │    │    └── filters
 │    │         └── (t1.a = t4.a) OR (t4.a IS NULL)
 │    ├── scan t90761 [as=t3]
 │    └── filters (true)
 └── projections
      └── 1

Do you think I should add this case?

[DrewKimball]

Reviewable status: complete! 2 of 0 LGTMs obtained (waiting on @mgartner, @msirek, and @rytaft)

---

pkg/sql/opt/xform/testdata/rules/join_order line 2967 at r2 (raw file):

Previously, DrewKimball (Drew Kimball) wrote…

Adding an OR t4.a IS NULL filter does it:

# The 't2.b > t4.b' filter should not be dropped.
opt format=hide-all
SELECT 1
FROM t90761 AS t1
JOIN t90761 AS t2
  LEFT JOIN t90761 AS t3
    JOIN t90761 AS t4 ON true
  ON t2.b > t4.b
ON (t1.a = t4.a OR t4.a IS NULL) AND t1.c = t2.c;
----
project
 ├── inner-join (cross)
 │    ├── inner-join (cross)
 │    │    ├── inner-join (hash)
 │    │    │    ├── scan t90761 [as=t1]
 │    │    │    ├── scan t90761 [as=t2]
 │    │    │    └── filters
 │    │    │         └── t1.c = t2.c
 │    │    ├── scan t90761 [as=t4]
 │    │    └── filters
 │    │         └── (t1.a = t4.a) OR (t4.a IS NULL)
 │    ├── scan t90761 [as=t3]
 │    └── filters (true)
 └── projections
      └── 1

Do you think I should add this case?

Nevermind, added it since I had to rebase anyway.

[DrewKimball]

TFTRs!

bors r+

[craig]

Build failed (retrying...):

• Bazel Essential CI (Cockroach)

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)

[blathers-crl]

Encountered an error creating backports. Some common things that can go wrong:

1. The backport branch might have already existed.
2. There was a merge conflict.
3. The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.

---

error creating merge commit from e8560b1 to blathers/backport-release-22.1-91425: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 22.1.x failed. See errors above.

---

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.}