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 009021e
Show file tree
Hide file tree
Showing 5 changed files with 102 additions and 32 deletions.
14 changes: 14 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,17 @@ query FFF
SELECT -0.1234567890123456, 123456789012345.6, 1234567.890123456
----
-0.1234567890123457 123456789012345.7 1234567.890123457

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
67 changes: 36 additions & 31 deletions pkg/sql/opt/norm/general_funcs.go
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,12 @@ func (c *CustomFuncs) IsTimestampTZ(scalar opt.ScalarExpr) bool {
return scalar.DataType().Family() == types.TimestampTZFamily
}

// 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 Expand Up @@ -329,7 +335,7 @@ func (c *CustomFuncs) OuterCols(e opt.Expr) opt.ColSet {
// column, or in other words, a reference to a variable that is not bound within
// its own scope. For example:
//
// SELECT * FROM a WHERE EXISTS(SELECT * FROM b WHERE b.x = a.x)
// SELECT * FROM a WHERE EXISTS(SELECT * FROM b WHERE b.x = a.x)
//
// The a.x variable in the EXISTS subquery references a column outside the scope
// of the subquery. It is an "outer column" for the subquery (see the comment on
Expand All @@ -340,11 +346,12 @@ func (c *CustomFuncs) HasOuterCols(input opt.Expr) bool {

// IsCorrelated returns true if any variable in the source expression references
// a column from the given set of output columns. For example:
// (InnerJoin
// (Scan a)
// (Scan b)
// [ ... (FiltersItem $item:(Eq (Variable a.x) (Const 1))) ... ]
// )
//
// (InnerJoin
// (Scan a)
// (Scan b)
// [ ... (FiltersItem $item:(Eq (Variable a.x) (Const 1))) ... ]
// )
//
// The $item expression is correlated with the (Scan a) expression because it
// references one of its columns. But the $item expression is not correlated
Expand All @@ -356,11 +363,11 @@ func (c *CustomFuncs) IsCorrelated(src memo.RelExpr, cols opt.ColSet) bool {
// IsBoundBy returns true if all outer references in the source expression are
// bound by the given columns. For example:
//
// (InnerJoin
// (Scan a)
// (Scan b)
// [ ... $item:(FiltersItem (Eq (Variable a.x) (Const 1))) ... ]
// )
// (InnerJoin
// (Scan a)
// (Scan b)
// [ ... $item:(FiltersItem (Eq (Variable a.x) (Const 1))) ... ]
// )
//
// The $item expression is fully bound by the output columns of the (Scan a)
// expression because all of its outer references are satisfied by the columns
Expand Down Expand Up @@ -413,11 +420,11 @@ func (c *CustomFuncs) FilterOuterCols(filters memo.FiltersExpr) opt.ColSet {
// FiltersBoundBy returns true if all outer references in any of the filter
// conditions are bound by the given columns. For example:
//
// (InnerJoin
// (Scan a)
// (Scan b)
// $filters:[ (FiltersItem (Eq (Variable a.x) (Const 1))) ]
// )
// (InnerJoin
// (Scan a)
// (Scan b)
// $filters:[ (FiltersItem (Eq (Variable a.x) (Const 1))) ]
// )
//
// The $filters expression is fully bound by the output columns of the (Scan a)
// expression because all of its outer references are satisfied by the columns
Expand Down Expand Up @@ -714,14 +721,14 @@ func (c *CustomFuncs) ReplaceFiltersItem(
// from the given list that are fully bound by the given columns (i.e. all
// outer references are to one of these columns). For example:
//
// (InnerJoin
// (Scan a)
// (Scan b)
// (Filters [
// (Eq (Variable a.x) (Variable b.x))
// (Gt (Variable a.x) (Const 1))
// ])
// )
// (InnerJoin
// (Scan a)
// (Scan b)
// (Filters [
// (Eq (Variable a.x) (Variable b.x))
// (Gt (Variable a.x) (Const 1))
// ])
// )
//
// Calling ExtractBoundConditions with the filter conditions list and the output
// columns of (Scan a) would extract the (Gt) expression, since its outer
Expand Down Expand Up @@ -908,11 +915,10 @@ func (c *CustomFuncs) AppendAggCols(
// function of the given operator type for each of column in the given set. For
// example, for ConstAggOp and columns (1,2), this expression is returned:
//
// (Aggregations
// [(ConstAgg (Variable 1)) (ConstAgg (Variable 2))]
// [1,2]
// )
//
// (Aggregations
// [(ConstAgg (Variable 1)) (ConstAgg (Variable 2))]
// [1,2]
// )
func (c *CustomFuncs) MakeAggCols(aggOp opt.Operator, cols opt.ColSet) memo.AggregationsExpr {
colsLen := cols.Len()
aggs := make(memo.AggregationsExpr, colsLen)
Expand Down Expand Up @@ -982,8 +988,7 @@ func (c *CustomFuncs) IsPositiveInt(datum tree.Datum) bool {
// For example, NormalizeCmpTimeZoneFunction uses this function implicitly to
// match a specific function, like so:
//
// (Function $args:* (FunctionPrivate "timezone"))
//
// (Function $args:* (FunctionPrivate "timezone"))
func (c *CustomFuncs) EqualsString(left string, right string) bool {
return left == right
}
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 009021e

Please sign in to comment.