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: inline all filters with virtual columns #60553

Merged
merged 2 commits into from
Feb 16, 2021

Conversation

mgartner
Copy link
Collaborator

@mgartner mgartner commented Feb 13, 2021

opt: inline all filters with virtual columns

Previously, InlineSelectVirtualColumns inlined virtual column
expressions in filters and pushed those filters below a Project only if
the virtual columns were indexed. Now, all virtual column expressions
are inlined. This allows exploration rules to generate expressions that
utilize partial indexes with predicates that reference virtual columns.

We considered extending InlineSelectVirtualColumns to also inline a
virtual columns if they are referenced in partial index predicates.
This would require addition metadata to keep track of the set of such
virtual columns. Virtual columns have already been inlined in the
predicates in the table metadata, so simply fetching the outer columns
of those predicates would not suffice. Because this rule already is
highly selective (only operating on virtual columns), we opted to
simplify the rule to inline all virtual columns instead.

There is no release note because virtual columns are gated behind an
experimental session setting.

Release note: None

opt: fix optsteps panic from InlineSelectVirtualColumns

Previously, running the optsteps test command panicked with "could not
find path to expr" for queries when InlineSelectVirtualColumns was
matched. This commit fixes the panic by eliminating the anti-pattern of
calling a constructor during the pattern-match phase of a normalization
rule.

The optsteps test command displays step n of optimization by running
the optimizer twice and halting after n and n - 1 rules have been
applied. The difference in the two resulting expressions represents the
change in step n. InlineSelectVirtualColumns previously called
ConstructFiltersItem during the pattern-match phase of the rule, but
only added the constructed filters item to the memo in the replace
phase of the rule. When some normalization rule applied during the call
to ConstructFiltersItem was the last rule applied in a step, the
constructed item was not added to the memo because the limit on the
number of rules prevented the replace phase of
InlineSelectVirtualColumns. As a result, optsteps panicked when it
could not find the normalized FiltersItem in the memo.

Release note: None

@mgartner mgartner requested a review from a team as a code owner February 13, 2021 00:12
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@mgartner mgartner force-pushed the inline-all-virtual-columns branch 3 times, most recently from 7be450f to 8cccb58 Compare February 16, 2021 01:29
Copy link
Member

@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.

:lgtm:

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @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 4 of 4 files at r1, 3 of 3 files at r2.
Reviewable status: :shipit: complete! 2 of 0 LGTMs obtained (waiting on @mgartner and @RaduBerinde)


pkg/sql/opt/norm/inline_funcs.go, line 244 at r2 (raw file):

		}

		// Initialize inlined lazily.

inlined -> inlinableFilters


pkg/sql/opt/norm/rules/inline.opt, line 189 at r2 (raw file):

#     a INT,
#     b INT,
#     v INT AS (abs(a)),

should this have the tag VIRTUAL?


pkg/sql/opt/xform/testdata/rules/select, line 1735 at r1 (raw file):

           └── b:3 = 10 [outer=(3), constraints=(/3: [/10 - /10]; tight), fd=()-->(3)]

# Test that we can constrain an partial index with a virtual column in the

[nit] an partial -> a partial

Previously, `InlineSelectVirtualColumns` inlined virtual column
expressions in filters and pushed those filters below a Project only if
the virtual columns were indexed. Now, all virtual column expressions
are inlined. This allows exploration rules to generate expressions that
utilize partial indexes with predicates that reference virtual columns.

We considered extending `InlineSelectVirtualColumns` to also inline a
virtual columns if they are referenced in partial index predicates.
This would require addition metadata to keep track of the set of such
virtual columns. Virtual columns have already been inlined in the
predicates in the table metadata, so simply fetching the outer columns
of those predicates would not suffice. Because this rule already is
highly selective (only operating on virtual columns), we opted to
simplify the rule to inline all virtual columns instead.

There is no release note because virtual columns are gated behind an
experimental session setting.

Release note: None
Previously, running the `optsteps` test command panicked with "could not
find path to expr" for queries when `InlineSelectVirtualColumns` was
matched. This commit fixes the panic by eliminating the anti-pattern of
calling a constructor during the pattern-match phase of a normalization
rule.

The `optsteps` test command displays step `n` of optimization by running
the optimizer twice and halting after `n` and `n - 1` rules have been
applied. The difference in the two resulting expressions represents the
change in step `n`. `InlineSelectVirtualColumns` previously called
`ConstructFiltersItem` during the pattern-match phase of the rule, but
only added the constructed filters item to the memo in the replace
phase of the rule. When some normalization rule applied during the call
to `ConstructFiltersItem` was the last rule applied in a step, the
constructed item was not added to the memo because the limit on the
number of rules prevented the replace phase of
`InlineSelectVirtualColumns`. As a result, `optsteps` panicked when it
could not find the normalized `FiltersItem` in the memo.

Release note: None
Copy link
Collaborator Author

@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 @rytaft)


pkg/sql/opt/norm/inline_funcs.go, line 244 at r2 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

inlined -> inlinableFilters

Done.


pkg/sql/opt/norm/rules/inline.opt, line 189 at r2 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

should this have the tag VIRTUAL?

Done.


pkg/sql/opt/xform/testdata/rules/select, line 1735 at r1 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

[nit] an partial -> a partial

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 r3, 3 of 3 files at r4.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 2 stale)

@mgartner
Copy link
Collaborator Author

bors r+

@craig
Copy link
Contributor

craig bot commented Feb 16, 2021

Build succeeded:

@craig craig bot merged commit dc90a0a into cockroachdb:master Feb 16, 2021
@mgartner mgartner deleted the inline-all-virtual-columns branch February 16, 2021 23:45
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.

4 participants