opt: fix functional deps and stats for WithScanExpr

[rytaft]

This commit fixes the functional dependencies and stats calculation
for WithScanExpr so they use the output columns rather than the input
columns.

Fixes #40296

Release note: None

[cockroach-teamcity]

This change is 

[justinj]

, nice fix on the FDs!

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

---

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

	inColSet := translateColSet(colSet, withScan.OutCols, withScan.InCols)

	// TODO(rytaft): Is there an easy way to access the WithExpr itself?

This was how it worked originally, but it was problematic because the WithExpr can change out from underneath the WithScan (via, say, the replacement of placeholders). I forget exactly why this caused problems but it made a lot of things difficult.

[rytaft]

---

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

Previously, justinj (Justin Jaffray) wrote…

This was how it worked originally, but it was problematic because the WithExpr can change out from underneath the WithScan (via, say, the replacement of placeholders). I forget exactly why this caused problems but it made a lot of things difficult.

But wouldn't we want to re-optimize the full tree if things change in the WithExpr? Like if we learn after placeholder replacement that the size of the WithExpr is much smaller than originally thought?

[justinj]

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

---

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

Previously, rytaft (Rebecca Taft) wrote…

But wouldn't we want to re-optimize the full tree if things change in the WithExpr? Like if we learn after placeholder replacement that the size of the WithExpr is much smaller than originally thought?

We don't have any precedent for re-optimizing expressions that haven't themselves changed, do we? We'd have to add in an extra mechanism that walks and re-interns (or something) WithScanExprs that reference WithExprs that have changed? Sounds doable but a little tricky, and like it might invalidate a handful of assumptions

[rytaft]

TFTR!

I had to add a buildWithScan function to the statisticsBuilder since the shallow copy of stats from the BindingProps was causing some nasty memory overwriting issues. PTAL.

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

---

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

Previously, justinj (Justin Jaffray) wrote…

We don't have any precedent for re-optimizing expressions that haven't themselves changed, do we? We'd have to add in an extra mechanism that walks and re-interns (or something) WithScanExprs that reference WithExprs that have changed? Sounds doable but a little tricky, and like it might invalidate a handful of assumptions

Yea, good point. Seems like it might be worth doing in the future, but not for 19.2. I rephrased this TODO a bit so we can come back to it later.

[rytaft]

@justinj ok to merge with the latest changes?

[justinj]

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

[rytaft]

TFTR!

bors r+

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

[craig]

Build succeeded

• GitHub CI (Cockroach)