Skip to content

Commit

Permalink
opt: fix invalid transformation in SplitLimitedSelectIntoUnionSelects
Browse files Browse the repository at this point in the history 
This commit removes some untested logic in
`SplitLimitedSelectIntoUnionSelects` that created invalid expression
transformations. With this logic, this rule could construct an unordered
limit below the `UnionAll` which is incorrect. The bug could cause
incorrect query results.

Fixes cockroachdb#88993

Release note (bug fix): A bug has been fixed that could cause incorrect
results in rare cases. The bug could only present if the following
conditions were true:
  1. A query with `ORDER BY` and `LIMIT` was executed.
  2. The table containing the `ORDER BY` columns had an index containing
     those columns.
  3. The index in (2) contained a prefix of columns held to a fixed
     number of values by the query filter (e.g., `WHERE a IN (1, 3)`),
     a `CHECK` constraint (e.g., `CHECK (a IN (1, 3))`), inferred by
     a computed column expression (e.g. `WHERE a IN (1, 3)` and a column
     `b INT AS (a + 10) STORED`), or inferred by a `PARTITION BY` clause
     (e.g. `INDEX (a, ...) PARTITION BY LIST (a) (PARTITION p VALUES
     ((1), (3)))`).
This bug was present since version 22.1.0.
  • Loading branch information
@mgartner
mgartner committed Sep 30, 2022
1 parent a55a1cb commit ff4e568
Show file tree
Hide file tree
Showing 5 changed files with 142 additions and 161 deletions.
1 change: 1 addition & 0 deletions pkg/ccl/logictestccl/testdata/logic_test/partitioning_constrained_scans
View file
Original file line number Diff line number Diff line change
Expand Up @@ -317,3 +317,4 @@ select
│ └── [/'foo'/e'bar\x00'/5 - ]
└── filters
└── c = 5

18 changes: 18 additions & 0 deletions pkg/ccl/logictestccl/testdata/logic_test/partitioning_index
View file
Original file line number Diff line number Diff line change
Expand Up @@ -262,3 +262,21 @@ NULL 10 1
20 NULL 1
20 20 1
25 NULL 1

# Regression test for #88993 where a limit pushed down into a union of scans
# caused incorrect query results.
statement ok
CREATE TABLE t88993 (
a INT,
b INT,
c INT,
INDEX (b, c, a) PARTITION BY LIST (b, c) (
PARTITION p1 VALUES IN ((11, 50))
)
);
INSERT INTO t88993 (a, b, c) VALUES (1, 10, 150), (0, 11, 100);

query I
SELECT min(a) FROM t88993
----
0
16 changes: 16 additions & 0 deletions pkg/sql/logictest/testdata/logic_test/aggregate
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3786,3 +3786,19 @@ query FFFF
select covar_pop(y, x), covar_samp(y, x), regr_sxx(y, x), regr_syy(y, x) from corrupt_combine
----
37.5 45 2983.333333333333 17.5

# Regression test for #88993 where a limit pushed down into a union of scans
# caused incorrect query results.
statement ok
CREATE TABLE t2 (
a INT,
b INT,
c INT,
INDEX (b, c, a)
);
INSERT INTO t2 (a, b, c) VALUES (1, 10, 20), (0, 11, 100);

query I
SELECT min(a) FROM t2 WHERE (b <= 11 AND c < 50) OR (b = 11 AND c = 50) OR (b >= 11 AND c > 50)
----
0
48 changes: 18 additions & 30 deletions pkg/sql/opt/xform/general_funcs.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -569,15 +569,23 @@ func (c *CustomFuncs) splitScanIntoUnionScansOrSelects(
)
newScanOrSelect := c.e.f.ConstructScan(newScanPrivate)
if !filters.IsTrue() {
newScanOrSelect = c.wrapScanInLimitedSelect(
newScanOrSelect = c.e.f.ConstructSelect(
newScanOrSelect,
sp,
newScanPrivate,
filters,
limit,
c.RemapScanColsInFilter(filters, sp, newScanPrivate),
)
}
queue.PushBack(newScanOrSelect)
// It is safe to use an empty ordering for this Limit because this
// is a single key span so a prefix of the index columns are held
// constant for this span, and the remaining columns correspond to
// the ordering columns (see hasOrderingSeq). If a Select was
// applied to the Scan above, the execution engine guarantees that
// the order of the rows from the Scan will be preserved.
newLimit := c.e.f.ConstructLimit(
newScanOrSelect,
c.IntConst(tree.NewDInt(tree.DInt(limit))),
c.EmptyOrdering(),
)
queue.PushBack(newLimit)
}
}

