opt/xform: make merge join have smaller table on the right side

[yuzefovich]

sql: ignore last noop processor on gateway when moving single flow

When figuring out whether a physical plan consists of flows on multiple
nodes, we want to ignore the noop processor that might be planned on the
gateway for a sole purpose of propagating the results back to the client
(in another words, when we have a flow on the gateway consisting only of
a noop processor). Previously, we forgot to do that.

Release note: None

opt/xform: make merge join have smaller table on the right side

A PR that is currently in-flight will change the implementation of the
vectorized merge joiner so that it processes the left input always in
a streaming fashion whereas the right input might be buffered
(partially) in some cases. To account for this difference this commit
changes the cost model by introducing a small factor to the right row
count which ensures that the smaller right side is preferred to the
symmetric join.

This commit also handles the case of partial joins in a similar manner
to how we handle the hash join.

Fixes: #71790.

Release note: None

[cockroach-teamcity]

This change is 

[yuzefovich]

There are still some stats tests that need an update, but I'll do that at the very end.

There is also a suspicious failure with TestPGTest - it's possible that the way I'm modifying *memo.MergeJoinExpr is wrong (or maybe it's not allowed at all). Will wait for a review on that one too.

[RaduBerinde]

What modification of memo.MergeJoinExpr are you referring to?

[yuzefovich]

This one:

cockroach/pkg/sql/opt/exec/execbuilder/relational.go

Lines 1179 to 1181 in 90c4cf2

	join.Left, join.Right = join.Right, join.Left
	join.LeftEq, join.RightEq = join.RightEq, join.LeftEq
	join.LeftOrdering, join.RightOrdering = join.RightOrdering, join.LeftOrdering

[yuzefovich]

This is needed since we didn't introduce separate (Left|Right)Semi and (Left|Right)Anti joins and is similar to what we already do for the hash joins (although there things were simpler, without having to modify the memo directly).

[RaduBerinde]

Oh, I see, let's not do that, just use some local variables.. If the full plan is cached and reused, multiple threads might be doing this at the same time.

[yuzefovich]

Makes sense, fixed.

However, TestPGTest is still failing (even if I comment out the part about commuting the LeftSemi/LeftAnti join) with

    --- FAIL: TestPGTest/row_description (0.34s)
        datadriven.go:75: 
            testdata/pgtest/row_description:126: RowDescription
            expected:
            {"Type":"RowDescription","Fields":[{"Name":"v1","TableOID":52,"TableAttributeNumber":1,"DataTypeOID":20,"DataTypeSize":8,"TypeModifier":-1,"Format":0},{"Name":"v2","TableOID":53,"TableAttributeNumber":2,"DataTypeOID":20,"DataTypeSize":8,"TypeModifier":-1,"Format":0}]}
            
            found:
            {"Type":"RowDescription","Fields":[{"Name":"v1","TableOID":0,"TableAttributeNumber":0,"DataTypeOID":20,"DataTypeSize":8,"TypeModifier":-1,"Format":0},{"Name":"v2","TableOID":0,"TableAttributeNumber":0,"DataTypeOID":20,"DataTypeSize":8,"TypeModifier":-1,"Format":0}]}

Any idea why that could be the case?

[yuzefovich]

I confirmed that it is the second commit that fails the test. Maybe the plan for the query used in TestPGTest changed and we have some bug in there?

[yuzefovich]

Alright, we have an already present bug (filed #71891), so for now I think it's ok to ignore the table OIDs.

[yuzefovich]

I think I fixed up all the test files. For now I didn't include the changes to the stats quality tests because there are changes in queries for which the plans haven't changed. Anyway, RFAL.

[cucaroach]

Reviewed 2 of 2 files at r1, 11 of 11 files at r3, 11 of 11 files at r4, 1 of 1 files at r5, 13 of 13 files at r6, 15 of 15 files at r7, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @RaduBerinde)

[rytaft]

nit: xfrom -> xform in the commit/PR message title

The stats quality test changes LGTM

Reviewed 2 of 2 files at r1, 11 of 11 files at r3, 11 of 11 files at r4, 1 of 1 files at r5, 13 of 13 files at r6, 15 of 15 files at r7, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @RaduBerinde and @yuzefovich)

---

-- commits, line 23 at r6:
nit: a similar -> a similar fashion?

