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

sql: support ALTER INDEX … VISIBILITY ... #87301

Merged
merged 1 commit into from
Apr 18, 2023

Conversation

wenyihu6
Copy link
Contributor

@wenyihu6 wenyihu6 commented Sep 1, 2022

This commit adds support to ALTER INDEX ... VISIBILITY .... Visibility is
currently not supported with CREATE INDEX or CREATE TABLE. Users would need
to create the index as it is fully not visible or fully visible first. And they
can change the index visibility to any float [0.0, 1.0] using ALTER INDEX ... VISIBILITY ....
Invisibility specifies the invisibility of an index to the
optimizer and can be any float64 between [0.0, 1.0]. An index with invisibility
0.0 means that the index is visible. An index with invisibility 1.0 means that
the index is fully not visible. By default, an index should be visible or
invisibility 0.0. An index with invisibility 1.0 is ignored by the optimizer
unless it is used for constraint check or is explicitly selected with index
hinting. An index with invisibility between (0.0, 1.0) would be made fully not
visible to a corresponding fraction of the queries. By convention, we will
refer any indexes with invisibility == 0.0 as visible, any indexes with
invisibility == 1.0 as fully not visible, and any indexes with index visibility
in-between as partially not visible.

Informs #82363

Release note (sql change): ALTER INDEX ... VISIBILITY ... is now supported.
It can change an index visibility to any visibility within [0.0, 1.0].
Visibility 0.0 means the index is not visible to the optimizer, while
visibility 1.0 means the index is fully visible. A value in the range
(0.0, 1.0) means the index will be visibile to the corresponding fraction of
queries.

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@wenyihu6
Copy link
Contributor Author

wenyihu6 commented Sep 1, 2022

The first commit comes from #87259.

@rytaft rytaft force-pushed the add-alter branch 2 times, most recently from da484d2 to 589012a Compare April 13, 2023 20:11
@rytaft rytaft changed the title sql: support ALTER INDEX … VISIBILITY=... sql: support ALTER INDEX … VISIBILITY ... Apr 13, 2023
@rytaft rytaft force-pushed the add-alter branch 8 times, most recently from 609bf42 to 79a844c Compare April 14, 2023 01:21
@rytaft rytaft marked this pull request as ready for review April 14, 2023 01:25
@rytaft rytaft requested review from a team April 14, 2023 01:25
@rytaft rytaft requested review from a team as code owners April 14, 2023 01:25
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.

I'm reviving this PR too. I've made a few tweaks to @wenyihu6's original commit, but not many! The first commit is #101334.

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

Copy link
Collaborator

@rharding6373 rharding6373 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 @mgartner and @wenyihu6)


pkg/sql/parser/sql.y line 2398 at r14 (raw file):

        return 1
      }
    invisibilityConst := 1.0 - visibilityConst

nit: I still don't understand why we don't store this internally as visibility instead of invisibility. It seems like the 1.0 - [some visibility-related var] calculations are unnecessary. I don't feel super strongly about consistently naming everything as visibility or visible, though, as I'm sure there's a reason why we use invisibility.

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.

TFTR!

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


pkg/sql/parser/sql.y line 2398 at r14 (raw file):

Previously, rharding6373 (Rachael Harding) wrote…

nit: I still don't understand why we don't store this internally as visibility instead of invisibility. It seems like the 1.0 - [some visibility-related var] calculations are unnecessary. I don't feel super strongly about consistently naming everything as visibility or visible, though, as I'm sure there's a reason why we use invisibility.

