Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

opt: factor check constraints into Scan FDs #47159

Merged
merged 2 commits into from Apr 9, 2020

Conversation

RaduBerinde
Copy link
Member

opt: build check constraint filters upfront

We use check constraint filters to try to improve index constraint generation. We
build all check constraints as scalars and store them in the TableMeta; then we
create the FiltersExpr in GenerateConstrainedScans.

With this change, we now create the filters upfront. This will allow other uses
for the filters (like better FD generation for Scans).

Release note: None

opt: factor check constraints into Scan FDs

The provided orderings logic has a fragility: if a child operator is "smarter"
about FDs than a parent operator, the parent operator might not be able to
calculate a provided ordering (the child ordering would seem incomplete). This
can happen when that child operator was created during exploration.

This change fixes an instance of this where the canonical scan doesn't take the
check constraints into account, but a constrained scan indirectly does (when the
checks are incorporated into the constraint). The fix is to take into account
the check constraints when calculating the Scan's FDs (which seems like a good
thing to do regardless of this issue).

Here's an example (simplified from #47041):

CREATE TABLE t (
  k INT8 PRIMARY KEY,
  a INT8 NOT NULL,
  b INT8 NOT NULL CHECK (b = 0),
  INDEX ba (b, a)
)
SELECT 1 FROM t WHERE a > 1 AND k > 0 GROUP BY b, a ORDER BY b

The optimized tree for this statement is a Project on top of DistinctOn on top
of a constrained Scan. DistinctOn requires +b,+a from its input (because it's
a streaming aggregation). The input provided ordering is just +a because the
constrained scan knows that b is constant. But the Select does not know that
b is constant, so DistinctOn does not realize that +a is not needed to
satisfy the required ordering. When we try to project away column a, we don't
know what to do with the +a ordering.

Fixes #47041.

Release note (bug fix): fixed an internal "no output column equivalent to X"
error in some very rare cases.

@RaduBerinde RaduBerinde requested a review from a team as a code owner April 7, 2020 23:17
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@blathers-crl
Copy link

blathers-crl bot commented Apr 7, 2020

❌ The GitHub CI (Cockroach) build has failed on 08451a6e.

🦉 Hoot! I am a Blathers, a bot for CockroachDB. I am experimental - my owner is otan.

@blathers-crl
Copy link

blathers-crl bot commented Apr 8, 2020

❌ The GitHub CI (Cockroach) build has failed on 9ccd7650.

🦉 Hoot! I am a Blathers, a bot for CockroachDB. I am experimental - my owner is otan.

Copy link
Contributor

@andy-kimball andy-kimball left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @andy-kimball, @mgartner, and @rytaft)

Copy link
Collaborator

@rytaft rytaft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 10 of 10 files at r1, 4 of 4 files at r2.
Reviewable status: :shipit: complete! 2 of 0 LGTMs obtained (waiting on @mgartner and @RaduBerinde)


pkg/sql/opt/table_meta.go, line 135 at r1 (raw file):

	IgnoreForeignKeys bool

	// Constraints stores a *FiltersExpr containing filters that are known to

Is there any reason not to make the type *FiltersExpr then?


pkg/sql/opt/xform/testdata/physprops/ordering, line 2054 at r2 (raw file):

----

opt

[nit] are we hiding the ordering info from the output?

Copy link
Member Author

@RaduBerinde RaduBerinde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! 2 of 0 LGTMs obtained (waiting on @mgartner and @rytaft)


pkg/sql/opt/table_meta.go, line 135 at r1 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

Is there any reason not to make the type *FiltersExpr then?

Package deps :/


pkg/sql/opt/xform/testdata/physprops/ordering, line 2054 at r2 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

[nit] are we hiding the ordering info from the output?

No, there is no ordering here once the fact that b is constant is factored in.

Copy link
Collaborator

@rytaft rytaft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! 2 of 0 LGTMs obtained (waiting on @mgartner)


