Skip to content

Commit

Permalink
norm: do not fold floor division with unit denominator
Browse files Browse the repository at this point in the history
The floor division operator `//` is like division but drops the
fractional portion of the result. Normalization rule FoldDivOne
rewrites `(column // 1)` as `(column)` which is incorrect if the
data in `column` has fractional digits, as those digits should be
dropped.

The solution is to only fold `(column // 1)` when `column` is one of
the integer types.

Release note (bug fix): This patch fixes incorrect results from
the floor division operator, `//`, when the numerator is non-constant
and the denominator is the constant 1.
  • Loading branch information
Mark Sirek committed Oct 4, 2022
1 parent 09c8f05 commit f992971
Show file tree
Hide file tree
Showing 5 changed files with 98 additions and 1 deletion.
34 changes: 34 additions & 0 deletions pkg/sql/logictest/testdata/logic_test/float
Original file line number Diff line number Diff line change
Expand Up @@ -139,3 +139,37 @@ query FFF
SELECT -0.1234567890123456, 123456789012345.6, 1234567.890123456
----
-0.1234567890123457 123456789012345.7 1234567.890123457

# Regression test to make sure `IS NAN` does not coerce NaN to a string.
statement error unsupported comparison operator: <tuple> = <decimal>
SELECT ROW() IS NAN

statement error unsupported comparison operator: <tuple> != <decimal>
SELECT ROW() IS NOT NAN

statement error unsupported comparison operator: <string> = <decimal>
SELECT 'NaN'::string IS NAN

query BB
SELECT 'nan'::float IS NAN, 'nan'::float IS NOT NAN
----
true false

query BB
SELECT 'nan'::decimal IS NAN, 'nan'::decimal IS NOT NAN
----
true false

statement ok
CREATE TABLE t87605 (col2 FLOAT8 NULL)

statement ok
insert into t87605 values (1.234567890123456e+13), (1.234567890123456e+6)

# Regression test for issue #87605.
# The floor division operator `//` should not be folded away.
query F
SELECT ((col2::FLOAT8 // 1.0:::FLOAT8::FLOAT8)::FLOAT8) FROM t87605@[0] ORDER BY 1
----
1.234567e+06
1.2345678901234e+13
12 changes: 12 additions & 0 deletions pkg/sql/opt/norm/general_funcs.go
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,18 @@ func (c *CustomFuncs) IsTimestampTZ(scalar opt.ScalarExpr) bool {
return scalar.DataType().Family() == types.TimestampTZFamily
}

// IsJSON returns true if the given scalar expression is of type
// JSON.
func (c *CustomFuncs) IsJSON(scalar opt.ScalarExpr) bool {
return scalar.DataType().Family() == types.JsonFamily
}

// IsInt returns true if the given scalar expression is of one of the
// integer types.
func (c *CustomFuncs) IsInt(scalar opt.ScalarExpr) bool {
return scalar.DataType().Family() == types.IntFamily
}

// BoolType returns the boolean SQL type.
func (c *CustomFuncs) BoolType() *types.T {
return types.Bool
Expand Down
8 changes: 7 additions & 1 deletion pkg/sql/opt/norm/rules/numeric.opt
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,13 @@

# FoldDivOne folds $left / 1 for numeric types.
[FoldDivOne, Normalize]
(Div | FloorDiv $left:* $right:(Const 1))
(Div $left:* $right:(Const 1))
=>
(Cast $left (BinaryType (OpName) $left $right))

# FoldFloorDivOne folds $left // 1 for integer types.
[FoldFloorDivOne, Normalize]
(FloorDiv $left:* $right:(Const 1) & (IsInt $left))
=>
(Cast $left (BinaryType (OpName) $left $right))

Expand Down
21 changes: 21 additions & 0 deletions pkg/sql/opt/norm/testdata/rules/fold_constants
Original file line number Diff line number Diff line change
Expand Up @@ -432,6 +432,27 @@ values
├── fd: ()-->(1)
└── (NULL,)

# Regression test for issue #87605.
norm expect=FoldBinary
SELECT 1.333 // 1.0
----
values
├── columns: "?column?":1!null
├── cardinality: [1 - 1]
├── key: ()
├── fd: ()-->(1)
└── (1,)

norm expect=FoldBinary
SELECT 2::INT // 1.0
----
values
├── columns: "?column?":1!null
├── cardinality: [1 - 1]
├── key: ()
├── fd: ()-->(1)
└── (2,)

# --------------------------------------------------
# FoldUnary
# --------------------------------------------------
Expand Down
24 changes: 24 additions & 0 deletions pkg/sql/opt/norm/testdata/rules/numeric
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,30 @@ project
└── projections
└── i:2::DECIMAL [as="?column?":8, outer=(2), immutable]

# --------------------------------------------------
# FoldFloorDivOne
# --------------------------------------------------

# Regression test for issue #87605.
# The floor division operator `//` should only be folded when the numerator is
# an integer.
norm expect=FoldFloorDivOne
SELECT
a.i // 1 AS r,
a.f // 1 AS s,
a.d // 1 AS t
FROM a
----
project
├── columns: r:8 s:9 t:10
├── immutable
├── scan a
│ └── columns: i:2 f:3 d:4
└── projections
├── i:2 [as=r:8, outer=(2)]
├── f:3 // 1.0 [as=s:9, outer=(3), immutable]
└── d:4 // 1 [as=t:10, outer=(4), immutable]

# --------------------------------------------------
# InvertMinus
# --------------------------------------------------
Expand Down

0 comments on commit f992971

Please sign in to comment.