opt: map and push down equality conditions

[rytaft]

This commit adds a new normalization rule to enable pushing variable
equality conditions such as a.x=b.x through joins.

For example, consider this query:

SELECT * FROM a, b, c WHERE a.x=b.x AND a.x=c.x

Given join ordering (a join (b join c)), it should be possible to infer the
filter b.x=c.x and push it down from the top level onto the join (b join c).
This commit enables that mapping and pushdown to happen.

In addition, this commit updates the AssociateJoin rule to map as many
equality conditions as possible to use the output columns of the new inner-most
join, allowing those conditions to be pushed onto that join.

For example, consider this query:

SELECT * FROM a, b, c WHERE a.x=b.x AND b.x=c.x

If the AssociateJoin rule creates a new join ordering (b join (a join c)),
it should be possible to map a.x=b.x to a.x=c.x and add it onto the new
inner-most join (a join c). This commit enables that mapping to happen.

Fixes #38716
Fixes #36226

Release note (performance improvement): Improved performance for some join
queries due to improved filter inference during planning.
Release justification: This commit will not be merged before the release
branch is cut.

[cockroach-teamcity]

This change is 

[justinj]

Nice! , this was a bit tricky to wrap my head around but once I understood it it's very nice and clean!

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

---

pkg/sql/opt/norm/join.go, line 181 at r1 (raw file):

	for i := range filters {
		fd := &filters[i].ScalarProps(c.mem).FuncDeps
		filterEqCols := fd.ComputeEquivClosure(fd.EquivReps())

Is there a case today in which this does not correspond to just a pair of columns being equated?

---

pkg/sql/opt/norm/join.go, line 278 at r1 (raw file):

	filters memo.FiltersExpr, src *memo.FiltersItem, dst memo.RelExpr,
) opt.ScalarExpr {
	// Fast path if src is already bound by dst.

can this be easily unified with CanMapJoinOpEquality?

---

pkg/sql/opt/norm/join.go, line 366 at r1 (raw file):

		if !scalarProps.OuterCols.Contains(col) {
			// Filter does not contain col.
			eqCols = scalarProps.FuncDeps.ComputeEquivClosure(eqCols)

Can't this still miss some, if we have a case like:

col: x
filter: x = y AND w = z AND y = w

?
(assuming I understood the problem that prompted this change correctly)

[justinj]

oops, only looked at first commit

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

[justinj]

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

---

pkg/sql/opt/norm/join.go, line 331 at r2 (raw file):

			(newFilters == nil && c.CanMapJoinOpEquality(filters, filter, dst)) {
			if newFilters == nil {
				// All right, it was necessary to allocate the filters after all.

lol, I like the resigned tone of this comment

---

pkg/sql/opt/norm/join.go, line 336 at r2 (raw file):

			}

			newFilters[i] = memo.FiltersItem{

Maybe I'm missing some important thing here, is it not possible for the replacement of the filter like this to cause us to lose some information? Like, I get if we were just appending this newly mapped filter we'd be fine, but couldn't replacing like this result in making the replacement no longer valid (since we use the filters themselves to justify the mapping)?

Like if we had something like:

a = b AND b = c AND c = d AND d = e AND e = f AND f = a

(a 6-cycle)
and then we replaced b = c => b = f and e = f => e = c, we no longer have an equivalent set of filters, we have two equivalence groups ({a,b,f} and {c,d,e}) rather than one ({a,b,c,d,e,f}).

[rytaft]

TFTR! Very thoughtful comments.

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @justinj, @RaduBerinde, and @rytaft)

---

pkg/sql/opt/norm/join.go, line 181 at r1 (raw file):

Previously, justinj (Justin Jaffray) wrote…

Is there a case today in which this does not correspond to just a pair of columns being equated?

I don't think so -- in logical_props_builder.go we only add equivalencies for filters that are variable equalities.

I did it this way since it seemed like the easiest way to extract the equivalent columns and remove them from eqCols.

---

pkg/sql/opt/norm/join.go, line 278 at r1 (raw file):

Previously, justinj (Justin Jaffray) wrote…

can this be easily unified with CanMapJoinOpEquality?

Not sure if I understand the question, but are you asking whether we can get rid of the fast path here by returning false from CanMapJoinOpEquality?

The problem is that we need CanMapJoinOpEquality to return true for this fast path so that the rule PushEqualityIntoJoinLeftAndRight will fire when appropriate. For example, suppose we have the following query: SELECT * from a, b WHERE a.x = a.y AND a.x = b.x AND a.y = b.y;. PushEqualityIntoJoinLeftAndRight has to fire to prevent MapEqualityIntoJoinLeft and MapEqualityIntoJoinRight from cycling with each other. They will infinitely loop mapping a.x=a.y to b.x=b.y and vice-versa.

I added a note above PushEqualityIntoJoinLeftAndRight to emphasize that it has to be first (this is the same pattern we used for the existing filter push down rules).

---

pkg/sql/opt/norm/join.go, line 366 at r1 (raw file):

Previously, justinj (Justin Jaffray) wrote…

Can't this still miss some, if we have a case like:

col: x
filter: x = y AND w = z AND y = w

?
(assuming I understood the problem that prompted this change correctly)

Good point! Fixed.

---

pkg/sql/opt/norm/join.go, line 336 at r2 (raw file):

Previously, justinj (Justin Jaffray) wrote…

Maybe I'm missing some important thing here, is it not possible for the replacement of the filter like this to cause us to lose some information? Like, I get if we were just appending this newly mapped filter we'd be fine, but couldn't replacing like this result in making the replacement no longer valid (since we use the filters themselves to justify the mapping)?

Like if we had something like:

a = b AND b = c AND c = d AND d = e AND e = f AND f = a

(a 6-cycle)
and then we replaced b = c => b = f and e = f => e = c, we no longer have an equivalent set of filters, we have two equivalence groups ({a,b,f} and {c,d,e}) rather than one ({a,b,c,d,e,f}).

This situation is not possible because of the blocks in CanMapJoinOpEquality and MapJoinOpEquality that have the following comment:

// Remove from consideration equality columns that already have an equality
// condition bound by dst.

So in the example you gave, suppose that the columns in dst are {a,b,f}, and we want to try to map b=c to use {a,b,f}. Since a=b and f=a are filters already bound by dst, all three columns (a, b and f) will be removed from consideration for the new equality condition. Therefore, it won't be possible to map to b=f.

That being said, I agree there's something about this that feels slightly weird. Another way to think about this problem is that if every column in the equivalence group is a node in a graph, we want to find the minimum spanning tree for the graph, where edges between nodes in dst have lower weight. I'm not sure if formulating it in this way makes it easier/cleaner to solve, but I'll spend some time thinking about it.

One thing I do like about this solution, though, is that it reuses the code from the normalization rules, and the normalization rules follow the same pattern we already used for filter push down.

Anyway, let me know if you have an idea about how to make this better!

[justinj]

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @RaduBerinde and @rytaft)

---

pkg/sql/opt/norm/join.go, line 181 at r1 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

I don't think so -- in logical_props_builder.go we only add equivalencies for filters that are variable equalities.

I did it this way since it seemed like the easiest way to extract the equivalent columns and remove them from eqCols.

Makes sense! Just wanted to make sure I was understanding right

---

pkg/sql/opt/norm/join.go, line 278 at r1 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

Not sure if I understand the question, but are you asking whether we can get rid of the fast path here by returning false from CanMapJoinOpEquality?

The problem is that we need CanMapJoinOpEquality to return true for this fast path so that the rule PushEqualityIntoJoinLeftAndRight will fire when appropriate. For example, suppose we have the following query: SELECT * from a, b WHERE a.x = a.y AND a.x = b.x AND a.y = b.y;. PushEqualityIntoJoinLeftAndRight has to fire to prevent MapEqualityIntoJoinLeft and MapEqualityIntoJoinRight from cycling with each other. They will infinitely loop mapping a.x=a.y to b.x=b.y and vice-versa.

I added a note above PushEqualityIntoJoinLeftAndRight to emphasize that it has to be first (this is the same pattern we used for the existing filter push down rules).

Sorry, was a little too terse, I meant these couple blocks of code seem very similar to the blocks in CanMapJoinOpEquality and it feels like maybe there's an extraction possible. If not that's fine!

---

pkg/sql/opt/norm/join.go, line 336 at r2 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

This situation is not possible because of the blocks in CanMapJoinOpEquality and MapJoinOpEquality that have the following comment:

// Remove from consideration equality columns that already have an equality
// condition bound by dst.

So in the example you gave, suppose that the columns in dst are {a,b,f}, and we want to try to map b=c to use {a,b,f}. Since a=b and f=a are filters already bound by dst, all three columns (a, b and f) will be removed from consideration for the new equality condition. Therefore, it won't be possible to map to b=f.

That being said, I agree there's something about this that feels slightly weird. Another way to think about this problem is that if every column in the equivalence group is a node in a graph, we want to find the minimum spanning tree for the graph, where edges between nodes in dst have lower weight. I'm not sure if formulating it in this way makes it easier/cleaner to solve, but I'll spend some time thinking about it.

One thing I do like about this solution, though, is that it reuses the code from the normalization rules, and the normalization rules follow the same pattern we already used for filter push down.

Anyway, let me know if you have an idea about how to make this better!

Talked about this offline

[rytaft]

I've completely changed this to use a minimum-spanning tree approach as Justin and I discussed offline. PTAL!

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @RaduBerinde and @rytaft)

[RaduBerinde]

Very cool! Some minor comments.

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @RaduBerinde and @rytaft)

