opt: fix buggy EliminateUpsertDistinctOnNoColumns rule#46408
opt: fix buggy EliminateUpsertDistinctOnNoColumns rule#46408craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
|
This change is |
RaduBerinde
left a comment
There was a problem hiding this comment.
Reviewable status:
complete! 1 of 0 LGTMs obtained (waiting on @andy-kimball and @RaduBerinde)
pkg/sql/opt/norm/rules/groupby.opt, line 165 at r1 (raw file):
# EliminateErrorDistinctNoColumns is similar to EliminateDistinctNoColumns, # except that it will raise an error if there are no grouping columns and the
[nit] phrasing here is a bit confusing (the rule itself doesn't raise errors). Maybe "except Max1Row is used to raise an error if"
andy-kimball
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/opt/norm/rules/groupby.opt, line 165 at r1 (raw file):
Previously, RaduBerinde wrote…
[nit] phrasing here is a bit confusing (the rule itself doesn't raise errors). Maybe "except Max1Row is used to raise an error if"
Done.
rytaft
left a comment
There was a problem hiding this comment.
Reviewed 4 of 5 files at r1, 1 of 1 files at r2.
Reviewable status:
pkg/sql/opt/norm/groupby.go, line 33 at r2 (raw file):
// RaiseErrorOnDup returns true if an UpsertDistinct operator raises an error // when duplicate values are detected. func (c *CustomFuncs) RaiseErrorOnDup(private *memo.GroupingPrivate) bool {
[nit] maybe change this to "RaisesErrorOnDup"? Currently sounds like this function will raise an error.
The fix for cockroachdb#37880 added a new ErrorOnDup field to the UpsertDistinctOn operator. However, the EliminateErrorDistinctNoColumns rule wasn't changed to respect this flag, and so there's still a case where ON CONFLICT DO NOTHING returns an error. This commit eliminates the error by separating out the ErrorOnDup=True case from the EliminateErrorDistinctNoColumns rule (which raises an error) and adding it instead to the EliminateDistinctNoColumns rule (which does not raise an error). Fixes cockroachdb#46395 Release justification: Bug fixes and low-risk updates to new functionality. This was always an error, so it's low-risk to make it a non-error. Release note: None
andy-kimball
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/opt/norm/groupby.go, line 33 at r2 (raw file):
Previously, rytaft (Rebecca Taft) wrote…
[nit] maybe change this to "RaisesErrorOnDup"? Currently sounds like this function will raise an error.
Done.
rytaft
left a comment
There was a problem hiding this comment.
Reviewed 2 of 2 files at r3.
Reviewable status:
|
bors r+ |
Build succeeded |
The fix for #37880 added a new ErrorOnDup field to the UpsertDistinctOn
operator. However, the EliminateErrorDistinctNoColumns rule wasn't changed
to respect this flag, and so there's still a case where ON CONFLICT DO
NOTHING returns an error.
This commit eliminates the error by separating out the ErrorOnDup=True case
from the EliminateErrorDistinctNoColumns rule (which raises an error) and
adding it instead to the EliminateDistinctNoColumns rule (which does not
raise an error).
Fixes #46395
Release justification: Bug fixes and low-risk updates to new functionality.
This was always an error, so it's low-risk to make it a non-error.
Release note: None