opt: Round decimal values before check constraints

The PG spec requires the following DECIMAL type behavior: If the scale of a value to be stored is greater than the declared scale of the column, the system will round the value to the specified number of fractional digits. This is currently happening at the very end of the mutation operator, after any check constraints. This commit moves the rounding to occur before check constraints. Rounding must be performed on inserted and updated values before computed columns are evaluated as well, since computed columns should run on the final values to be inserted/updated. Fixes cockroachdb#35364 Release note (sql change): Computed columns are now evaluated after rounding any decimal values in input columns. Previously, computed columns used input values before rounding.
andy-kimball · Mar 25, 2019 · 06026cc · 06026cc
1 parent 32f1a9f
commit 06026cc
Show file tree

Hide file tree

Showing 23 changed files with 981 additions and 42 deletions.
diff --git a/docs/generated/sql/functions.md b/docs/generated/sql/functions.md
@@ -958,6 +958,8 @@ may increase either contention or retry errors, or both.</p>
 </span></td></tr>
 <tr><td><code>crdb_internal.pretty_key(raw_key: <a href="bytes.html">bytes</a>, skip_fields: <a href="int.html">int</a>) &rarr; <a href="string.html">string</a></code></td><td><span class="funcdesc"><p>This function is used only by CockroachDB’s developers for testing purposes.</p>
 </span></td></tr>
+<tr><td><code>crdb_internal.round_decimal_values(val: anyelement, scale: <a href="int.html">int</a>) &rarr; anyelement</code></td><td><span class="funcdesc"><p>This function is used internally to round decimal values during mutations.</p>
+</span></td></tr>
 <tr><td><code>crdb_internal.set_vmodule(vmodule_string: <a href="string.html">string</a>) &rarr; <a href="int.html">int</a></code></td><td><span class="funcdesc"><p>Set the equivalent of the <code>--vmodule</code> flag on the gateway node processing this request; it affords control over the logging verbosity of different files. Example syntax: <code>crdb_internal.set_vmodule('recordio=2,file=1,gfs*=3')</code>. Reset with: <code>crdb_internal.set_vmodule('')</code>. Raising the verbosity can severely affect performance.</p>
 </span></td></tr>
 <tr><td><code>current_database() &rarr; <a href="string.html">string</a></code></td><td><span class="funcdesc"><p>Returns the current database.</p>

diff --git a/pkg/sql/logictest/testdata/logic_test/insert b/pkg/sql/logictest/testdata/logic_test/insert
@@ -686,3 +686,22 @@ INSERT INTO t35611 (a) VALUES (1)
 
 statement ok
 COMMIT
+
+# ------------------------------------------------------------------------------
+# Regression for #35364.
+# ------------------------------------------------------------------------------
+subtest regression_35364
+
+statement ok
+CREATE TABLE t35364(x DECIMAL(1,0) CHECK (x = 0))
+
+statement ok
+INSERT INTO t35364(x) VALUES (0.1)
+
+query T
+SELECT x FROM t35364
+----
+0
+
+statement ok
+DROP TABLE t35364
diff --git a/pkg/sql/logictest/testdata/logic_test/update b/pkg/sql/logictest/testdata/logic_test/update
@@ -527,3 +527,22 @@ query II
 SELECT * FROM t32054
 ----
 NULL  NULL
+
+# ------------------------------------------------------------------------------
+# Regression for #35364.
+# ------------------------------------------------------------------------------
+subtest regression_35364
+
+statement ok
+CREATE TABLE t35364(x DECIMAL(1,0) CHECK (x >= 1))
+
+statement ok
+INSERT INTO t35364 VALUES (1)
+
+statement ok
+UPDATE t35364 SET x=0.5
+
+query T
+SELECT x FROM t35364
+----
+1
diff --git a/pkg/sql/logictest/testdata/logic_test/upsert b/pkg/sql/logictest/testdata/logic_test/upsert
@@ -868,3 +868,69 @@ INSERT INTO test35040(a,b) VALUES (0,1) ON CONFLICT(a) DO UPDATE SET c = 1111111
 
 statement ok
 DROP TABLE test35040
