Merge cockroachdb#108070 cockroachdb#108174

108070: opt: don't remap cols with non-identical types during decorrelation r=DrewKimball a=DrewKimball

#### opt: don't remap cols with non-identical types during decorrelation

The decorrelation rules `TryRemapJoinOuterColsRight`,
`TryRemapJoinOuterColsLeft`, and `TryRemapSelectOuterCols` attempt to
decorrelate an expression by replacing an outer-column reference with
an equivalent non-outer column reference. However, this includes columns
that are _equivalent_ but not _identical_, the difference being that
equivalent columns can have different width or precision, while identical
types are exactly the same. This could cause incorrect results, like the
following example:
```
statement ok
CREATE TABLE xy108057 (x DECIMAL(10, 2) PRIMARY KEY, y DECIMAL(10, 2));
CREATE TABLE ab108057 (a DECIMAL(10, 0), b DECIMAL(10, 0));

statement ok
INSERT INTO xy108057 VALUES (1.00, 1.00);
INSERT INTO ab108057 VALUES (1, 1);

query RRRR
SELECT * FROM xy108057 INNER JOIN LATERAL (SELECT a, a+x FROM ab108057) ON a = x
----
1.00  1.00  1  2.00
```
This patch fixes this bug by checking that the replacement column
is identical, not just equivalent, to the outer column.

Fixes cockroachdb#108057

Release note (bug fix): Fixed a bug introduced in 23.1 that could cause
the precision of some values to be incorrectly truncated for a query
with a correlated subquery and an equality between a column from the
subquery and the outer query. This applies to types that are "equivalent"
but have different precision levels, e.g. `DECIMAL(10, 0)` vs
`DECIMAL(10, 2)` or `NAME` vs `CHAR`.

108174: ccl/sqlproxyccl: fixes a data race in the ACL's watcher r=JeffSwenson a=jaylim-crl

Fixes cockroachdb#108156. Regression from cockroachdb#108097.

Previously, we updated the code to return w.controllers after releasing the
lock because checkConnection may be long running, and we did not want to block
new connections. We had forgotten the contract where fields on the Watcher
object needs `mu` as they may be updated, so that approach had data races.
This commit address that by ensuring that a copy of w.controllers is returned
instead.

Release note: None

Epic: none

Co-authored-by: Drew Kimball <[email protected]>
Co-authored-by: Jay <[email protected]>

Author: 3 people

pkg/ccl/sqlproxyccl/acl/watcher.go