Expand Down Expand Up @@ -654,7 +662,10 @@ func (c *CustomFuncs) splitScanIntoUnionScansOrSelects(
})
newScanOrSelect := c.e.f.ConstructScan(newScanPrivate)
if !filters.IsTrue() {
newScanOrSelect = c.wrapScanInLimitedSelect(newScanOrSelect, sp, newScanPrivate, filters, limit)
newScanOrSelect = c.e.f.ConstructSelect(
newScanOrSelect,
c.RemapScanColsInFilter(filters, sp, newScanPrivate),
)
}
// TODO(mgartner/msirek): Converting ColSets to ColLists here is only safe
// because column IDs are always allocated in a consistent, ascending order
Expand Down Expand Up @@ -711,29 +722,6 @@ func (c *CustomFuncs) numAllowedValues(
return 0, false
}

// wrapScanInLimitedSelect wraps "scan" in a SelectExpr with filters mapped from
// the originalScanPrivate columns to the columns in scan. If limit is non-zero,
// the SelectExpr is wrapped in a LimitExpr with that limit.
func (c *CustomFuncs) wrapScanInLimitedSelect(
scan memo.RelExpr,
originalScanPrivate, newScanPrivate *memo.ScanPrivate,
filters memo.FiltersExpr,
limit int,
) (limitedSelect memo.RelExpr) {
limitedSelect = c.e.f.ConstructSelect(
scan,
c.RemapScanColsInFilter(filters, originalScanPrivate, newScanPrivate),
)
if limit != 0 {
limitedSelect = c.e.f.ConstructLimit(
limitedSelect,
c.IntConst(tree.NewDInt(tree.DInt(limit))),
c.EmptyOrdering(),
)
}
return limitedSelect
}

// indexHasOrderingSequence returns whether the Scan can provide a given
// ordering under the assumption that we are scanning a single-key span with the
// given keyLength (and if so, whether we need to scan it in reverse).
Expand Down
220 changes: 89 additions & 131 deletions pkg/sql/opt/xform/testdata/rules/limit
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2380,9 +2380,9 @@ top-k
│ └── [/10/4 - /10/4]
└── key: (1-4)

