opt: split a disjunction of equijoin predicates into a union of joins

[msirek]

Previously, when the ON clause of an inner, semi or anti join contained
ORed equality predicates, the only available join method was cross join.

This was inadequate because cross join is often the worst performing
join method for joining large tables.

To address this, this patch adds a new cost-based transformation which
evaluates each disjunct in a separate join and unions or intersects the
results together.

Fixes #74302

Example query:

SELECT *
FROM   classRequest
       INNER JOIN classes
               ON classRequest.firstChoiceClassid = classes.classid
                  OR classRequest.secondChoiceClassid = classes.classid;

Transformation result written in pseudo-SQL:

SELECT DISTINCT ON (classes.<rowid_or_primary_key_columns>,
                    classRequest.<rowid_or_primary_key_columns>)
       dt.*
FROM   (
        SELECT     *
        FROM       classRequest
                   INNER JOIN classes
                   ON         classRequest.firstChoiceClassid =
		              classes.classid
                   UNION ALL
        SELECT     *
        FROM       classRequest
                   INNER JOIN classes
                   ON         classRequest.secondChoiceClassid =
		              classes.classid
       ) dt;

In addition, ORed ON clause selectivity estimation is enhanced to
estimate the selectivity of each '=' predicate separately and
combine the estimates in an iterative fashion like PostgreSQL does. This
enables the optimizer to cost the rewritten plan more accurately so it
will get picked.

Release justification: Performance improvement for queries with ORed
join predicates and improved selectivity estimation of ORed predicates

Release note (performance improvement): Performance of inner, semi or
anti join between two tables with ORed equijoin predicates is improved
by enabling the optimizer to select a join plan in which each equijoin
predicate is evaluated by a separate join, with the results of the joins
unioned or intersected together.

[blathers-crl]

Thank you for contributing to CockroachDB. Please ensure you have followed the guidelines for creating a PR.

My owl senses detect your PR is good for review. Please keep an eye out for any test failures in CI.

I was unable to automatically find a reviewer. You can try CCing one of the following members:

• A person you worked with closely on this PR.
• The person who created the ticket, or a CRDB organization member involved with the ticket (author, commenter, etc.).
• Join our community slack channel and ask on #contributors.
• Try find someone else from here.

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

[cockroach-teamcity]

This change is 

[blathers-crl]

Thank you for updating your pull request.

My owl senses detect your PR is good for review. Please keep an eye out for any test failures in CI.

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

[mgartner]

Nice! It will take me some time to go through all this, but I've left a few comments for now.

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

---

pkg/sql/opt/xform/rules/join.opt, line 136 at r1 (raw file):

# relations to the Join.
#
# The replace pattern adds primary key columns to the original Scan ColSet.

It seems like quite a bit of the complexity in the rule is there in order to supporting adding key columns when they are not already part of the input relations. Does it makes sense to not support that case initially and require that both input have key columns to simplify this? Then we can expand the rule or create a new one in a follow-up?

---

pkg/sql/opt/xform/rules/join.opt, line 192 at r1 (raw file):

                $origLeftScanPrivate
            )
            $ok

I'm surprised that you can use a variable bound outside a Let as the Let's return value. I didn't anticipate usage of Let like this. I'd probably consider it a bug. If we do have use cases where we want to bind function return values to arguments and those functions can never fail, maybe we should add new syntax, like a Let without the return value suffix: (Let ($a $b):(GetAB $input)), or maybe use a new keyword. (Bind ($a $b):(GetAB $input)). That's definitely outside the scope of this PR though.

However, I think the complexity of this optgen code is a possible hint that more of this logic should be written in Go, a la GeneratedLookupJoins, or GenerateConstrainedScans. Optgen's strength is matching expression patterns, and this rule doesn't have very complex pattern matching. It's mostly an InnerJoin pattern match with a bunch of Let bindings and function calls. So I don't think we're getting much value out of having so much logic written in optgen. It feels a bit unnatural. As a rough example of what it could look like if some of it was moved to Go (this is psuedo-optgen, and I'm not sure this is the optimal breakdown between optgetn/go, so you'll have to play around with it):