Same comment as the other PR -- the user-facing feature is described as "visibility" (since that's more intuitive), but in the codebase we use "invisibility" for the most part since we want the default to be that all indexes are visible, and therefore have invisibility=0. I think it's easier to have the default match the go default, but let me know if you think it's too confusing.

Copy link
Collaborator

@rharding6373 rharding6373 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! 1 of 0 LGTMs obtained (waiting on @mgartner)


pkg/sql/parser/sql.y line 2398 at r14 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

Same comment as the other PR -- the user-facing feature is described as "visibility" (since that's more intuitive), but in the codebase we use "invisibility" for the most part since we want the default to be that all indexes are visible, and therefore have invisibility=0. I think it's easier to have the default match the go default, but let me know if you think it's too confusing.

Ok, let's keep as is.

This commit adds support to `ALTER INDEX ... VISIBILITY ...`. `Visibility` is
currently not supported with `CREATE INDEX` or `CREATE TABLE`. Users would need
to create the index as it is fully not visible or fully visible first. And they
can change the index visibility to any float [0.0, 1.0] using `ALTER INDEX ...
VISIBILITY ...` Invisibility specifies the invisibility of an index to the
optimizer and can be any float64 between [0.0, 1.0]. An index with invisibility
0.0 means that the index is visible. An index with invisibility 1.0 means that
the index is fully not visible. By default, an index should be visible or
invisibility 0.0. An index with invisibility 1.0 is ignored by the optimizer
unless it is used for constraint check or is explicitly selected with index
hinting. An index with invisibility between (0.0, 1.0) would be made fully not
visible to a corresponding fraction of the queries. By convention, we will
refer any indexes with invisibility == 0.0 as visible, any indexes with
invisibility == 1.0 as fully not visible, and any indexes with index visibility
in-between as partially not visible.

Informs cockroachdb#82363

Release note (sql change): `ALTER INDEX ... VISIBILITY ...` is now supported.
It can change an index visibility to any visibility within [0.0, 1.0].
Visibility 0.0 means the index is not visible to the optimizer, while
visibility 1.0 means the index is fully visible. A value in the range
(0.0, 1.0) means the index will be visibile to the corresponding fraction of
queries.

Co-authored-by: Rebecca Taft <[email protected]>
@rytaft
Copy link
Collaborator

rytaft commented Apr 18, 2023

bors r+

@craig
Copy link
Contributor

craig bot commented Apr 18, 2023

Build succeeded:

@craig craig bot merged commit 68ca638 into cockroachdb:master Apr 18, 2023
rytaft added a commit to rytaft/cockroach that referenced this pull request Apr 19, 2023
This commit adds support for CREATE INDEX ... VISIBILITY ... as well as
CREATE TABLE ... (... INDEX (...) VISIBILITY ...). This allows users to set
the index visibility to any float [0.0, 1.0] upon creation of the index.

ALTER INDEX ... VISIBILITY ... was previously supported in cockroachdb#87301.

Informs cockroachdb#82363

Release note (sql change): CREATE INDEX ... VISIBILITY ... as well as
CREATE TABLE ... (... INDEX (...) VISIBILITY ...) are now supported. This
allows a user to set the index visibility to any visibility within [0.0, 1.0].
Visibility 0.0 means the index is not visible to the optimizer, while
visibility 1.0 means the index is fully visible. A value in the range
(0.0, 1.0) means the index will be visibile to the corresponding fraction of
queries.
craig bot pushed a commit that referenced this pull request Apr 20, 2023
101812: sql: support setting index visibility in CREATE INDEX and CREATE TABLE r=rytaft a=rytaft

This commit adds support for `CREATE INDEX ... VISIBILITY ...` as well as `CREATE TABLE ... (... INDEX (...) VISIBILITY ...)`. This allows users to set the index visibility to any float [0.0, 1.0] upon creation of the index.

`ALTER INDEX ... VISIBILITY ...` was previously supported in #87301.

Informs #82363

Release note (sql change): `CREATE INDEX ... VISIBILITY ...` as well as `CREATE TABLE ... (... INDEX (...) VISIBILITY ...)` are now supported. This allows a user to set the index visibility to any visibility within [0.0, 1.0]. Visibility 0.0 means the index is not visible to the optimizer, while visibility 1.0 means the index is fully visible. A value in the range (0.0, 1.0) means the index will be visibile to the corresponding fraction of queries.

Co-authored-by: Rebecca Taft <[email protected]>
@wenyihu6 wenyihu6 deleted the add-alter branch October 30, 2023 17:38
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