# Repro for #82730
# Regression test for #82730.
exec-ddl
CREATE TABLE table1 (
CREATE TABLE t82730a (
col1_0 NAME,
col1_1 INT8,
col1_3 INT8,
Expand Down Expand Up @@ -2413,13 +2413,13 @@ CREATE TABLE table1 (
----

opt expect-not=SplitLimitedSelectIntoUnionSelects
UPDATE table1
UPDATE t82730a
SET col1_5 = col1_5
WHERE col1_0 ILIKE col1_0
ORDER BY col1_3
LIMIT 84
----
update table1
update t82730a
├── columns: <none>
├── fetch columns: col1_0:7 col1_1:8 col1_3:9 col1_5:10
├── update-mapping:
Expand All @@ -2437,15 +2437,15 @@ update table1
├── columns: col1_0:7!null col1_1:8 col1_3:9 col1_5:10
├── key: (7)
├── fd: (7)-->(8-10), (8,9)~~>(7,10)
├── scan table1
├── scan t82730a
│ ├── columns: col1_0:7!null col1_1:8 col1_3:9 col1_5:10
│ ├── key: (7)
│ └── fd: (7)-->(8-10), (8,9)~~>(7,10)
└── filters
└── col1_0:7 ILIKE col1_0:7 [outer=(7), constraints=(/7: (/NULL - ])]

exec-ddl
CREATE TABLE table2 (
CREATE TABLE t82730b (
col1_0 NAME,
col1_1 INT8,
col1_3 INT8,
Expand Down Expand Up @@ -2476,144 +2476,102 @@ CREATE TABLE table2 (
----

opt expect=SplitLimitedSelectIntoUnionSelects
UPDATE table2 AS tab_41831
UPDATE t82730b AS tab_41831
SET col1_5 = col1_5
WHERE col1_0 ILIKE col1_0
ORDER BY col1_3
LIMIT 84
----
update table2 [as=tab_41831]
update t82730b [as=tab_41831]
├── columns: <none>
├── fetch columns: col1_0:7 col1_1:8 col1_3:9 col1_5:10
├── update-mapping:
│ └── col1_5:10 => col1_5:4
├── cardinality: [0 - 0]
├── volatile, mutations
└── index-join table2
└── top-k
├── columns: col1_0:7!null col1_1:8 col1_3:9 col1_5:10
├── internal-ordering: +9
├── k: 84
├── cardinality: [0 - 84]
├── key: (7)
├── fd: (7)-->(8-10), (8,9)~~>(7,10)
└── limit
├── columns: col1_0:7!null col1_1:8 col1_3:9
├── internal-ordering: +9
├── cardinality: [0 - 84]
└── select
├── columns: col1_0:7!null col1_1:8 col1_3:9 col1_5:10
├── key: (7)
├── fd: (7)-->(8,9), (8,9)~~>(7)
├── union-all
│ ├── columns: col1_0:7!null col1_1:8 col1_3:9
│ ├── left columns: col1_0:37 col1_1:38 col1_3:39
│ ├── right columns: col1_0:43 col1_1:44 col1_3:45
│ ├── cardinality: [0 - 336]
│ ├── ordering: +9
│ ├── limit hint: 84.00
│ ├── union-all
│ │ ├── columns: col1_0:37!null col1_1:38!null col1_3:39
│ │ ├── left columns: col1_0:31 col1_1:32 col1_3:33
│ │ ├── right columns: col1_0:25 col1_1:26 col1_3:27
│ │ ├── cardinality: [0 - 252]
│ │ ├── ordering: +39
│ │ ├── limit hint: 84.00
│ │ ├── union-all
│ │ │ ├── columns: col1_0:31!null col1_1:32!null col1_3:33
│ │ │ ├── left columns: col1_0:13 col1_1:14 col1_3:15
│ │ │ ├── right columns: col1_0:19 col1_1:20 col1_3:21
│ │ │ ├── cardinality: [0 - 168]
│ │ │ ├── ordering: +33
│ │ │ ├── limit hint: 84.00
│ │ │ ├── limit
│ │ │ │ ├── columns: col1_0:13!null col1_1:14!null col1_3:15
│ │ │ │ ├── cardinality: [0 - 84]
│ │ │ │ ├── key: (13)
│ │ │ │ ├── fd: ()-->(14), (13)-->(15), (14,15)~~>(13)
│ │ │ │ ├── ordering: +15 opt(14) [actual: +15]
│ │ │ │ ├── limit hint: 84.00
│ │ │ │ ├── select
│ │ │ │ │ ├── columns: col1_0:13!null col1_1:14!null col1_3:15
│ │ │ │ │ ├── key: (13)
│ │ │ │ │ ├── fd: ()-->(14), (13)-->(15), (14,15)~~>(13)
│ │ │ │ │ ├── ordering: +15 opt(14) [actual: +15]
│ │ │ │ │ ├── limit hint: 84.00
│ │ │ │ │ ├── scan table2@table2_col1_1_col1_3_key [as=tab_41831]
│ │ │ │ │ │ ├── columns: col1_0:13!null col1_1:14!null col1_3:15
│ │ │ │ │ │ ├── constraint: /14/15: [/2 - /2]
│ │ │ │ │ │ ├── key: (13)
│ │ │ │ │ │ ├── fd: ()-->(14), (13)-->(15), (14,15)~~>(13)
│ │ │ │ │ │ └── ordering: +15 opt(14) [actual: +15]
│ │ │ │ │ └── filters
│ │ │ │ │ └── col1_0:13 ILIKE col1_0:13 [outer=(13), constraints=(/13: (/NULL - ])]
│ │ │ │ └── 84
│ │ │ └── limit
│ │ │ ├── columns: col1_0:19!null col1_1:20!null col1_3:21
│ │ │ ├── cardinality: [0 - 84]
│ │ │ ├── key: (19)
│ │ │ ├── fd: ()-->(20), (19)-->(21), (20,21)~~>(19)
│ │ │ ├── ordering: +21 opt(20) [actual: +21]
│ │ │ ├── limit hint: 84.00
│ │ │ ├── select
│ │ │ │ ├── columns: col1_0:19!null col1_1:20!null col1_3:21
│ │ │ │ ├── key: (19)
│ │ │ │ ├── fd: ()-->(20), (19)-->(21), (20,21)~~>(19)
│ │ │ │ ├── ordering: +21 opt(20) [actual: +21]
│ │ │ │ ├── limit hint: 84.00
│ │ │ │ ├── scan table2@table2_col1_1_col1_3_key [as=tab_41831]
│ │ │ │ │ ├── columns: col1_0:19!null col1_1:20!null col1_3:21
│ │ │ │ │ ├── constraint: /20/21: [/3 - /3]
│ │ │ │ │ ├── key: (19)
│ │ │ │ │ ├── fd: ()-->(20), (19)-->(21), (20,21)~~>(19)
│ │ │ │ │ └── ordering: +21 opt(20) [actual: +21]
│ │ │ │ └── filters
│ │ │ │ └── col1_0:19 ILIKE col1_0:19 [outer=(19), constraints=(/19: (/NULL - ])]
│ │ │ └── 84
│ │ └── limit
│ │ ├── columns: col1_0:25!null col1_1:26!null col1_3:27
│ │ ├── cardinality: [0 - 84]
│ │ ├── key: (25)
│ │ ├── fd: ()-->(26), (25)-->(27), (26,27)~~>(25)
│ │ ├── ordering: +27 opt(26) [actual: +27]
│ │ ├── limit hint: 84.00
│ │ ├── select
│ │ │ ├── columns: col1_0:25!null col1_1:26!null col1_3:27
│ │ │ ├── key: (25)
│ │ │ ├── fd: ()-->(26), (25)-->(27), (26,27)~~>(25)
│ │ │ ├── ordering: +27 opt(26) [actual: +27]
│ │ │ ├── limit hint: 84.00
│ │ │ ├── scan table2@table2_col1_1_col1_3_key [as=tab_41831]
│ │ │ │ ├── columns: col1_0:25!null col1_1:26!null col1_3:27
│ │ │ │ ├── constraint: /26/27: [/4 - /4]
│ │ │ │ ├── key: (25)
│ │ │ │ ├── fd: ()-->(26), (25)-->(27), (26,27)~~>(25)
│ │ │ │ └── ordering: +27 opt(26) [actual: +27]
│ │ │ └── filters
│ │ │ └── col1_0:25 ILIKE col1_0:25 [outer=(25), constraints=(/25: (/NULL - ])]
│ │ └── 84
│ └── sort
│ ├── columns: col1_0:43!null col1_1:44 col1_3:45
│ ├── cardinality: [0 - 84]
│ ├── key: (43)
│ ├── fd: (43)-->(44,45), (44,45)~~>(43)
│ ├── ordering: +45
│ ├── limit hint: 84.00
│ └── limit
│ ├── columns: col1_0:43!null col1_1:44 col1_3:45
│ ├── cardinality: [0 - 84]
│ ├── key: (43)
│ ├── fd: (43)-->(44,45), (44,45)~~>(43)
│ ├── select
│ │ ├── columns: col1_0:43!null col1_1:44 col1_3:45
│ │ ├── key: (43)
│ │ ├── fd: (43)-->(44,45), (44,45)~~>(43)
│ │ ├── limit hint: 84.00
│ │ ├── scan table2@table2_col1_1_col1_3_key [as=tab_41831]
│ │ │ ├── columns: col1_0:43!null col1_1:44 col1_3:45
│ │ │ ├── constraint: /44/45
│ │ │ │ ├── [/NULL - /0]
│ │ │ │ ├── [/6 - /4999999]
│ │ │ │ └── [/5000001 - ]
│ │ │ ├── key: (43)
│ │ │ ├── fd: (43)-->(44,45), (44,45)~~>(43)
│ │ │ └── limit hint: 252.00
│ │ └── filters
│ │ └── col1_0:43 ILIKE col1_0:43 [outer=(43), constraints=(/43: (/NULL - ])]
│ └── 84
└── 84
├── fd: (7)-->(8-10), (8,9)~~>(7,10)
├── scan t82730b [as=tab_41831]
│ ├── columns: col1_0:7!null col1_1:8 col1_3:9 col1_5:10
│ ├── key: (7)
│ └── fd: (7)-->(8-10), (8,9)~~>(7,10)
└── filters
└── col1_0:7 ILIKE col1_0:7 [outer=(7), constraints=(/7: (/NULL - ])]

# Regression test for #88993. Do not construct an unordered limit below the
# UnionAll.
exec-ddl
CREATE TABLE t88993 (
a INT,
b INT,
c INT,
INDEX bca_idx (b, c, a)
);
----

opt disable=GenerateTopK
SELECT min(a) FROM t88993@bca_idx WHERE (b <= 11 AND c < 50) OR (b = 11 AND c = 50) OR (b >= 11 AND c > 50)
----
scalar-group-by
├── columns: min:7
├── cardinality: [1 - 1]
├── key: ()
├── fd: ()-->(7)
├── limit
│ ├── columns: a:1!null b:2!null c:3!null
│ ├── internal-ordering: +1
│ ├── cardinality: [0 - 1]
│ ├── key: ()
│ ├── fd: ()-->(1-3)
│ ├── union-all
│ │ ├── columns: a:1!null b:2!null c:3!null
│ │ ├── left columns: a:8 b:9 c:10
│ │ ├── right columns: a:14 b:15 c:16
│ │ ├── ordering: +1
│ │ ├── limit hint: 1.00
│ │ ├── limit
│ │ │ ├── columns: a:8!null b:9!null c:10!null
│ │ │ ├── cardinality: [0 - 1]
│ │ │ ├── key: ()
│ │ │ ├── fd: ()-->(8-10)
│ │ │ ├── limit hint: 1.00
│ │ │ ├── select
│ │ │ │ ├── columns: a:8!null b:9!null c:10!null
│ │ │ │ ├── fd: ()-->(9,10)
│ │ │ │ ├── limit hint: 1.00
│ │ │ │ ├── scan t88993@bca_idx
│ │ │ │ │ ├── columns: a:8!null b:9!null c:10!null
│ │ │ │ │ ├── constraint: /9/10/8/11: (/11/50/NULL - /11/50]
│ │ │ │ │ ├── flags: force-index=bca_idx
│ │ │ │ │ └── fd: ()-->(9,10)
│ │ │ │ └── filters
│ │ │ │ └── (((b:9 <= 11) AND (c:10 < 50)) OR ((b:9 = 11) AND (c:10 = 50))) OR ((b:9 >= 11) AND (c:10 > 50)) [outer=(9,10), constraints=(/9: (/NULL - ]; /10: (/NULL - /49] [/50 - /50] [/51 - ])]
│ │ │ └── 1
│ │ └── sort
│ │ ├── columns: a:14!null b:15!null c:16!null
│ │ ├── ordering: +14
│ │ ├── limit hint: 1.00
│ │ └── select
│ │ ├── columns: a:14!null b:15!null c:16!null
│ │ ├── scan t88993@bca_idx
│ │ │ ├── columns: a:14 b:15!null c:16
│ │ │ ├── constraint: /15/16/14/17
│ │ │ │ ├── (/NULL - /11/49]
│ │ │ │ └── (/11/51/NULL - ]
│ │ │ └── flags: force-index=bca_idx
│ │ └── filters
│ │ ├── (((b:15 <= 11) AND (c:16 < 50)) OR ((b:15 = 11) AND (c:16 = 50))) OR ((b:15 >= 11) AND (c:16 > 50)) [outer=(15,16), constraints=(/15: (/NULL - ]; /16: (/NULL - /49] [/50 - /50] [/51 - ])]
│ │ └── a:14 IS NOT NULL [outer=(14), constraints=(/14: (/NULL - ]; tight)]
│ └── 1
└── aggregations
└── const-agg [as=min:7, outer=(1)]
└── a:1

0 comments on commit ff4e568

Please sign in to comment.