opt: fix error with DISTINCT ON and ORDER BY ASC NULLS LAST #107842
Conversation
|
This change is |
rytaft
added
backport-22.2.x
backport-23.1.x
There was a problem hiding this comment.
Nice fix! Should we try to handle ordering by expressions also in this PR, or open a separate issue for it? For example:
SELECT
DISTINCT ON (COALESCE(t2.id, t1.id))
t1.id,
t1.str
FROM t1 JOIN t2 ON t1.id = t2.id
ORDER BY COALESCE(t2.id, t1.id) DESC NULLS FIRST;
error (42P10): SELECT DISTINCT ON expressions must match initial ORDER BY expressionsReviewed 3 of 3 files at r1, all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @DrewKimball)
|
TFTR! Let's create a separate issue. |
There was a problem hiding this comment.
Created #107848
Reviewable status:
There was a problem hiding this comment.
Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status:
pkg/sql/opt/optbuilder/distinct.go line 95 at r1 (raw file):
scopeCol := inScope.getColumn(col.ID()) if scopeCol != nil { if isExpr, ok := scopeCol.scalar.(*memo.IsExpr); ok {
Does this bit of logic allow an query like SELECT DISTINCT ON (x) * FROM xy ORDER BY x IS NULL, x;?
There was a problem hiding this comment.
Any ideas why we didn't catch this with sqlsmith with b10363d ?
Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status:
pkg/sql/opt/optbuilder/distinct.go line 95 at r1 (raw file):
Previously, DrewKimball (Drew Kimball) wrote…
Does this bit of logic allow an query like
SELECT DISTINCT ON (x) * FROM xy ORDER BY x IS NULL, x;?
+1 that'd be a good test to add.
rytaft
force-pushed
the
distinct-order-by-bug
branch
from
7346f0a to
f891522
Compare
There was a problem hiding this comment.
TFTRs!
Any ideas why we didn't catch this with sqlsmith with b10363d ?
It doesn't return an internal error, so I don't think we'd catch it in a normal sqlsmith test. We wouldn't catch it in one of the query comparison tests either since we can't really catch optbuilder bugs with those tests.
Reviewable status:
pkg/sql/opt/optbuilder/distinct.go line 95 at r1 (raw file):
Previously, mgartner (Marcus Gartner) wrote…
+1 that'd be a good test to add.
Yes it does (that also worked before this most recent change). I added a test.
There was a problem hiding this comment.
bors r+
Reviewable status:
|
Build failed (retrying...): |
|
Build failed: |
There was a problem hiding this comment.
Flake: #108246
bors r+
Reviewable status:
|
Build failed: |
|
Another flake: #108248 bors r+ |
|
Build failed: |
There was a problem hiding this comment.
Another: #108221
bors r+
Reviewable status:
|
Timed out. |
There was a problem hiding this comment.
bors r+
Reviewable status:
|
Build failed: |
|
Flake: #106960 bors r+ |
|
Build failed: |
This commit fixes an error that could occur in the optbuilder when planning a query with DISTINCT ON and a non-standard nulls ordering. The optimizer supports queries with a non-standard nulls ordering by projecting a column with the expression (col IS NULL) and adding it to the ordering. Since we require that DISTINCT ON columns must be the prefix of any ordering columns, we must account for the new ordering column when building DISTINCT ON. A previous bug fix for cockroachdb#90763 caused the new column to be simply ignored when building DISTINCT ON, but this was insufficient. We need to actually include the new column among the DISTINCT ON columns. This commit makes that change. Fixes cockroachdb#107839 Release note (bug fix): Fixed a spurious error "no data source matches prefix" that could occur during planning for a query with DISTINCT ON and ORDER BY ASC NULLS LAST or ORDER BY DESC NULLS FIRST.
rytaft
force-pushed
the
distinct-order-by-bug
branch
from
f891522 to
9dcdbcd
Compare
|
This is ridiculous 😭 I just rebased in the hopes that some of the flakes are fixed. bors r+ |
|
Build succeeded: |
This commit fixes an error that could occur in the optbuilder when planning a query with
DISTINCT ONand a non-standard nulls ordering.The optimizer supports queries with a non-standard nulls ordering by projecting a column with the expression
(col IS NULL)and adding it to the ordering. Since we require thatDISTINCT ONcolumns must be the prefix of any ordering columns, we must account for the new ordering column when buildingDISTINCT ON. A previous bug fix for #90763 caused the new column to be simply ignored when buildingDISTINCT ON, but this was insufficient. We need to actually include the new column among theDISTINCT ONcolumns. This commit makes that change.Fixes #107839
Release note (bug fix): Fixed a spurious error "no data source matches prefix" that could occur during planning for a query with
DISTINCT ONandORDER BY ASC NULLS LASTorORDER BY DESC NULLS FIRST.