---

pkg/sql/opt/norm/join.go, line 122 at r3 (raw file):

//
// If src has a correlated subquery, CanMapJoinOpFilter returns false.
func (c *CustomFuncs) CanMapJoinOpFilter(

we should move this so it stays close to MapJoinOpFilter

---

pkg/sql/opt/norm/join.go, line 171 at r3 (raw file):

	// If more than one equality condition connecting columns in the equivalence
	// group spans both sides of the join, these conditions can be remapped.
	found := false

[nit] would be more suggestive if we used found := 0 and then found++; if found > 1 { return true }

---

pkg/sql/opt/norm/join.go, line 195 at r3 (raw file):

// containing col that forms an equivalence group in filters. Then it
// (conceptually) constructs a graph in which the nodes represent columns and
// the edges represent equality conditions between the columns. The goal is to

This is a bit confusing. There is the graph that represents the given equality conditions (the first diagram below), and then there's the conceptual complete graph where there's an edge between any pair of columns in the equivalency group. We are talking about a spanning tree of the latter, not the former. The former graph isn't important really, I don't think we should draw it.

What does "minimum" spanning tree mean here? Sounds like we're looking for any spanning tree with a single "crossing" edge. I get that the "restriction" can be thought of as a per-edge cost but we should use either one definition or the other.

We may be complicating things unnecessarily by talking about graphs and spanning trees. We could just say that we have a set of columns that are all equivalent, some on the left side and some on the right side. We construct a new set of equalities that implies the same equivalency group, with the property that there is a single condition with one left column and one right column.

---

pkg/sql/opt/norm/join.go, line 246 at r3 (raw file):

		))
	}
	for col, ok := leftEqCols.Next(prevCol + 1); ok; col, ok = leftEqCols.Next(col + 1) {

Is it important here how we "connect" them? It would be more intuitive (and easier to descripbe) to just pick an arbitrary column on each side and make everything on that side equal to that column. Eg if we have l1, l2, l3 and r1, r2, r3, we could pick l1 and r2 and generate l1 = r2 plus l1 = l2, l1 = l3 plus r1 = r2, r1 = r3.

---

pkg/sql/opt/norm/join.go, line 252 at r3 (raw file):

		prevCol = col
	}
	for col, ok := rightEqCols.Next(0); ok; col, ok = rightEqCols.Next(col + 1) {

It's subtle that the "crossing" condition happens here implicitly, should add a comment.

[rytaft]

TFTR! Updated.

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @justinj and @rytaft)