+
+# ------------------------------------------------------------------------------
+# Regression for #35364.
+# ------------------------------------------------------------------------------
+subtest regression_35364
+
+statement ok
+CREATE TABLE t35364(x INT PRIMARY KEY, y DECIMAL(10,1) CHECK(y >= 8.0), UNIQUE INDEX (y))
+
+statement ok
+INSERT INTO t35364(x, y) VALUES (1, 10.2)
+
+# 10.18 should be mapped to 10.2 before the left outer join so that the conflict
+# can be detected, and 7.95 should be mapped to 8.0 so that check constraint
+# will pass.
+statement ok
+INSERT INTO t35364(x, y) VALUES (2, 10.18) ON CONFLICT (y) DO UPDATE SET y=7.95
+
+query IT
+SELECT * FROM t35364
+----
+1  8.0
+
+statement ok
+DROP TABLE t35364
+
+# Check UPSERT syntax.
+statement ok
+CREATE TABLE t35364(
+    x DECIMAL(10,0) CHECK (x >= 0) PRIMARY KEY,
+    y DECIMAL(10,0) CHECK (y >= 0)
+)
+
+statement ok
+UPSERT INTO t35364 (x) VALUES (-0.1)
+
+query TT
+SELECT * FROM t35364
+----
+-0  NULL
+
+statement ok
+UPSERT INTO t35364 (x, y) VALUES (-0.2, -0.3)
+
+query TT
+SELECT * FROM t35364
+----
+-0  -0
+
+statement ok
+UPSERT INTO t35364 (x, y) VALUES (1.5, 2.5)
+
+query TT
+SELECT * FROM t35364
+----
+-0  -0
+2   3
+
+statement ok
+INSERT INTO t35364 (x) VALUES (1.5) ON CONFLICT (x) DO UPDATE SET x=2.5, y=3.5
+
+query TT
+SELECT * FROM t35364
+----
+-0  -0
+3   4
diff --git a/pkg/sql/opt/cat/column.go b/pkg/sql/opt/cat/column.go
@@ -35,6 +35,26 @@ type Column interface {
 	// DatumType returns the data type of the column.
 	DatumType() types.T
 
+	// ColTypePrecision returns the precision of the column's SQL data type. This
+	// is only defined for the Decimal data type and represents the max number of
+	// decimal digits in the decimal (including fractional digits). If precision
+	// is 0, then the decimal has no max precision.
+	ColTypePrecision() int
+
+	// ColTypeWidth returns the width of the column's SQL data type. This has
+	// different meanings depending on the data type:
+	//
+	//   Decimal  : scale
+	//   Int      : # bits (16, 32, 64, etc)
+	//   Bit Array: # bits
+	//   String   : rune count
+	//
+	// TODO(andyk): It'd be better to expose the attributes of the column type
+	// using a different type or interface. However, currently that's hard to do,
+	// since using sqlbase.ColumnType creates an import cycle, and there's no good
+	// way to create a coltypes.T from sqlbase.ColumnType.
+	ColTypeWidth() int
+
 	// ColTypeStr returns the SQL data type of the column, as a string. Note that
 	// this is sometimes different than DatumType().String(), since datum types
 	// are a subset of column types.

diff --git a/pkg/sql/opt/exec/execbuilder/testdata/insert b/pkg/sql/opt/exec/execbuilder/testdata/insert
@@ -494,3 +494,21 @@ render                    ·         ·                    (z)
                 └── scan  ·         ·                    (a, b, c)                 +c
 ·                         table     abc@abc_c_idx        ·                         ·
 ·                         spans     ALL                  ·                         ·
+
+# ------------------------------------------------------------------------------
+# Regression for #35364. This tests behavior that is different between the CBO
+# and the HP. The CBO will (deliberately) round any input columns *before*
+# evaluating any computed columns, as well as rounding the output.
+# ------------------------------------------------------------------------------
+
+statement ok
+CREATE TABLE t35364(
+    x DECIMAL(10,0) CHECK(ROUND(x) = x) PRIMARY KEY,
+    y DECIMAL(10,0) DEFAULT (1.5),
+    z DECIMAL(10,0) AS (x+y+2.5) STORED CHECK(z >= 7)
+)
+
+query TTT
+INSERT INTO t35364 (x) VALUES (1.5) RETURNING *
+----
+2  2  7
diff --git a/pkg/sql/opt/exec/execbuilder/testdata/update b/pkg/sql/opt/exec/execbuilder/testdata/update
@@ -322,3 +322,26 @@ render                    ·         ·              (a)                       +
                 └── scan  ·         ·              (a, b, c, rowid[hidden])  +c
 ·                         table     abc@abc_c_idx  ·                         ·
 ·                         spans     ALL            ·                         ·
+
+# ------------------------------------------------------------------------------
+# Regression for #35364. This tests behavior that is different between the CBO
+# and the HP. The CBO will (deliberately) round any input columns *before*
+# evaluating any computed columns, as well as rounding the output.
+# ------------------------------------------------------------------------------
+
+statement ok
+CREATE TABLE t35364(
+    x DECIMAL(10,0) CHECK(ROUND(x) = x) PRIMARY KEY,
+    y DECIMAL(10,0) DEFAULT (1.5),
+    z DECIMAL(10,0) AS (x+y+2.5) STORED CHECK(z >= 7)
+)
+
+query TTT
+INSERT INTO t35364 (x) VALUES (1.5) RETURNING *
+----
+2  2  7
+
+query TTT
+UPDATE t35364 SET x=2.5 RETURNING *
+----
+3  2  8
diff --git a/pkg/sql/opt/exec/execbuilder/testdata/upsert b/pkg/sql/opt/exec/execbuilder/testdata/upsert
@@ -342,3 +342,34 @@ render                         ·         ·                    (z)
                      └── scan  ·         ·                    (a, b, c)                    +c
 ·                              table     abc@abc_c_idx        ·                            ·
 ·                              spans     ALL                  ·                            ·
+
+# ------------------------------------------------------------------------------
+# Regression for #35364. This tests behavior that is different between the CBO
+# and the HP. The CBO will (deliberately) round any input columns *before*
+# evaluating any computed columns, as well as rounding the output.
+# ------------------------------------------------------------------------------
+
+statement ok
+CREATE TABLE t35364(
+    x DECIMAL(10,0) CHECK(ROUND(x) = x) PRIMARY KEY,
+    y DECIMAL(10,0) DEFAULT (1.5),
+    z DECIMAL(10,0) AS (x+y+2.5) STORED CHECK(z >= 7)
+)
+
+query TTT
+UPSERT INTO t35364 (x) VALUES (1.5) RETURNING *
+----
+2  2  7
+
+query TTT
+UPSERT INTO t35364 (x, y) VALUES (1.5, 2.5) RETURNING *
+----
+2  3  8
+
+query TTT
+INSERT INTO t35364 (x) VALUES (1.5) ON CONFLICT (x) DO UPDATE SET x=2.5 RETURNING *
+----
+3  3  9
+
+statement error pq: failed to satisfy CHECK constraint \(z >= 7\)
+UPSERT INTO t35364 (x) VALUES (0)
diff --git a/pkg/sql/opt/memo/testdata/logprops/upsert b/pkg/sql/opt/memo/testdata/logprops/upsert
@@ -78,11 +78,11 @@ project
       │    └──  upsert_rowid:20 => rowid:4
       ├── side-effects, mutations
       └── project
-           ├── columns: upsert_a:17(int) upsert_b:18(int) upsert_c:19(int) upsert_rowid:20(int) x:5(int!null) y:6(int!null) column8:8(int) column9:9(int) a:10(int) b:11(int) c:12(int) rowid:13(int)
+           ├── columns: upsert_a:17(int) upsert_b:18(int) upsert_c:19(int) upsert_rowid:20(int) x:5(int!null) y:6(int!null) column8:8(int) column9:9(int) a:10(int) b:11(int) c:12(int) rowid:13(int) column14:14(int!null) column15:15(int) column16:16(int)
            ├── side-effects
            ├── key: (5,13)
-           ├── fd: ()-->(6,9), (5)-->(8), (13)-->(10-12), (10)-->(11-13), (11,12)~~>(10,13), (5,13)-->(17-19), (8,13)-->(20)
-           ├── prune: (5,6,8-13,17-20)
+           ├── fd: ()-->(6,9,14), (5)-->(8), (13)-->(10-12), (10)-->(11-13), (11,12)~~>(10,13), (12)-->(15), (15)-->(16), (5,13)-->(17), (13,15)-->(18), (13,16)-->(19), (8,13)-->(20)
+           ├── prune: (5,6,8-20)
            ├── interesting orderings: (+5) (+6) (+13) (+10) (+11,+12,+13)
            ├── project
            │    ├── columns: column16:16(int) x:5(int!null) y:6(int!null) column8:8(int) column9:9(int) a:10(int) b:11(int) c:12(int) rowid:13(int) column14:14(int!null) column15:15(int)
@@ -400,11 +400,11 @@ project
  │    ├── cardinality: [2 - ]
  │    ├── side-effects, mutations
  │    └── project
- │         ├── columns: upsert_b:14(int) upsert_c:15(int) upsert_rowid:16(int) column1:5(int) column6:6(int!null) column7:7(int) column8:8(int) a:9(int) b:10(int) c:11(int) rowid:12(int)
+ │         ├── columns: upsert_b:14(int) upsert_c:15(int) upsert_rowid:16(int) column1:5(int) column6:6(int!null) column7:7(int) column8:8(int) a:9(int) b:10(int) c:11(int) rowid:12(int) column13:13(int)
  │         ├── cardinality: [2 - ]
  │         ├── side-effects
- │         ├── fd: ()-->(6,8), (12)-->(9-11), (9)-->(10-12), (10,11)~~>(9,12), (10,12)-->(14), (7,12)-->(16)
- │         ├── prune: (5-12,14-16)
+ │         ├── fd: ()-->(6,8), (12)-->(9-11), (9)-->(10-12), (10,11)~~>(9,12), (10)-->(13), (10,12)-->(14), (12,13)-->(15), (7,12)-->(16)
+ │         ├── prune: (5-16)
  │         ├── interesting orderings: (+12) (+9) (+10,+11,+12)
  │         ├── project
  │         │    ├── columns: column13:13(int) column1:5(int) column6:6(int!null) column7:7(int) column8:8(int) a:9(int) b:10(int) c:11(int) rowid:12(int)

diff --git a/pkg/sql/opt/memo/testdata/stats/upsert b/pkg/sql/opt/memo/testdata/stats/upsert
@@ -79,9 +79,9 @@ select
  │    ├── side-effects, mutations
  │    ├── stats: [rows=200, distinct(1)=181.351171, null(1)=0, distinct(2)=200, null(2)=0]
  │    └── project
- │         ├── columns: upsert_x:13(string) upsert_y:14(int) upsert_z:15(float) a:4(int!null) b:5(string!null) column8:8(float) x:9(string) y:10(int) z:11(float)
+ │         ├── columns: upsert_x:13(string) upsert_y:14(int) upsert_z:15(float) a:4(int!null) b:5(string!null) column8:8(float) x:9(string) y:10(int) z:11(float) column12:12(int!null)
  │         ├── stats: [rows=200, distinct(13)=181.351171, null(13)=0, distinct(14)=200, null(14)=0]
- │         ├── fd: ()-->(5,8), (9)-->(10,11), (9)-->(13), (4,9)-->(14)
+ │         ├── fd: ()-->(5,8,12), (9)-->(10,11), (9)-->(13), (4,9)-->(14)
  │         ├── project
  │         │    ├── columns: column12:12(int!null) a:4(int!null) b:5(string!null) column8:8(float) x:9(string) y:10(int) z:11(float)
  │         │    ├── stats: [rows=200, distinct(5,9)=181.351171, null(5,9)=0, distinct(4,9,12)=200, null(4,9,12)=0]

diff --git a/pkg/sql/opt/norm/testdata/rules/prune_cols b/pkg/sql/opt/norm/testdata/rules/prune_cols
@@ -1845,9 +1845,9 @@ upsert a
       └── projections
            └── CASE WHEN k IS NULL THEN column7 ELSE i + 1 END [type=int, outer=(7,9,10)]
 
-# No pruning when RETURNING clause is present.
-# TODO(andyk): Need to prune output columns.
-opt expect-not=(PruneMutationFetchCols,PruneMutationInputCols)
+# Prune update columns replaced by upsert columns.
+# TODO(andyk): Need to also prune output columns.
+opt expect=PruneMutationInputCols expect-not=PruneMutationFetchCols
 INSERT INTO a (k, s) VALUES (1, 'foo') ON CONFLICT (k) DO UPDATE SET i=a.i+1 RETURNING *
 ----
 upsert a

diff --git a/pkg/sql/opt/optbuilder/insert.go b/pkg/sql/opt/optbuilder/insert.go
@@ -223,11 +223,19 @@ func (b *Builder) buildInsert(ins *tree.Insert, inScope *scope) (outScope *scope
 		mb.buildInputForInsert(inScope, nil /* rows */)
 	}
 
-	// Add default and computed columns that were not explicitly specified by
-	// name or implicitly targeted by input columns. This includes any columns
-	// undergoing write mutations, as they must always have a default or computed
-	// value.
-	mb.addDefaultAndComputedColsForInsert()
+	// Add default columns that were not explicitly specified by name or
+	// implicitly targeted by input columns. This includes columns undergoing
+	// write mutations, if they have a default value.
+	mb.addDefaultColsForInsert()
+
+	// Possibly round DECIMAL-related columns containing insertion values. Do
+	// this before evaluating computed expressions, since those may depend on
+	// the inserted columns.
+	mb.roundDecimalValues(mb.insertOrds, false /* roundComputedCols */)
+
+	// Add any computed columns. This includes columns undergoing write mutations,
+	// if they have a computed value.
+	mb.addComputedColsForInsert()
 
 	var returning tree.ReturningExprs
 	if resultsNeeded(ins.Returning) {
@@ -284,10 +292,6 @@ func (b *Builder) buildInsert(ins *tree.Insert, inScope *scope) (outScope *scope
 		// Build each of the SET expressions.
 		mb.addUpdateCols(ins.OnConflict.Exprs)
 
-		// Add additional columns for computed expressions that may depend on any
-		// updated columns.
-		mb.addComputedColsForUpdate()
-
 		// Build the final upsert statement, including any returned expressions.
 		mb.buildUpsert(returning)
 	}
@@ -565,27 +569,31 @@ func (mb *mutationBuilder) buildInputForInsert(inScope *scope, inputRows *tree.S
 	}
 }
 
-// addDefaultAndComputedColsForInsert wraps an Insert input expression with
-// Project operator(s) containing any default (or nullable) and computed columns
-// that are not yet part of the target column list. This includes mutation
-// columns, since they must always have default or computed values.
-//
-// After this call, the input expression will provide values for every one of
-// the target table columns, whether it was explicitly specified or implicitly
-// added.
-func (mb *mutationBuilder) addDefaultAndComputedColsForInsert() {
-	// Add any missing default and nullable columns.
+// addDefaultColsForInsert wraps an Insert input expression with a Project
+// operator containing any default (or nullable) columns that are not yet part
+// of the target column list. This includes mutation columns, since they must
+// always have default or computed values.
+func (mb *mutationBuilder) addDefaultColsForInsert() {
 	mb.addSynthesizedCols(
 		mb.insertOrds,
 		func(tabCol cat.Column) bool { return !tabCol.IsComputed() },
 	)
+}
 
-	// Add any missing computed columns. This must be done after adding default
-	// columns above, because computed columns can depend on default columns.
+// addComputedColsForInsert wraps an Insert input expression with a Project
+// operator containing computed columns that are not yet part of the target
+// column list. This includes mutation columns, since they must always have
+// default or computed values. This must be done after calling
+// addDefaultColsForInsert, because computed columns can depend on default
+// columns.
+func (mb *mutationBuilder) addComputedColsForInsert() {
 	mb.addSynthesizedCols(
 		mb.insertOrds,
 		func(tabCol cat.Column) bool { return tabCol.IsComputed() },
 	)
+
+	// Possibly round DECIMAL-related computed columns.
+	mb.roundDecimalValues(mb.insertOrds, true /* roundComputedCols */)
 }
 
 // buildInsert constructs an Insert operator, possibly wrapped by a Project