Merge cockroachdb#107995 cockroachdb#108045 cockroachdb#108049 cockroachdb#108106

107995: opt: do not remap columns with non-identical types r=mgartner a=mgartner

This commit limits the remapping of column during join elimination to
only destination columns with identical types to their equivalent source
columns. This prevents the optimizer from producing an incorrect query
plan. See the test cases for examples.

Fixes cockroachdb#107850

There is no release note because this bug does not exist in any
releases.

Release note: None


108045: opt: propagate HasUDF in cached shared logical properties r=mgartner a=mgartner

This commit fixes a minor bug where the `props.Shared.HasUDF` property
was not correctly propagated from cached shared properties of an
expression's children. As far as I can tell, this bug currently has no
effect on any optimizer behavior. `HasUDF` is only used in determining
if the insert fast path can be used and there are other, more strict
restrictions for the insert fast path that preclude acceptance of any
expression in which `HasUDF` may not be properly propagated (e.g., a
Values expression without subqueries is required and the only way I can
figure to include a UDF invocation in a relational expression that's a
child of a Values clause is with a subquery).

Epic: None

Release note: None


108049: ui: fix infinite cluster setting refresh r=zachlite a=zachlite

The DatabaseTablePage was unconditionally refreshing cluster settings causing a flood of network requests.

Epic: none
Release note (bux fix): A bug has been fixed that caused a flood of requests to refresh cluster settings on the Table Detail page.

If a user would like to see the effect of a modified cluster setting in DB Console, a page-reload is required.

108106: asim: still non-deterministic r=kvoli a=wenyihu6

Part Of: cockroachdb#105904, cockroachdb#108092
Release Note: None

Co-authored-by: Marcus Gartner <[email protected]>
Co-authored-by: Zach Lite <[email protected]>
Co-authored-by: wenyihu6 <[email protected]>

Author: 4 people

pkg/kv/kvserver/asim/asim_test.go