(InnerJoin
    $leftInput:*
    $rightInput:*
    $on:*
    $joinPrivate:* &
        (Let ($firstJoin $secondJoin $setPrivate $groupingCols $ok):(SplitJoinDisjunction $leftInput $rightInput $on $joinPrivate) $ok)
)
=>
(Project
    (DistinctOn
        (UnionAll
            $firstJoin
            $secondJoin
            $setPrivate
        )
        $groupingCols
        ...
    )
)

---

pkg/sql/opt/xform/testdata/rules/join, line 9896 at r1 (raw file):

# --------------------------------------------------
# SplitDisjunctionOfJoinTerms

I think there's some tests we should add like:

• The left and right sides of the join already produce key columns
• Join of tables with compound primary keys
• More than one disjunction in the filter, like abc.a = xyz.x or abc.b = xyz.y or abc.c = xyz.z
• Equality filters that do not reference a column on each side of the joins like abc.a = abc.b OR abc.c = xyz.z
• ON filters that are not a disjunction of equality filters, like (abc.a = xyz.x AND abc.b = xyz.y) OR abc.c = xyz.z
• Semi-join and anti-join tests with correlated and uncorrelated

---

pkg/sql/opt/xform/testdata/rules/join, line 9921 at r1 (raw file):

# Inner join without constraints
opt expect=SplitDisjunctionOfJoinTerms format=hide-all

nit: it'd be nice to see the output columns of expressions and grouping columns of the distinct-on in the expression trees, so I think you should remove format=hide-all.

[msirek]

@mgartner Thanks for taking a look at this. I agree, it's a lot cleaner to keep the optgen logic to a minimum. I've pushed a commit. I haven't finished adding tests, but just thought I'd get this out there so you could see the direction it's headed. Regarding whether restricting the current fix to the HasStrictKey case would reduce complexity, it would mostly just remove the building of the aggCols and groupingCols in new method SplitJoinWithEquijoinDisjuncts. But maybe you are concerned about something else. Please take a look at the new code and let me know if you still think I should just handle the HasStrictKey case at this time. By the way, I had previously tried using UNION instead of UNION ALL + DISTINCT for that case, and UNION ALL performed slightly better, so I thought to unify the rules. If the PK columns are already in the scan, the call to AddPrimaryKeyColsToScanPrivate has no effect. For compound PK testing, a case such as the following with PK columns (a,b) will not trigger the transformation:

SELECT c, a FROM abc INNER JOIN xyz on (abc.a = xyz.x and abc.b = xyz.y) or (abc.b = xyz.x and abc.a = xyz.y);

Currently it only handles simple equality predicates ORed together.

[blathers-crl]

Thank you for updating your pull request.

Before a member of our team reviews your PR, I have some potential action items for you:

• We notice you have more than one commit in your PR. We try break logical changes into separate commits, but commits such as "fix typo" or "address review commits" should be squashed into one commit and pushed with --force
• Please ensure your git commit message contains a release note.
• When CI has completed, please ensure no errors have appeared.

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

[blathers-crl]

Thank you for updating your pull request.

Before a member of our team reviews your PR, I have some potential action items for you:

• We notice you have more than one commit in your PR. We try break logical changes into separate commits, but commits such as "fix typo" or "address review commits" should be squashed into one commit and pushed with --force
• Please ensure your git commit message contains a release note.
• When CI has completed, please ensure no errors have appeared.

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

[msirek]

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

---

pkg/sql/opt/xform/rules/join.opt, line 136 at r1 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

It seems like quite a bit of the complexity in the rule is there in order to supporting adding key columns when they are not already part of the input relations. Does it makes sense to not support that case initially and require that both input have key columns to simplify this? Then we can expand the rule or create a new one in a follow-up?

The code is cleaned up now. I was seeing better performance in UNION ALL + DISTINCT ON vs. UNION, so was hoping to have one rule to cover both cases, but if you think it makes more sense to have separate rules, we could do that. The complexity perhaps is contained within the column remapping, such as this code in join_funcs.go:

	aggCols =
		c.e.funcs.remapColSet(
			c.e.funcs.UnionCols(
				c.e.funcs.NonKeyCols(newLeftScan),
				c.e.funcs.NonKeyCols(newRightScan)),
			firstJoinCols,
			amendedOrigCols,
		)

Maybe a line-by-line code walk-through is in order here, to see what's going on.
During expanded testing, as suggested, I did run across an issue in DISTINCT processing, which seems independent of my changes, which I documented here:
#74554