pkg/sql/opt/table_meta.go, line 135 at r1 (raw file):

Previously, RaduBerinde wrote…

Package deps :/

Ah, right


pkg/sql/opt/xform/testdata/physprops/ordering, line 2054 at r2 (raw file):

Previously, RaduBerinde wrote…

No, there is no ordering here once the fact that b is constant is factored in.

👍

Copy link
Collaborator

@mgartner mgartner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! 2 of 0 LGTMs obtained (waiting on @RaduBerinde)


pkg/sql/opt/optbuilder/select.go, line 579 at r1 (raw file):

	var filters memo.FiltersExpr
	for ; chkIdx < numChecks; chkIdx++ {

Took me a few minutes to understand what you're doing here with chkIdx and above, but very cool. You're skipping ahead to the first validated constraint rather than starting at the first again.

nit: add a quick comment here to explain that? If it's a common pattern we use then don't worry about it.

@rytaft
Copy link
Collaborator

rytaft commented Apr 9, 2020


pkg/sql/opt/optbuilder/select.go, line 579 at r1 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

Took me a few minutes to understand what you're doing here with chkIdx and above, but very cool. You're skipping ahead to the first validated constraint rather than starting at the first again.

nit: add a quick comment here to explain that? If it's a common pattern we use then don't worry about it.

+1 -- also took me a minute to see what was happening

We use check constraint filters to try to improve index constraint generation.
We build all check constraints as scalars and store them in the TableMeta; then
we create the FiltersExpr in GenerateConstrainedScans.

With this change, we now create the filters upfront. This will allow other uses
for the filters (like better FD generation for Scans).

Release note: None
The provided orderings logic has a fragility: if a child operator is "smarter"
about FDs than a parent operator, the parent operator might not be able to
calculate a provided ordering (the child ordering would seem incomplete). This
can happen when that child operator was created during exploration.

This change fixes an instance of this where the canonical scan doesn't take the
check constraints into account, but a constrained scan indirectly does (when the
checks are incorporated into the constraint). The fix is to take into account
the check constraints when calculating the Scan's FDs (which seems like a good
thing to do regardless of this issue).

Here's an example (simplified from cockroachdb#47041):
```
CREATE TABLE t (
  k INT8 PRIMARY KEY,
  a INT8 NOT NULL,
  b INT8 NOT NULL CHECK (b = 0),
  INDEX ba (b, a)
)
SELECT 1 FROM t WHERE a > 1 AND k > 0 GROUP BY b, a ORDER BY b
```

The optimized tree for this statement is a Project on top of DistinctOn on top
of a constrained Scan. DistinctOn requires `+b,+a` from its input (because it's
a streaming aggregation). The input provided ordering is just `+a` because the
constrained scan knows that `b` is constant. But the Select does not know that
`b` is constant, so DistinctOn does not realize that `+a` is not needed to
satisfy the required ordering. When we try to project away column `a`, we don't
know what to do with the `+a` ordering.

Fixes cockroachdb#47041.

Release note (bug fix): fixed an internal "no output column equivalent to X"
error in some very rare cases.
Copy link
Member Author

@RaduBerinde RaduBerinde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! 2 of 0 LGTMs obtained (waiting on @mgartner)


pkg/sql/opt/optbuilder/select.go, line 579 at r1 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

+1 -- also took me a minute to see what was happening

Done.

Copy link
Collaborator

@rytaft rytaft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 4 of 4 files at r4.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 2 stale) (waiting on @mgartner)

@RaduBerinde
Copy link
Member Author

bors r+

@craig
Copy link
Contributor

craig bot commented Apr 9, 2020

Build succeeded

@craig craig bot merged commit 4bdcec3 into cockroachdb:master Apr 9, 2020
@RaduBerinde RaduBerinde deleted the scan-check-props branch April 9, 2020 16:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SELECT statement on table with a hash-sharded index results in an internal error
5 participants