@@ -40,7 +40,7 @@ func TestRunAllocatorSimulator(t *testing.T) {
 }
 
 func TestAsimDeterministic(t *testing.T) {
-	skip.UnderRace(t, 105904, "asim is non-deterministic under race")
+	skip.WithIssue(t, 105904, "asim is non-deterministic")
 	settings := config.DefaultSimulationSettings()
 
 	runs := 3

pkg/sql/logictest/testdata/logic_test/join

@@ -1217,3 +1217,24 @@ SELECT * FROM (SELECT * FROM t106371 ORDER BY y LIMIT 1) a
 JOIN (SELECT DISTINCT ON (x) * FROM (SELECT * FROM t106371 WHERE y = 2)) b ON a.x = b.x;
 ----
 1  1  1  2
+
+# Regression test for #107850. Do not eliminate joins and remap columns that are
+# equivalent but have non-identical types.
+subtest regression_107850
+
+statement ok
+CREATE TABLE t107850a (
+  d2 DECIMAL(10, 2) NOT NULL UNIQUE
+);
+CREATE TABLE t107850b (
+  d0 DECIMAL(10, 0) NOT NULL REFERENCES t107850a (d2)
+);
+INSERT INTO t107850a VALUES (1.00);
+INSERT INTO t107850b VALUES (1);
+
+query R
+SELECT d2 FROM t107850a JOIN t107850b ON d2 = d0
+----
+1.00
+
+subtest end

pkg/sql/opt/memo/logical_props_builder.go

@@ -1730,6 +1730,9 @@ func BuildSharedProps(e opt.Expr, shared *props.Shared, evalCtx *eval.Context) {
 			if cached.HasCorrelatedSubquery {
 				shared.HasCorrelatedSubquery = true
 			}
+			if cached.HasUDF {
+				shared.HasUDF = true
+			}
 		} else {
 			BuildSharedProps(e.Child(i), shared, evalCtx)
 		}

pkg/sql/opt/memo/testdata/logprops/udf

@@ -209,3 +209,68 @@ project
                                │    └── projections
                                │         └── const: 1 [as="?column?":5, type=int]
                                └── const: 1 [type=int]
+
+# The "udf" property should propagate to the top-most filter.
+build
+SELECT i FROM (VALUES (1), (2)) v(i) WHERE i = (SELECT a FROM ab WHERE b = fn_leakproof())
+----
+select
+ ├── columns: i:1(int!null)
+ ├── cardinality: [0 - 2]
+ ├── values
+ │    ├── columns: column1:1(int!null)
+ │    ├── cardinality: [2 - 2]
+ │    ├── prune: (1)
+ │    ├── tuple [type=tuple{int}]
+ │    │    └── const: 1 [type=int]
+ │    └── tuple [type=tuple{int}]
+ │         └── const: 2 [type=int]
+ └── filters
+      └── eq [type=bool, outer=(1), subquery, udf, constraints=(/1: (/NULL - ])]
+           ├── variable: column1:1 [type=int]
+           └── subquery [type=int]
+                └── max1-row
+                     ├── columns: a:2(int!null)
+                     ├── error: "more than one row returned by a subquery used as an expression"
+                     ├── cardinality: [0 - 1]
+                     ├── key: ()
+                     ├── fd: ()-->(2)
+                     └── project
+                          ├── columns: a:2(int!null)
+                          ├── key: (2)
+                          ├── prune: (2)
+                          ├── interesting orderings: (+2)
+                          └── select
+                               ├── columns: a:2(int!null) b:3(int!null) crdb_internal_mvcc_timestamp:4(decimal) tableoid:5(oid)
+                               ├── key: (2)
+                               ├── fd: ()-->(3), (2)-->(4,5)
+                               ├── prune: (2,4,5)
+                               ├── interesting orderings: (+2 opt(3))
+                               ├── scan ab
+                               │    ├── columns: a:2(int!null) b:3(int) crdb_internal_mvcc_timestamp:4(decimal) tableoid:5(oid)
+                               │    ├── key: (2)
+                               │    ├── fd: (2)-->(3-5)
+                               │    ├── prune: (2-5)
+                               │    └── interesting orderings: (+2)
+                               └── filters
+                                    └── eq [type=bool, outer=(3), udf, constraints=(/3: (/NULL - ]), fd=()-->(3)]
+                                         ├── variable: b:3 [type=int]
+                                         └── udf: fn_leakproof [type=int]
+                                              └── body
+                                                   └── limit
+                                                        ├── columns: "?column?":6(int!null)
+                                                        ├── cardinality: [1 - 1]
+                                                        ├── key: ()
+                                                        ├── fd: ()-->(6)
+                                                        ├── project
+                                                        │    ├── columns: "?column?":6(int!null)
+                                                        │    ├── cardinality: [1 - 1]
+                                                        │    ├── key: ()
+                                                        │    ├── fd: ()-->(6)
+                                                        │    ├── values
+                                                        │    │    ├── cardinality: [1 - 1]
+                                                        │    │    ├── key: ()
+                                                        │    │    └── tuple [type=tuple]
+                                                        │    └── projections
+                                                        │         └── const: 1 [as="?column?":6, type=int]
+                                                        └── const: 1 [type=int]

pkg/sql/opt/norm/general_funcs.go

@@ -308,15 +308,34 @@ func (c *CustomFuncs) MakeBoolCol() opt.ColumnID {
 // CanRemapCols returns true if it's possible to remap every column in the
 // "from" set to a column in the "to" set using the given FDs.
 func (c *CustomFuncs) CanRemapCols(from, to opt.ColSet, fds *props.FuncDepSet) bool {
-	for col, ok := from.Next(0); ok; col, ok = from.Next(col + 1) {
-		if !fds.ComputeEquivGroup(col).Intersects(to) {
+	for fromCol, ok := from.Next(0); ok; fromCol, ok = from.Next(fromCol + 1) {
+		if _, ok := c.remapColWithIdenticalType(fromCol, to, fds); !ok {
 			// It is not possible to remap this column to one from the "to" set.
 			return false
 		}
 	}
 	return true
 }
 
+// remapColWithIdenticalType returns a column in the "to" set that is equivalent
+// to "fromCol" and has the identical type. It returns ok=false if no such
+// column exists.
+func (c *CustomFuncs) remapColWithIdenticalType(
+	fromCol opt.ColumnID, to opt.ColSet, fds *props.FuncDepSet,
+) (_ opt.ColumnID, ok bool) {
+	md := c.mem.Metadata()
+	fromColType := md.ColumnMeta(fromCol).Type
+	equivCols := fds.ComputeEquivGroup(fromCol)
+	equivCols.IntersectionWith(to)
+	for equivCol, ok := equivCols.Next(0); ok; equivCol, ok = equivCols.Next(equivCol + 1) {
+		equivColType := md.ColumnMeta(equivCol).Type
+		if fromColType.Identical(equivColType) {
+			return equivCol, true
+		}
+	}
+	return 0, false
+}
+
 // ----------------------------------------------------------------------
 //
 // Outer column functions

pkg/sql/opt/norm/project_funcs.go

@@ -859,14 +859,10 @@ func (c *CustomFuncs) ProjectRemappedCols(
 ) (projections memo.ProjectionsExpr) {
 	for col, ok := from.Next(0); ok; col, ok = from.Next(col + 1) {
 		if !to.Contains(col) {
-			// TODO(mgartner): We don't need to compute the entire equivalence group,
-			// we only need to find the first equivalent column that's in the to set.
-			candidates := fds.ComputeEquivGroup(col)
-			candidates.IntersectionWith(to)
-			if candidates.Empty() {
+			toCol, ok := c.remapColWithIdenticalType(col, to, fds)
+			if !ok {
 				panic(errors.AssertionFailedf("cannot remap column %v", col))
 			}
-			toCol, _ := candidates.Next(0)
 			projections = append(
 				projections,
 				c.f.ConstructProjectionsItem(c.f.ConstructVariable(toCol), col),
@@ -881,18 +877,6 @@ func (c *CustomFuncs) ProjectRemappedCols(
 func (c *CustomFuncs) RemapProjectionCols(
 	projections memo.ProjectionsExpr, to opt.ColSet, fds *props.FuncDepSet,
 ) memo.ProjectionsExpr {
-	getReplacement := func(col opt.ColumnID) opt.ColumnID {
-		// TODO(mgartner): We don't need to compute the entire equivalence group, we
-		// only need to find the first equivalent column that's in the to set.
-		candidates := fds.ComputeEquivGroup(col)
-		candidates.IntersectionWith(to)
-		if candidates.Empty() {
-			panic(errors.AssertionFailedf("cannot remap column"))
-		}
-		replacement, _ := candidates.Next(0)
-		return replacement
-	}
-
 	// Use the projection outer columns to filter out columns that are synthesized
 	// within the projections themselves (e.g. in a subquery).
 	outerCols := c.ProjectionOuterCols(projections)
@@ -902,7 +886,11 @@ func (c *CustomFuncs) RemapProjectionCols(
 	replace = func(e opt.Expr) opt.Expr {
 		if v, ok := e.(*memo.VariableExpr); ok && !to.Contains(v.Col) && outerCols.Contains(v.Col) {
 			// This variable needs to be remapped.
-			return c.f.ConstructVariable(getReplacement(v.Col))
+			toCol, ok := c.remapColWithIdenticalType(v.Col, to, fds)
+			if !ok {
+				panic(errors.AssertionFailedf("cannot remap column %v", v.Col))
+			}
+			return c.f.ConstructVariable(toCol)
 		}
 		return c.f.Replace(e, replace)
 	}

pkg/sql/opt/norm/testdata/rules/project

@@ -266,6 +266,14 @@ project
  └── projections
       └── child.parent_id:2 [as=parent_id:9, outer=(2)]
 
+exec-ddl
+DROP TABLE child
+----
+
+exec-ddl
+DROP TABLE parent
+----
+
 # The join can be eliminated as long as it doesn't require remapping with
 # OptimizerUseImprovedJoinElimination disabled.
 norm set=optimizer_use_improved_join_elimination=false expect=EliminateJoinUnderProjectLeft
@@ -395,6 +403,87 @@ project
                      └── projections
                           └── NULL [as="?column?":5]
 
+# Regression test for #107850 - columns should not be mapped if their types are
+# not identical.
+exec-ddl
+CREATE TABLE parent (
+  id DECIMAL(10, 2) PRIMARY KEY
+)
+----
+
+exec-ddl
+CREATE TABLE child (
+  id INT PRIMARY KEY,
+  parent_id DECIMAL(10, 0) NOT NULL REFERENCES parent(id),
+  parent_id2 DECIMAL(10, 2) NOT NULL REFERENCES parent(id)
+)
+----
+
+norm expect-not=EliminateJoinUnderProjectRight
+SELECT parent.id FROM parent JOIN child ON parent.id = child.parent_id
+----
+project
+ ├── columns: id:1!null
+ ├── immutable
+ └── inner-join (hash)
+      ├── columns: parent.id:1!null parent_id:5!null
+      ├── multiplicity: left-rows(zero-or-more), right-rows(exactly-one)
+      ├── immutable
+      ├── fd: (1)==(5), (5)==(1)
+      ├── scan parent
+      │    ├── columns: parent.id:1!null
+      │    └── key: (1)
+      ├── scan child
+      │    └── columns: parent_id:5!null
+      └── filters
+           └── parent.id:1 = parent_id:5 [outer=(1,5), immutable, constraints=(/1: (/NULL - ]; /5: (/NULL - ]), fd=(1)==(5), (5)==(1)]
+
+# The projected column with the identical type, child.parent_id2, should be
+# remapped if there are multiple equivalent columns.
+norm
+SELECT parent.id FROM parent JOIN child ON parent.id = child.parent_id AND child.parent_id = child.parent_id2
+----
+project
+ ├── columns: id:1!null
+ ├── immutable
+ ├── select
+ │    ├── columns: parent_id:5!null parent_id2:6!null
+ │    ├── immutable
+ │    ├── fd: (5)==(6), (6)==(5)
+ │    ├── scan child
+ │    │    └── columns: parent_id:5!null parent_id2:6!null
+ │    └── filters
+ │         └── parent_id:5 = parent_id2:6 [outer=(5,6), immutable, constraints=(/5: (/NULL - ]; /6: (/NULL - ]), fd=(5)==(6), (6)==(5)]
+ └── projections
+      └── parent_id2:6 [as=parent.id:1, outer=(6)]
+
+# In a projection expression the column with the identical type,
+# child.parent_id2, should be remapped if there are multiple equivalent columns.
+norm
+SELECT 1 + parent.id FROM parent JOIN child ON parent.id = child.parent_id AND child.parent_id = child.parent_id2
+----
+project
+ ├── columns: "?column?":9!null
+ ├── immutable
+ ├── select
+ │    ├── columns: parent_id:5!null parent_id2:6!null
+ │    ├── immutable
+ │    ├── fd: (5)==(6), (6)==(5)
+ │    ├── scan child
+ │    │    └── columns: parent_id:5!null parent_id2:6!null
+ │    └── filters
+ │         └── parent_id:5 = parent_id2:6 [outer=(5,6), immutable, constraints=(/5: (/NULL - ]; /6: (/NULL - ]), fd=(5)==(6), (6)==(5)]
+ └── projections
+      └── parent_id2:6 + 1 [as="?column?":9, outer=(6), immutable]
+
+exec-ddl
+DROP TABLE child
+----
+
+exec-ddl
+DROP TABLE parent
+----
+
 # --------------------------------------------------
 # EliminateProject
 # --------------------------------------------------

pkg/ui/workspaces/cluster-ui/src/databaseTablePage/databaseTablePage.tsx

@@ -246,6 +246,9 @@ export class DatabaseTablePage extends React.Component<
 
   componentDidMount(): void {
     this.refresh();
+    if (this.props.refreshSettings != null) {
+      this.props.refreshSettings();
+    }
   }
 
   componentDidUpdate(prevProp: Readonly<DatabaseTablePageProps>): void {
@@ -284,10 +287,6 @@ export class DatabaseTablePage extends React.Component<
         this.props.name,
       );
     }
-
-    if (this.props.refreshSettings != null) {
-      this.props.refreshSettings();
-    }
   }
 
   private changeIndexSortSetting(sortSetting: SortSetting) {