I am not sure if such a problem could happen on real customer clusters, but if it could, that issue should probably be fixed before this PR is merged. If that issue does not affect UNION processing, that may be an argument for introducing a separate UNION rule, which could be merged sooner.

---

pkg/sql/opt/xform/rules/join.opt, line 192 at r1 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

I'm surprised that you can use a variable bound outside a Let as the Let's return value. I didn't anticipate usage of Let like this. I'd probably consider it a bug. If we do have use cases where we want to bind function return values to arguments and those functions can never fail, maybe we should add new syntax, like a Let without the return value suffix: (Let ($a $b):(GetAB $input)), or maybe use a new keyword. (Bind ($a $b):(GetAB $input)). That's definitely outside the scope of this PR though.

However, I think the complexity of this optgen code is a possible hint that more of this logic should be written in Go, a la GeneratedLookupJoins, or GenerateConstrainedScans. Optgen's strength is matching expression patterns, and this rule doesn't have very complex pattern matching. It's mostly an InnerJoin pattern match with a bunch of Let bindings and function calls. So I don't think we're getting much value out of having so much logic written in optgen. It feels a bit unnatural. As a rough example of what it could look like if some of it was moved to Go (this is psuedo-optgen, and I'm not sure this is the optimal breakdown between optgetn/go, so you'll have to play around with it):

(InnerJoin
    $leftInput:*
    $rightInput:*
    $on:*
    $joinPrivate:* &
        (Let ($firstJoin $secondJoin $setPrivate $groupingCols $ok):(SplitJoinDisjunction $leftInput $rightInput $on $joinPrivate) $ok)
)
=>
(Project
    (DistinctOn
        (UnionAll
            $firstJoin
            $secondJoin
            $setPrivate
        )
        $groupingCols
        ...
    )
)

Yeah, I noticed optgen allows you to reference a variable on any line after it's defined. I wasn't sure how to write a Let rule which didn't require evaluation of a boolean expression return value, that's why I just reused the ok value. I've rewritten the rules as suggested.

---

pkg/sql/opt/xform/testdata/rules/join, line 9896 at r1 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

I think there's some tests we should add like:

• The left and right sides of the join already produce key columns
• Join of tables with compound primary keys
• More than one disjunction in the filter, like abc.a = xyz.x or abc.b = xyz.y or abc.c = xyz.z
• Equality filters that do not reference a column on each side of the joins like abc.a = abc.b OR abc.c = xyz.z
• ON filters that are not a disjunction of equality filters, like (abc.a = xyz.x AND abc.b = xyz.y) OR abc.c = xyz.z
• Semi-join and anti-join tests with correlated and uncorrelated

Thanks, I've added two new test files in the rules tests and logic tests named disjunction_in_join to cover these cases plus a few more. Also, I've extended the code to not only support single-column equality, eg. a1 = b1 OR a2 = b2, but multicolumn equality as well: (a1 = b1 AND a2 = b2) OR (a3 = b3 AND a4 = b4). This includes the join selectivity estimation code in statistics_builder.go.

---

pkg/sql/opt/xform/testdata/rules/join, line 9921 at r1 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

nit: it'd be nice to see the output columns of expressions and grouping columns of the distinct-on in the expression trees, so I think you should remove format=hide-all.

Removed

[rytaft]

Very impressive work here! I've gone through some of the PR and left some comments, but still have about half left.

Reviewed 3 of 11 files at r1, 1 of 4 files at r5, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @mgartner, @msirek, and @rytaft)

---

pkg/sql/opt/memo/statistics_builder.go, line 787 at r6 (raw file):

	// ------------------------------------------------------------------------
	if pred != nil {
		predUnappliedConjucts, predConstrainedCols, predHistCols := sb.applyFilters(pred, scan, relProps, false)

nit: false /* skipOrTermAccounting */

here and below

---

pkg/sql/opt/memo/statistics_builder.go, line 1157 at r6 (raw file):

			equivReps, h.leftProps.OutputCols, h.rightProps.OutputCols, &h.filtersFD, join, s,
		))
		var OredTermSelectivity props.Selectivity

OredTermSelectivity -> oredTermSelectivity

(this variable shouldn't be exported)

---

pkg/sql/opt/memo/statistics_builder.go, line 1158 at r6 (raw file):

		))
		var OredTermSelectivity props.Selectivity
		OredTermSelectivity, numUnappliedConjuncts = sb.selectivityFromOredEquivalencies(h, join, s, numUnappliedConjuncts, true)

