opt: change JoinMultiplicity from a Relational prop to a join field

[DrewKimball]

Previously, the JoinMultiplicity property was stored as a Relational
prop. This is a problem because all expressions in a memo group share
the same Relational props during exploration, and there are exploration
rules that can flip a join's left/right multiplicity.

This patch instead stores JoinMultiplcity as a join field that is
initialized during construction of the join. This fixes the shared
Relational props issue, and also makes it possible for JoinMultiplicity
to aid in calculating other logical properties.

Fixes #49821

Release note: None

[cockroach-teamcity]

This change is 

[DrewKimball]

I ended up just leaving the multiplicity output code (in expr_format) with the RuleProps, but I'm open to alternative suggestions. One possibility is to give JoinMultiplicity its own expr format flag.

[rytaft]

Nice! I think you should print out the UnfilteredCols property along with the other RuleProps. Otherwise just a couple of comments need updating.

Reviewed 21 of 21 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @DrewKimball)

---

pkg/sql/opt/memo/multiplicity_builder.go, line 83 at r1 (raw file):

}

// GetJoinMultiplicity recursively derives the UnfilteredCols field and

GetJoinMultiplicity -> deriveUnfilteredCols

---

pkg/sql/opt/memo/multiplicity_builder.go, line 84 at r1 (raw file):

// GetJoinMultiplicity recursively derives the UnfilteredCols field and
// populates the props.Relational.Rule.MultiplicityProps field as it goes to

this comment is out of date

---

pkg/sql/opt/memo/testdata/logprops/join, line 2201 at r1 (raw file):

# Regression test for #49821.
opt format=show-ruleprops

Since you're showing rule props you should print the unfiltered cols.

---

pkg/sql/opt/props/logical.go, line 343 at r1 (raw file):

		// cannot add any columns, as it may have filtered rows.
		//
		// UnfilteredCols is lazily populated by DeriveJoinMultiplicity. It is only

DeriveJoinMultiplicity -> GetJoinMultiplicityFromInputs

[DrewKimball]

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

---

pkg/sql/opt/memo/multiplicity_builder.go, line 83 at r1 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

GetJoinMultiplicity -> deriveUnfilteredCols

Done.

---

pkg/sql/opt/memo/multiplicity_builder.go, line 84 at r1 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

this comment is out of date

Done.

---

pkg/sql/opt/memo/testdata/logprops/join, line 2201 at r1 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

Since you're showing rule props you should print the unfiltered cols.

I was worried that this might lead to long output strings, but it actually turned out to be really compact. Done.

---

pkg/sql/opt/props/logical.go, line 343 at r1 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

DeriveJoinMultiplicity -> GetJoinMultiplicityFromInputs

Done.

[rytaft]

Reviewed 17 of 17 files at r2.
Reviewable status: complete! 1 of 0 LGTMs obtained

[RaduBerinde]

Nice!

One thing is that we should only need to derive unfiltered cols in those special cases where we have a self-join or a foreign key join. I realize that would make the logic even more complicated.. But we could at least exit early in some simple cases (no self-joins, and no foreign keys in the tables involved).

Reviewable status: complete! 2 of 0 LGTMs obtained (waiting on @DrewKimball)

---

pkg/sql/opt/memo/expr.go, line 452 at r2 (raw file):

// supported (currently InnerJoin, LeftJoin, and FullJoin) to be treated
// polymorphically.
type multiplicityJoin interface {

Having both multiplicityJoin and JoinMultiplicity is a bit confusing. Maybe change this to joinWithMultiplicity

---

pkg/sql/opt/memo/expr.go, line 456 at r2 (raw file):

	getMultiplicity() props.JoinMultiplicity
}

[nit] add some type assertions here to make it clear what types implement it (var _ multiplicityJoin = &InnerJoinExpr{})

---

pkg/sql/opt/memo/expr_format.go, line 696 at r2 (raw file):

			tp.Childf("unfiltered-cols: %s", r.UnfilteredCols.String())
		}
		if join, ok := e.(multiplicityJoin); ok {

We have ExprFmtHideMiscProps, maybe move this in that block

---

pkg/sql/opt/memo/multiplicity_builder.go, line 33 at r2 (raw file):

		right := t.Child(1).(RelExpr)
		filters := *t.Child(2).(*FiltersExpr)
		multiplicity := GetJoinMultiplicityFromInputs(t.Op(), left, right, filters)

[nit] DeriveJoinMultiplicityFromInputs would be a better name for this one

[DrewKimball]

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

---

pkg/sql/opt/memo/expr.go, line 452 at r2 (raw file):

Previously, RaduBerinde wrote…

Having both multiplicityJoin and JoinMultiplicity is a bit confusing. Maybe change this to joinWithMultiplicity

Done.

---

pkg/sql/opt/memo/expr.go, line 456 at r2 (raw file):

Previously, RaduBerinde wrote…

[nit] add some type assertions here to make it clear what types implement it (var _ multiplicityJoin = &InnerJoinExpr{})

Done.

---

pkg/sql/opt/memo/expr_format.go, line 696 at r2 (raw file):

Previously, RaduBerinde wrote…

We have ExprFmtHideMiscProps, maybe move this in that block

Done. This causes JoinMultiplicity to be outputted more often though, is that ok?

---

pkg/sql/opt/memo/multiplicity_builder.go, line 33 at r2 (raw file):

Previously, RaduBerinde wrote…

[nit] DeriveJoinMultiplicityFromInputs would be a better name for this one

Done.

[DrewKimball]

One thing is that we should only need to derive unfiltered cols in those special cases where we have a self-join or a foreign key join. I realize that would make the logic even more complicated.. But we could at least exit early in some simple cases (no self-joins, and no foreign keys in the tables involved).

I like that idea. I've got a PR coming after this one that will add a few performance improvements for this library; I'll implement this idea there.

[RaduBerinde]

---

pkg/sql/opt/memo/expr_format.go, line 696 at r2 (raw file):

Previously, DrewKimball (Drew Kimball) wrote…

Done. This causes JoinMultiplicity to be outputted more often though, is that ok?

It doesn't seem less important than the other "misc props", I'd say yes

[DrewKimball]

bors r+

[craig]

Merge conflict (retrying...)

[craig]

Merge conflict (retrying...)

[craig]

Merge conflict (retrying...)

[craig]

Canceled

[DrewKimball]

bors r+

[craig]

Build failed (retrying...)

• GitHub CI (Cockroach)

[craig]

Build succeeded

• GitHub CI (Cockroach)