---

pkg/sql/opt/norm/join.go, line 278 at r1 (raw file):

Previously, justinj (Justin Jaffray) wrote…

Sorry, was a little too terse, I meant these couple blocks of code seem very similar to the blocks in CanMapJoinOpEquality and it feels like maybe there's an extraction possible. If not that's fine!

This code is different now.

---

pkg/sql/opt/norm/join.go, line 122 at r3 (raw file):

Previously, RaduBerinde wrote…

we should move this so it stays close to MapJoinOpFilter

Done.

---

pkg/sql/opt/norm/join.go, line 171 at r3 (raw file):

Previously, RaduBerinde wrote…

[nit] would be more suggestive if we used found := 0 and then found++; if found > 1 { return true }

Done.

---

pkg/sql/opt/norm/join.go, line 195 at r3 (raw file):

Previously, RaduBerinde wrote…

This is a bit confusing. There is the graph that represents the given equality conditions (the first diagram below), and then there's the conceptual complete graph where there's an edge between any pair of columns in the equivalency group. We are talking about a spanning tree of the latter, not the former. The former graph isn't important really, I don't think we should draw it.

What does "minimum" spanning tree mean here? Sounds like we're looking for any spanning tree with a single "crossing" edge. I get that the "restriction" can be thought of as a per-edge cost but we should use either one definition or the other.