nit: true /* semiJoin */

---

pkg/sql/opt/memo/statistics_builder.go, line 1177 at r6 (raw file):

		s.ApplySelectivity(sb.selectivityFromEquivalencies(equivReps, &h.filtersFD, join, s))
		var OredTermSelectivity props.Selectivity
		OredTermSelectivity, numUnappliedConjuncts = sb.selectivityFromOredEquivalencies(h, join, s, numUnappliedConjuncts, false)

nit: false /* semiJoin */

---

pkg/sql/opt/memo/statistics_builder.go, line 2942 at r6 (raw file):

// counts and histograms will be used later to determine the selectivity of
// the filter. If skipOrTermAccounting is true, OrExpr filters are not counted
// towards numUnappliedConjuncts.

It seems like we might want to do this for regular selects in addition to joins -- any reason not to remove this parameter and always try to apply your new logic?

---

pkg/sql/opt/memo/statistics_builder.go, line 3924 at r6 (raw file):

	selectivity = props.OneSelectivity
	var conjunctSelectivity props.Selectivity
_FilterLoop:

We generally try to avoid these sorts of GOTO type features. Any way you can refactor to avoid this? Perhaps a helper function?

---

pkg/sql/opt/memo/statistics_builder.go, line 3936 at r6 (raw file):

		var filter FiltersItem
		var filters FiltersExpr = nil
		var filtersFDs []props.FuncDepSet = nil

you don't need to assign nil, the default construction will be nil

here and below

---

