distsql: support left outer lookup joins

[solongordon]

joinReader now supports left outer lookup joins. This required adding a
new joinReaderSpec field for specifying the join type.

Release note (sql change): The experimental lookup join feature now
supports left outer joins.

[cockroach-teamcity]

This change is 

[solongordon]

Please ignore the first commit, which corresponds to #25628.

cc @asubiotto @petermattis

[petermattis]

Reviewed 9 of 9 files at r1.
Review status: 6 of 11 files reviewed at latest revision, all discussions resolved.

---

pkg/sql/distsqlrun/joinreader.go, line 399 at r2 (raw file):

		return true, err
	}
	emittedRowIndices := util.MakeFastIntSet()

FastIntSet is fast when the range of indices is [0,63]. The default batch size for lookups is 100. Rather than using a FastIntSet, I'd add a scratch space []int to joinReader and keep track of the unmatched rows.

---

pkg/sql/distsqlrun/joinreader.go, line 421 at r2 (raw file):

		} else {
			if jr.primaryFetcher != nil {
				// TODO(solon): Batch these primary index scans.

I didn't notice this in the other PR, but we'll need to fix this before merging it.

---

Comments from Reviewable

[solongordon]

Review status: 6 of 11 files reviewed at latest revision, 2 unresolved discussions.

---

pkg/sql/distsqlrun/joinreader.go, line 399 at r2 (raw file):

Previously, petermattis (Peter Mattis) wrote…

FastIntSet is fast when the range of indices is [0,63]. The default batch size for lookups is 100. Rather than using a FastIntSet, I'd add a scratch space []int to joinReader and keep track of the unmatched rows.

I see, thanks. I replaced the set with a []bool array. Is that reasonable?

---

pkg/sql/distsqlrun/joinreader.go, line 421 at r2 (raw file):

Previously, petermattis (Peter Mattis) wrote…

I didn't notice this in the other PR, but we'll need to fix this before merging it.

OK to handle this in a follow-up PR to keep the code reviews simpler?

---

Comments from Reviewable

[petermattis]

Review status: 3 of 18 files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

---

pkg/sql/distsqlrun/joinreader.go, line 421 at r2 (raw file):

Previously, solongordon (Solon) wrote…

OK to handle this in a follow-up PR to keep the code reviews simpler?

Yes, fine to handle the batching in a follow-on PR, though I'm assuming it will be the next thing worked on. Not batching the primary index lookups is a huge hit on performance.

---

pkg/sql/distsqlrun/joinreader.go, line 465 at r4 (raw file):

	if emitted != nil {
		// Emit unmatched rows.
		for i := 0; i < len(rows); i++ {

The downside to an emitted []bool var is that you have to loop over the input rows. My suggestion for an unmatchedRows []int slice was so that you only loop over the unmatched rows. It isn't clear to me why this is more difficult. Can't you detect an unmatched row as the for which jr.renderedRow == nil? If that is wrong for some reason, the emitted approach looks fine.

---

Comments from Reviewable

[solongordon]

Review status: 3 of 18 files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

---

pkg/sql/distsqlrun/joinreader.go, line 421 at r2 (raw file):

Previously, petermattis (Peter Mattis) wrote…

Yes, fine to handle the batching in a follow-on PR, though I'm assuming it will be the next thing worked on. Not batching the primary index lookups is a huge hit on performance.

Yup, coming up next.

---

pkg/sql/distsqlrun/joinreader.go, line 465 at r4 (raw file):

Previously, petermattis (Peter Mattis) wrote…

The downside to an emitted []bool var is that you have to loop over the input rows. My suggestion for an unmatchedRows []int slice was so that you only loop over the unmatched rows. It isn't clear to me why this is more difficult. Can't you detect an unmatched row as the for which jr.renderedRow == nil? If that is wrong for some reason, the emitted approach looks fine.

My thinking was that the renderedRow == nil approach isn't always correct because an input row might map to multiple lookup rows, some of which meet the ON condition and some of which don't.

---

Comments from Reviewable

[petermattis]

Review status: 3 of 18 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

---

pkg/sql/distsqlrun/joinreader.go, line 442 at r4 (raw file):

			}
			for _, rowIdx := range rowIndices {
				jr.renderedRow = jr.renderedRow[:0]

Not your code: do you know what the purpose of this is? Looks like joinerBase.render returns joinerBase.combinedRow. I don't see a point to this. I think joinReader.renderedRow can be removed and a local variable can be used here.

---

pkg/sql/distsqlrun/joinreader.go, line 465 at r4 (raw file):

Previously, solongordon (Solon) wrote…

My thinking was that the renderedRow == nil approach isn't always correct because an input row might map to multiple lookup rows, some of which meet the ON condition and some of which don't.

I might be misreading the code, but it seems to me that they only way for emitted[rowIdx] to be false is for jr.renderedRow to be nil. If that isn't true, can you walk me through why it isn't?

---

Comments from Reviewable

[solongordon]

Review status: 3 of 18 files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

---

pkg/sql/distsqlrun/joinreader.go, line 465 at r4 (raw file):

Previously, petermattis (Peter Mattis) wrote…

I might be misreading the code, but it seems to me that they only way for emitted[rowIdx] to be false is for jr.renderedRow to be nil. If that isn't true, can you walk me through why it isn't?

I think there are two problems actually. The simpler one is that emitted[rowIdx] might be false because the index scan returned no results for that row, so it never even makes it to the render step.

The more complicated one is the index lookup might return multiple index rows for one input row (say if the lookup is on only a prefix of the index columns.) In that case, renderedRow could be nil for some but not all of those index rows. We need to make sure not to emit the input row in that case.

---

Comments from Reviewable

[petermattis]

Review status: 3 of 18 files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

---

pkg/sql/distsqlrun/joinreader.go, line 465 at r4 (raw file):

Previously, solongordon (Solon) wrote…

I think there are two problems actually. The simpler one is that emitted[rowIdx] might be false because the index scan returned no results for that row, so it never even makes it to the render step.

The more complicated one is the index lookup might return multiple index rows for one input row (say if the lookup is on only a prefix of the index columns.) In that case, renderedRow could be nil for some but not all of those index rows. We need to make sure not to emit the input row in that case.

Ah, got it. Thanks for the explanation. Might be useful to add some comments to make this clearer.

---

Comments from Reviewable

[solongordon]

Review status: 3 of 18 files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

---

pkg/sql/distsqlrun/joinreader.go, line 442 at r4 (raw file):

Previously, petermattis (Peter Mattis) wrote…

Not your code: do you know what the purpose of this is? Looks like joinerBase.render returns joinerBase.combinedRow. I don't see a point to this. I think joinReader.renderedRow can be removed and a local variable can be used here.

Agreed, fixed.

---

pkg/sql/distsqlrun/joinreader.go, line 465 at r4 (raw file):

Previously, petermattis (Peter Mattis) wrote…

Ah, got it. Thanks for the explanation. Might be useful to add some comments to make this clearer.

Will do.

---

Comments from Reviewable

[solongordon]

The dependent PR has now been merged so this should be better reviewable now. PTAL.

[jordanlewis]

Code - I think it would be good to add some more tests, though, unless the old tests are somehow exercising the new code sufficiently.

---

Reviewed 15 of 15 files at r5.
Review status: all files reviewed at latest revision, all discussions resolved.

---

pkg/sql/distsqlrun/joinreader.go, line 413 at r5 (raw file):

	var emitted []bool
	if jr.joinType == sqlbase.LeftOuterJoin {
		emitted = make([]bool, len(rows))

This gets called once per batch, right? You could save on allocations by pre-allocating this array once, in the setup code of the joinReader if it's a LeftOuterJoin, and zeroing it in this function. But I suppose in the common case, we would hope that joinReader wouldn't have too many batches, since that would indicate that we should have used a different join strategy...

Take it or leave it!

---

pkg/sql/logictest/testdata/logic_test/lookup_join, line 230 at r5 (raw file):

# Left lookup outer join
query T
SELECT a.book FROM authors a LEFT OUTER JOIN books b ON a.book = b.title WHERE b.title IS NULL

You mentioned above that there are a few reasons why we can't track un-emitted rows, like when the index scan returns no rows vs the when index scan returns multiple rows. Are all of those cases tested sufficiently here? In my experience, outer joins have a lot of edge cases - we should make sure those are all tested for this new method of executing them.

---

Comments from Reviewable

[solongordon]

Review status: all files reviewed at latest revision, 2 unresolved discussions.

---

pkg/sql/distsqlrun/joinreader.go, line 413 at r5 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

This gets called once per batch, right? You could save on allocations by pre-allocating this array once, in the setup code of the joinReader if it's a LeftOuterJoin, and zeroing it in this function. But I suppose in the common case, we would hope that joinReader wouldn't have too many batches, since that would indicate that we should have used a different join strategy...

Take it or leave it!

Yeah, true. It's such a small allocation that I'm tempted to opt for more allocations over more code complexity. (Also now we know we could avoid a heap allocation by changing the length arg from len(rows) to joinReaderBatchSize, but even that seems excessive to me.)

---

pkg/sql/logictest/testdata/logic_test/lookup_join, line 230 at r5 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

You mentioned above that there are a few reasons why we can't track un-emitted rows, like when the index scan returns no rows vs the when index scan returns multiple rows. Are all of those cases tested sufficiently here? In my experience, outer joins have a lot of edge cases - we should make sure those are all tested for this new method of executing them.

Good call. I'll at least add one for the case where the lookup yields multiple rows, some emitted and some not.

---

Comments from Reviewable

[solongordon]

I found an annoying bug while adding tests. If a query had an ON filter on the right side of the join (the index), the left side would not be emitted if all the right rows were filtered out. This was due to the way filters were originally implemented in lookup joins. If the logical plan had a filter on the right side scan, it was just added as a post-processing filter on the joinReader. This meant rows got filtered out in the emit stage, too late for the left join logic to know. (Also it made an ON filter indistinguishable from a WHERE filter.)

I handled this by specifying index filters explicitly in the JoinReaderSpec. This allows applying the filter earlier, right after the index lookup. It also makes the physical planning simpler, so it's got that going for it, which is nice.

[jordanlewis]

Ouch, glad you caught that. On the plus side, as you note, the code is much easier to read now.

Just making sure - WHERE conditions don't get propagated down into the lookup join's scanNode, do they? If they did, we'd be incorrectly moving WHERE conditions that only concern the right side into ON conditions, which would be similarly broken.

---

Review status: 0 of 7 files reviewed at latest revision, all discussions resolved.

---

pkg/sql/distsqlrun/joinreader.go, line 64 at r6 (raw file):

	// the index columns. These are the equality columns of the join.
	lookupCols  columns
	indexFilter exprHelper

comment on this field would be nice. "indexFilter is the part of the ON condition that applies to the index being probed. Probed index rows that fail the indexFilter do not count as matches, and will not be emitted" or something.

---

pkg/sql/logictest/testdata/logic_test/lookup_join, line 104 at r6 (raw file):

SELECT "URL" FROM [EXPLAIN (DISTSQL) SELECT * FROM distsql_lookup_test_1 JOIN distsql_lookup_test_2 ON f = b WHERE a > 1 AND e > 1]
----
https://cockroachdb.github.io/distsqlplan/decode.html?eJyUkcFq8zAQhO__U5g9708sp-lBJ11TSlJCb8UY1VqCWkerSjK0BL97sVVoUoibHne0386gOYJjQxt9oAjyCQQgrKBG8IFbipHDKOeltXkHWSJY5_s0yjVCy4FAHiHZ1BFI2PB_9osKEAwlbbtpbUDgPn1DMek9gVwOeHJYzB9-1M8d7UgbCovy7Dz4YA86fChjY4pvXdMxv_a-SRRTI-CSu_iL-x1b92Uurjcfv-F-mosXtq5gJws1its-yUIJVBWqJaobVCtUtxejVmdRf2lgR9Gzi3RVBeVQI5DZU245ch9aegjcTjZ53E7cJBiKKb8u87B2-WkMeAqLWbiah6tZuPwB18O_zwAAAP__2Q3qzg==

It's unfortunate that the new plan diagram doesn't contain the filter. I wouldn't want to hold up this PR on that fact, but I think we definitely need to reinstate the filter into the plan diagram by looking at the new filter expr you added in the diagram creation code.

---

pkg/sql/logictest/testdata/logic_test/lookup_join, line 228 at r6 (raw file):

Hal Abelson

sorry, bell hooks! i didn't mean to kick you out. you belong in this pantheon.

---

Comments from Reviewable

[solongordon]

Right, I tested out the WHERE case and it was fine (though maybe I should add that to the logic tests to be sure.) I think the logical plan would be incorrect in that case. It would be wrong for it to push a WHERE filter down below the left join node.

---

Review status: 0 of 7 files reviewed at latest revision, 3 unresolved discussions.

---

Comments from Reviewable

[solongordon]

Review status: 0 of 7 files reviewed at latest revision, 3 unresolved discussions.

---

pkg/sql/logictest/testdata/logic_test/lookup_join, line 104 at r6 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

It's unfortunate that the new plan diagram doesn't contain the filter. I wouldn't want to hold up this PR on that fact, but I think we definitely need to reinstate the filter into the plan diagram by looking at the new filter expr you added in the diagram creation code.

Yeah, agree, I'll make an issue for this. I actually originally added it but the column indices were confusing since they only referred to the right side, so I'll figure this out separately.

---

Comments from Reviewable

[solongordon]

Review status: 0 of 7 files reviewed at latest revision, 3 unresolved discussions.

---

pkg/sql/distsqlrun/joinreader.go, line 64 at r6 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

comment on this field would be nice. "indexFilter is the part of the ON condition that applies to the index being probed. Probed index rows that fail the indexFilter do not count as matches, and will not be emitted" or something.

Done. Btw it's not necessarily part of the ON cond. For inner joins it could be a WHERE condition which got pushed down.

---

Comments from Reviewable

[solongordon]

bors r+

[craig]

Build succeeded

• GitHub CI (Cockroach)