---

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

	// ensures that a join with the smaller right side is preferred to the
	// symmetric join.
	cost := memo.Cost(leftRowCount+1.25*rightRowCount) * cpuCostFactor

Have you compared the impact on plans if you also multiply leftRowCount by 0.75? (We might want to consider doing this if there are any cases where a different join type is now preferred over merge join, but I didn't see many of those...)

[yuzefovich]

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

---

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

Previously, rytaft (Rebecca Taft) wrote…

Have you compared the impact on plans if you also multiply leftRowCount by 0.75? (We might want to consider doing this if there are any cases where a different join type is now preferred over merge join, but I didn't see many of those...)

I don't think I saw any merge joins change into non-merge joins. Is you reasoning such that we'd like to keep the cost of merge joins roughly unchanged? Would we be happy with 0.75 x 1.25 multiples? Maybe 0.9 x 1.1 would be better?

[rytaft]

Reviewed all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @RaduBerinde and @yuzefovich)

---

-- commits, line 24 at r8:
nit: to how we handle

---

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

Previously, yuzefovich (Yahor Yuzefovich) wrote…

I don't think I saw any merge joins change into non-merge joins. Is you reasoning such that we'd like to keep the cost of merge joins roughly unchanged? Would we be happy with 0.75 x 1.25 multiples? Maybe 0.9 x 1.1 would be better?

I think I saw at least one non-trivial plan change. But I agree that there weren't many.

Either way, I do think it is worth trying to be sure that we're not increasing the total cost of merge joins with this change. If anything, we should decrease it since your execution change makes them more efficient, not less. But anyway, either 0.75 x 1.25 or 0.9 x 1.1 seems like it would probably do the right thing -- you might just need to give it a try and see which plans change.

[yuzefovich]

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

---

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

Previously, rytaft (Rebecca Taft) wrote…

I think I saw at least one non-trivial plan change. But I agree that there weren't many.

Either way, I do think it is worth trying to be sure that we're not increasing the total cost of merge joins with this change. If anything, we should decrease it since your execution change makes them more efficient, not less. But anyway, either 0.75 x 1.25 or 0.9 x 1.1 seems like it would probably do the right thing -- you might just need to give it a try and see which plans change.

I think I'll do 0.9 x 1.1 and will regenerate the test files.

I looked over the PR and still don't see any plans that actually changed - do you mind pointing me to it?

[rytaft]

Reviewed 2 of 2 files at r9, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @RaduBerinde and @yuzefovich)

---

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

Previously, yuzefovich (Yahor Yuzefovich) wrote…

I think I'll do 0.9 x 1.1 and will regenerate the test files.

I looked over the PR and still don't see any plans that actually changed - do you mind pointing me to it?

I added a comment -- only found one.

---

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

	// The vectorized merge join in some cases buffers rows from the right side
	// whereas the left side is processed in a streaming fashion. To account for
	// this difference, we add small factors to both row counts which ensures

super nit: you're not really "adding" a factor to both row counts now... maybe use "multiply" instead?

---

pkg/sql/opt/xform/testdata/external/pgjdbc, line 203 at r9 (raw file):

      │    │    │    │    │    │    │    ├── columns: n.oid:2!null n.nspname:3!null c.oid:7!null c.relname:8!null c.relnamespace:9!null c.relkind:24!null attrelid:44!null attname:45 atttypid:46 attlen:48 attnum:49!null atttypmod:52 a.attnotnull:56 attisdropped:60!null
      │    │    │    │    │    │    │    ├── fd: ()-->(3,60), (2)==(9), (9)==(2), (7)==(44), (44)==(7)
      │    │    │    │    │    │    │    ├── inner-join (merge)

This is the plan that seems to have changed to eliminate merge join in favor of hash

[yuzefovich]

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

---

pkg/sql/opt/xform/testdata/external/pgjdbc, line 203 at r9 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

This is the plan that seems to have changed to eliminate merge join in favor of hash

Oh indeed, thanks. This is a merge join again now.

[rytaft]

nit: further down in the PR message you still have "opt/xfrom"

Otherwise,

Reviewed 9 of 9 files at r10, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @RaduBerinde)

[yuzefovich]

nit: further down in the PR message you still have "opt/xfrom"

Fixed.

TFTRs!

bors r+

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

[craig]

Build failed (retrying...):

• GitHub CI (Cockroach)

[craig]

Build succeeded:

• GitHub CI (Cockroach)