pkg/sql/opt/memo/statistics_builder.go, line 3941 at r6 (raw file):

		for i := 0; i < len(disjuncts); i++ {
			var andFilters FiltersExpr = nil
			filtersFD := props.FuncDepSet{}

nit: var filtersFD props.FuncDepSet

---

pkg/sql/opt/memo/statistics_builder.go, line 3958 at r6 (raw file):

			// If no column equivalencies are found, we know nothing about this term,
			// so should skip selectivity estimation on the entire conjunct.
			if filtersFD.Empty() {

even if it's not empty, it's possible that the FD does not contain an equivalency. You might want to check equivReps here instead of below.

But it's also possible that we can do better even if equivReps is empty -- e.g., if the column is constant, we can also calculate selectivity for that. See buildDisjunctionConstraints, which is doing something similar.

Along those lines, do you think there's a way we can extract a helper function that can be used here and elsewhere in the code for calculating selectivity that would take into account all the different types of selectivity? It's also possible that we are starting to run into the limits of the current design of this stats estimation code. Would be great to chat with you about ideas for how we can improve the state of the world here.

I don't want to hold up this PR for that, though, so if you don't see a relatively simple approach to extracting a helper function (I don't see anything obvious right now), no worries.

---

pkg/sql/opt/memo/statistics_builder.go, line 3985 at r6 (raw file):

			selectivities = append(selectivities, singleSelectivity)
		}
		// Iteratively apply the General Disjunction Rule.

Nice comment and explanation! I agree with this approach as a starting point -- we can consider a more complex solution if we find cases where this is insufficient.

---

pkg/sql/opt/memo/statistics_builder.go, line 4033 at r6 (raw file):

// with identical behavior to the Factory method of the same name, but for use
// in cases where the Factory is not accessible.
func ConstructFiltersItem(condition opt.ScalarExpr, m *Memo) FiltersItem {

This function shouldn't be exported (change to constructFiltersItem)

---

pkg/sql/opt/memo/statistics_builder.go, line 4042 at r6 (raw file):

// continues to find nested And operators. It adds any equality expression
// conjuncts to the given FiltersExpr and returns true.
// This function is inspired by norm.CustomFuncs.addEqExprConjuncts, but serves

I can't find that function -- are you sure this is the correct name?

---

pkg/sql/opt/xform/general_funcs.go, line 147 at r6 (raw file):

	relation memo.RelExpr,
) (scanExpr *memo.ScanExpr, scanPrivate *memo.ScanPrivate, filters memo.FiltersExpr, ok bool) {
	filters = nil

this isn't necessary

---

pkg/sql/opt/xform/general_funcs.go, line 159 at r6 (raw file):

	scanPrivate = &scanExpr.ScanPrivate
	ok = true
	return

for clarity, we usually still include the parameters in the return statement. e.g.:

return scanExpr, scanPrivate, filters, true

---

pkg/sql/opt/xform/join_funcs.go, line 1664 at r6 (raw file):

		)
	newCols = c.e.funcs.OutputCols(newScan)
	return

nit: return newScan, newSelect, newCols

---

pkg/sql/logictest/testdata/logic_test/disjunction_in_join, line 7 at r6 (raw file):

statement ok
CREATE TABLE A(a1 INT, a2 INT, a3 INT, a4 INT, PRIMARY KEY(a1, a2, a3, a4))

nit: we tend to use lower case names for tables and columns (so I'd call this a instead of A)

same thing below

[msirek]

Right. Maybe it should merge just to master. If there is an urgent need to backport it, we could always reconsider. It looks like the release-22.1 branch already exists. I suppose this means anything merging to master will go in the next release and not end up in 22.1?

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

---

pkg/sql/opt/xform/join_funcs.go, line 1630 at r9 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

If you use the new RemapScanColsInFilter (or RemapCols if the first option isn't appropriate), does this requirement go away?

I thought the requirement would go away, but when I tried removing this logic I found a problem case. Not only the old remapping method relies on left and right scans maintaining the same relative order of column ids, so does construction of the UNION ALL. Here's one problem cases that shows how the plan can go bad, with one branch of the UNION ALL seeing x as the first column, and another branch seeing a as the first column:

EXPLAIN(verbose)
SELECT * FROM abc WHERE EXISTS (SELECT * FROM xyz WHERE (abc.a = xyz.x OR abc.b = xyz.y) AND (abc.a = xyz.y OR abc.b = xyz.y));
                                                       info
-------------------------------------------------------------------------------------------------------------------
  distribution: local
  vectorized: true

  • distinct
  │ columns: (a, b, c)
  │ estimated row count: 0
  │ distinct on: x
  │
  └── • project
      │ columns: (x, y, a)
      │
      └── • distinct
          │ columns: (x, y, a, b, c)
          │ estimated row count: 1,002
          │ distinct on: x, b
          │
          └── • union all
              │ columns: (x, y, a, b, c)
              │ estimated row count: 1,002
              │
              ├── • distinct
              │   │ columns: (x, y, a, b, c)
              │   │ estimated row count: 1
              │   │ distinct on: x, b
              │   │
              │   └── • union all
              │       │ columns: (x, y, a, b, c)
              │       │ estimated row count: 1
              │       │
              │       ├── • lookup join (inner)
              │       │   │ columns: (x, y, a, b, c)
              │       │   │ estimated row count: 1
              │       │   │ table: abc@abc_pkey
              │       │   │ equality: (x) = (a)
              │       │   │ equality cols are key
              │       │   │
              │       │   └── • filter
              │       │       │ columns: (x, y)
              │       │       │ estimated row count: 1
              │       │       │ filter: x = y
              │       │       │
              │       │       └── • scan
              │       │             columns: (x, y)
              │       │             estimated row count: 1,000 (100% of the table; stats collected 1420 days ago)
              │       │             table: xyz@xyz_y
              │       │             spans: /!NULL-
              │       │
              │       └── • lookup join (inner)
              │           │ columns: (x, y, a, b, c)
              │           │ estimated row count: 0
              │           │ table: abc@abc_pkey
              │           │ equality: (a) = (a)
              │           │ equality cols are key
              │           │
              │           └── • lookup join (inner)
              │               │ columns: (x, y, a, b)
              │               │ estimated row count: 0
              │               │ table: abc@abc_b
              │               │ equality: (y, x) = (b,a)
              │               │ equality cols are key
              │               │
              │               └── • scan
              │                     columns: (x, y)
              │                     estimated row count: 1,000 (100% of the table; stats collected 1420 days ago)
              │                     table: xyz@xyz_y
              │                     spans: FULL SCAN
              │
              └── • hash join (inner)
                  │ columns: (a, b, c, x, y)
                  │ estimated row count: 1,000
                  │ equality: (b) = (y)
                  │
                  ├── • filter
                  │   │ columns: (a, b, c)
                  │   │ estimated row count: 3,333
                  │   │ filter: (a = b) OR ((b IS DISTINCT FROM CAST(NULL AS INT8)) OR CAST(NULL AS BOOL))
                  │   │
                  │   └── • scan
                  │         columns: (a, b, c)
                  │         estimated row count: 10,000 (100% of the table; stats collected 1420 days ago)
                  │         table: abc@abc_pkey
                  │         spans: FULL SCAN
                  │
                  └── • scan
                        columns: (x, y)
                        estimated row count: 1,000 (100% of the table; stats collected 1420 days ago)
                        table: xyz@xyz_y
                        spans: FULL SCAN

I got rid of a lot of the old logic, because it was a little messy, and added an updated comment:

	// The UNION ALL and join operations treat the output columns in the output
	// row as having the same order as the column id numbers. So, in order for the
	// resulting row definition to have the same columns in the same positions as
	// the join we're replacing, maintain this relative order. Follow a simple
	// rule that the new duplicated left and right tables maintain the same column
	// id order as the original scans. Scans allocate column ids in contiguous
	// chunks, so if the first column id in the left is less than the first column
	// id in the right, then all column ids in the left will be lower than all
	// right column ids.
	if leftFirstColID < rightFirstColID {
		newLeftScanPrivate = c.DuplicateScanPrivate(leftSP)
		newRightScanPrivate = c.DuplicateScanPrivate(rightSP)
		newLeftScanPrivate2 = c.DuplicateScanPrivate(leftSP)
		newRightScanPrivate2 = c.DuplicateScanPrivate(rightSP)
	} else {
		newRightScanPrivate = c.DuplicateScanPrivate(rightSP)
		newLeftScanPrivate = c.DuplicateScanPrivate(leftSP)
		newRightScanPrivate2 = c.DuplicateScanPrivate(rightSP)
		newLeftScanPrivate2 = c.DuplicateScanPrivate(leftSP)
	}

---

pkg/sql/opt/xform/join_funcs.go, line 2029 at r10 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

Is there value in performing the split if the columns in the left and right scalar expressions are not indexed? I guess there might be if it transforms a cross-join into a hash-join, is that correct?

For SplitDisjunction, we ensure that the rule only fires if there is some chance that splitting up the disjunction will lead to constrained indexed scans on both sides of the disjunction, so that we don't needlessly increase the size of the memo:

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

Lines 1671 to 1678 in 2fdd9ae

	// Return an empty pair if either of the groups is empty or if either the
	// left or right groups are unlikely to constrain an index scan.
	if len(leftExprs) == 0 ||
	len(rightExprs) == 0 ||
	!c.canMaybeConstrainIndexWithCols(sp, leftColSet) ||
	!c.canMaybeConstrainIndexWithCols(sp, rightColSet) {
	return nil, nil, false
	}

Yes, even with no indexes, enabling the optimizer to pick hash join can make the join go faster. The number of comparisons of left rows to right rows can be greatly reduced. I'm not sure what a good check for joins would be, to avoid needlessly increasing the size of the memo. I thought if leftScan.Relational().Cardinality.IsZeroOrOne() is true, then we should require that the right scan passes the canMaybeConstrainIndexWithCols check, but in the test case I tried, select * from t1, (select * from t2 limit 1) lt2 where t1.a = lt2.a or t1.a = lt2.b, the limited scan is not a canonical scan, so it fails the getfilteredCanonicalScan check, and we don't try the split join plan anyway. So, adding this new rule may not have any effect. Let me know if you can think of any cases we should limit. If none, I guess we just leave this as-is.

---

pkg/sql/opt/xform/testdata/rules/disjunction_in_join, line 33 at r10 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

Why do some of these tests show the memo rather than just the final plan? If the purpose of these tests is to ensure that the rule creates valid transformations, it's very difficult to determine this by looking at the memo.

These queries didn't actually pick the split join plan, so I didn't use opt. I guess I used memo just to check that the UNION ALL plan was considered, but I suppose expect=SplitDisjunctionOfJoinTerms already indicates that. I've changed these from memo to build-cascades tests to keep the output low.

Code quote:

The left AND right sides of the join already produce key columns

---

pkg/sql/opt/xform/testdata/rules/join, line 9896 at r1 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

Thanks! Should these tests live in the new rules test file? Or is there a reason you've split them up?

I think these were earlier tests, then I added more robust testing in disjunction_in_join. These earlier tests might be redundant. Do you think I should remove them?

[msirek]

TFTRs!
bors r+

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

[craig]

Build succeeded:

• GitHub CI (Cockroach)