We may be complicating things unnecessarily by talking about graphs and spanning trees. We could just say that we have a set of columns that are all equivalent, some on the left side and some on the right side. We construct a new set of equalities that implies the same equivalency group, with the property that there is a single condition with one left column and one right column.

Done.

---

pkg/sql/opt/norm/join.go, line 246 at r3 (raw file):

Previously, RaduBerinde wrote…

Is it important here how we "connect" them? It would be more intuitive (and easier to descripbe) to just pick an arbitrary column on each side and make everything on that side equal to that column. Eg if we have l1, l2, l3 and r1, r2, r3, we could pick l1 and r2 and generate l1 = r2 plus l1 = l2, l1 = l3 plus r1 = r2, r1 = r3.

Done.

---

pkg/sql/opt/norm/join.go, line 252 at r3 (raw file):

Previously, RaduBerinde wrote…

It's subtle that the "crossing" condition happens here implicitly, should add a comment.

Done.

[justinj]

:

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

---

pkg/sql/opt/norm/join.go, line 163 at r4 (raw file):

//   SELECT * FROM a, b WHERE a.x = a.y AND b.x = b.y AND a.x = b.x
//
func (c *CustomFuncs) MapJoinOpEqualities(

Nice, this function is super clear now 👍

[RaduBerinde]

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

---

pkg/sql/opt/norm/rules/join.opt, line 179 at r4 (raw file):

        ...
        $item:(FiltersItem (Eq (Variable $col:*) (Variable))) &
            (CanMapJoinOpEqualities $on $col $leftCols:(OutputCols $left) $rightCols:(OutputCols $right))

If we have a lot of equalities, and we can't Map them, we will recompute this function for each item no? Could we just check it once on the entire $on? (I'd think the common case is to have at least one equality)

[rytaft]

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

---

pkg/sql/opt/norm/rules/join.opt, line 179 at r4 (raw file):

Previously, RaduBerinde wrote…

If we have a lot of equalities, and we can't Map them, we will recompute this function for each item no? Could we just check it once on the entire $on? (I'd think the common case is to have at least one equality)

Good point -- updated so now it just checks once per equivalence group rather than once per equality condition.

[rytaft]

bors r+

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

[craig]

Build failed (retrying...)

• GitHub CI (Cockroach)

[craig]

Build succeeded

• GitHub CI (Cockroach)