@@ -276,7 +276,9 @@ func (w *Watcher) ListenForDenied(
 
 		w.listeners.ReplaceOrInsert(l)
 
-		return l, w.controllers
+		// We need a new copy of w.controllers so that it doesn't race with the
+		// add and update operations.
+		return l, append([]AccessController(nil), w.controllers...)
 	}()
 	if err := checkConnection(ctx, connection, controllers); err != nil {
 		w.removeListener(lst)

pkg/sql/logictest/testdata/logic_test/subquery_correlated

@@ -1332,3 +1332,22 @@ SELECT (2, 20) = ANY (
 ----
 NULL
 NULL
+
+# Regression test for #108057 - maintain the precision of the a+x operation.
+statement ok
+CREATE TABLE xy108057 (x DECIMAL(10, 2) PRIMARY KEY, y DECIMAL(10, 2));
+CREATE TABLE ab108057 (a DECIMAL(10, 0), b DECIMAL(10, 0));
+
+statement ok
+INSERT INTO xy108057 VALUES (1.00, 1.00);
+INSERT INTO ab108057 VALUES (1, 1);
+
+query RRRR
+SELECT * FROM xy108057 INNER JOIN LATERAL (SELECT a, a+x FROM ab108057) ON a = x
+----
+1.00  1.00  1  2.00
+
+query RRRR
+SELECT * FROM ab108057 INNER JOIN LATERAL (SELECT x, x+a FROM xy108057) ON a = x
+----
+1  1  1.00  2.00

pkg/sql/opt/norm/decorrelate_funcs.go

@@ -1147,8 +1147,8 @@ func (c *CustomFuncs) TryRemapOuterCols(
 }
 
 // tryRemapOuterCols handles the traversal and outer-column replacement for
-// TryRemapOuterCols. It returns the replacement expression and whether an
-// outer-column reference was successfully remapped.
+// TryRemapOuterCols. It returns the replacement expression, which may be
+// unchanged if remapping was not possible.
 func (c *CustomFuncs) tryRemapOuterCols(
 	expr opt.Expr, outerCol opt.ColumnID, substituteCols opt.ColSet,
 ) opt.Expr {
@@ -1160,9 +1160,15 @@ func (c *CustomFuncs) tryRemapOuterCols(
 	switch t := expr.(type) {
 	case *memo.VariableExpr:
 		if t.Col == outerCol {
-			if replaceCol, ok := substituteCols.Next(0); ok {
-				// This outer-column reference can be remapped.
-				return c.f.ConstructVariable(replaceCol)
+			md := c.mem.Metadata()
+			outerColTyp := md.ColumnMeta(outerCol).Type
+			replaceCol, ok := substituteCols.Next(0)
+			for ; ok; replaceCol, ok = substituteCols.Next(replaceCol + 1) {
+				if outerColTyp.Identical(md.ColumnMeta(replaceCol).Type) {
+					// Only perform the replacement if the types are identical.
+					// This outer-column reference can be remapped.
+					return c.f.ConstructVariable(replaceCol)
+				}
 			}
 		}
 	case memo.RelExpr:

pkg/sql/opt/norm/testdata/rules/decorrelate

@@ -18,6 +18,14 @@ exec-ddl
 CREATE TABLE ab (a INT, b INT)
 ----
 
+exec-ddl
+CREATE TABLE xy_decimal (x DECIMAL(10, 2) PRIMARY KEY, y DECIMAL(10, 2));
+----
+
+exec-ddl
+CREATE TABLE ab_decimal (a DECIMAL(10, 0), b DECIMAL(10, 0));
+----
+
 # --------------------------------------------------
 # DecorrelateJoin
 # --------------------------------------------------
@@ -6925,6 +6933,31 @@ project
       └── filters
            └── corr = x
 
+# Regression test for #108057. Do not remap a column into an equivalent, but not
+# identically typed column. The a + x projection should remain.
+norm expect-not=(TryRemapJoinOuterColsRight,TryRemapJoinOuterColsLeft,TryRemapSelectOuterCols)
+SELECT * FROM xy_decimal INNER JOIN LATERAL (SELECT a, a+x FROM ab_decimal) ON a = x
+----
+project
+ ├── columns: x:1!null y:2 a:5!null "?column?":10!null
+ ├── immutable
+ ├── fd: (1)-->(2), (1)==(5), (5)==(1), (5)-->(10)
+ ├── inner-join (hash)
+ │    ├── columns: x:1!null y:2 a:5!null
+ │    ├── multiplicity: left-rows(zero-or-more), right-rows(zero-or-one)
+ │    ├── immutable
+ │    ├── fd: (1)-->(2), (1)==(5), (5)==(1)
+ │    ├── scan xy_decimal
+ │    │    ├── columns: x:1!null y:2
+ │    │    ├── key: (1)
+ │    │    └── fd: (1)-->(2)
+ │    ├── scan ab_decimal
+ │    │    └── columns: a:5
+ │    └── filters
+ │         └── a:5 = x:1 [outer=(1,5), immutable, constraints=(/1: (/NULL - ]; /5: (/NULL - ]), fd=(1)==(5), (5)==(1)]
+ └── projections
+      └── a:5 + x:1 [as="?column?":10, outer=(1,5), immutable]
+
 # --------------------------------------------------
 # TryRemapJoinOuterColsLeft
 # --------------------------------------------------