sql: store physical props in indexJoinNode

[justinj]

The change in #25292 was incomplete, this change finishes that work to
store the physical props on the indexJoinNode and not simply pass
through to the index scan.

Release note: None

[cockroach-teamcity]

This change is 

[rytaft]

---

Review status: all files reviewed at latest revision, all discussions resolved, all commit checks successful.

---

pkg/sql/index_join.go, line 218 at r1 (raw file):

		},
		// In the heuristic planner, since all the columns are present in the index
		// join node, we don't need to do any translation here.

[nit] this wording is a bit awkward. I'd also explicitly mention the optimizer to explain why this is needed at all.

---

pkg/sql/opt_needed.go, line 72 at r1 (raw file):

		// These will always line up with the columns in the table, since this
		// function is only used by the heuristic planner, and it plans such that
		// things line up that way.

[nit] This comment is a bit confusing. Maybe just say the length of needed equals the length of resultColumns? Also, it might be worth adding a panic if they are not the same length....

---

Comments from Reviewable

[RaduBerinde]

---

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

---

pkg/sql/expand_plan.go, line 705 at r1 (raw file):

	case *indexJoinNode:
		n.index.props.trim(usefulOrdering)

Add a comment here saying that indexJoinNodes created by the heuristic planner always have the same schema with the underlying tables, so we can pass through usefulOrdering.

---

pkg/sql/opt_needed.go, line 72 at r1 (raw file):

Previously, rytaft wrote…

[nit] This comment is a bit confusing. Maybe just say the length of needed equals the length of resultColumns? Also, it might be worth adding a panic if they are not the same length....

This comment explains why it's ok to call setNeededColumns(n.table, needed) without any translation, it should be above. The loop seems legit regardless if the columns line up.

---

Comments from Reviewable

[RaduBerinde]

Review status: all files reviewed at latest revision, 3 unresolved discussions, all commit checks successful.

---

pkg/sql/opt/exec/execbuilder/testdata/distsql_indexjoin, line 39 at r1 (raw file):

SELECT "URL" FROM [EXPLAIN (DISTSQL) SELECT w FROM t WHERE v > 10 AND v < 50 ORDER BY v]
----
https://cockroachdb.github.io/distsqlplan/decode.html?eJyslL9uqzAYR_f7FFe_9foq2JA_ZWJNh6SKulUMFH-KkBKMbFO1qnj3KjhSTdU4RGHE9vmOz4A_UStJm-JIBukLOBgEGGIwJGCYI2dotCrJGKVPRxywlu9II4aqblrrlm1lD4QUSkvSJMEgyRbVoZ-b8X_Iu5yhVJqQfp_eqP-qma0Gp_OOQbX2PDlnMLbYE9K4Y56de_ZfBj8XrwfaUSFJz6LhZd4yC4Zta9O_GWeZwCUhv0X4qKr67EuGvkZXx0J_eNb4olIMlGJ8I5-k8YrQa5xP1RiPbxSTNF4Reo2LqRqT8Y3xJI1XhF7jcqrGKKzckWlUbWjUnx6dngqSe3JPi1GtLulJq7LXuM9tz_ULkox1uw_uY127rdMFfZgHYRGGRRCOBjD_CcdBOAmbk3vM8yC8CJsX95iXQXgVNq9uMufdn68AAAD__5mfNlw=

This plan looks incorrect (we are discarding v and sorting by w in the final merge). Could you add a logic test that reproduces the problem? A similar query should show the incorrect order in fakedist mode.

---

Comments from Reviewable

[justinj]

Review status: 4 of 10 files reviewed at latest revision, 4 unresolved discussions, all commit checks successful.

---

pkg/sql/opt/exec/execbuilder/testdata/distsql_indexjoin, line 39 at r1 (raw file):

Previously, RaduBerinde wrote…

This plan looks incorrect (we are discarding v and sorting by w in the final merge). Could you add a logic test that reproduces the problem? A similar query should show the incorrect order in fakedist mode.

Ok, added in select_index.

---

Comments from Reviewable

[justinj]

Review status: 4 of 10 files reviewed at latest revision, 4 unresolved discussions, all commit checks successful.

---

pkg/sql/expand_plan.go, line 705 at r1 (raw file):

Previously, RaduBerinde wrote…

Add a comment here saying that indexJoinNodes created by the heuristic planner always have the same schema with the underlying tables, so we can pass through usefulOrdering.

Done.

---

pkg/sql/index_join.go, line 218 at r1 (raw file):

Previously, rytaft wrote…

[nit] this wording is a bit awkward. I'd also explicitly mention the optimizer to explain why this is needed at all.

Done.

---

Comments from Reviewable

[RaduBerinde]

Review status: all files reviewed at latest revision, 4 unresolved discussions, all commit checks successful.

---

pkg/sql/opt/exec/execbuilder/testdata/distsql_indexjoin, line 39 at r1 (raw file):

Previously, justinj (Justin Jaffray) wrote…

Ok, added in select_index.

Looks great, thanks!

---

Comments from Reviewable

[justinj]

TFTRs!

bors r+

[craig]

Build failed

• GitHub CI (Cockroach)

[justinj]

Moved the explain test to the execbuild tests

bors r+

[RaduBerinde]

I'd remove the INSERT from the test with the explain..

[justinj]

bors cancel

[justinj]

bors r-

[craig]

Canceled

[justinj]

oops yeah, that particular insert was leftover from debugging.

bors r+

[craig]

Build failed

• GitHub CI (Cockroach)

[justinj]

bors r+

[craig]

Build succeeded

• GitHub CI (Cockroach)