-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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: added Hidden to index descriptor and SHOW INDEX #83388
Conversation
f4240b4
to
b659287
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewing for schema, most of my comments are about the names of things, other than that this looks like a reasonable change, done very thoroughly. Thank you for that.
SELECT * FROM crdb_internal.table_indexes WHERE descriptor_name = '' | ||
---- | ||
descriptor_id descriptor_name index_id index_name index_type is_unique is_inverted is_sharded shard_bucket_count created_at | ||
descriptor_id descriptor_name index_id index_name index_type is_unique is_inverted is_sharded is_invisible shard_bucket_count created_at |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already have a concept of hidden columns, which is somewhat similar to what you're trying to introduce for indexes. I feel it would make sense to re-use the existing terminology for the sake of coherence. This renaming should be straightforward. In this instance, consider renaming is_invisible
to is_hidden
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I posted a few other comments in the same vein, but they aren't exhaustive.
pkg/sql/alter_primary_key.go
Outdated
@@ -183,6 +183,7 @@ func (p *planner) AlterPrimaryKey( | |||
Version: descpb.StrictIndexColumnIDGuaranteesVersion, | |||
ConstraintID: tableDesc.GetNextConstraintID(), | |||
CreatedAtNanos: p.EvalContext().GetTxnTimestamp(time.Microsecond).UnixNano(), | |||
Invisible: false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Naming consistency: consider renaming to Hidden
.
pkg/sql/alter_table.go
Outdated
@@ -280,6 +280,7 @@ func (n *alterTableNode) startExec(params runParams) error { | |||
idx := descpb.IndexDescriptor{ | |||
Name: string(d.Name), | |||
Unique: true, | |||
Invisible: false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Naming consistency: consider renaming the field in the index descriptor proto to hidden
.
pkg/sql/catalog/catformat/index.go
Outdated
@@ -174,6 +174,10 @@ func indexForDisplay( | |||
} | |||
} | |||
|
|||
if index.Invisible { | |||
f.WriteString(" INVISIBLE") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Naming consistency: consider replacing the INVISIBLE
SQL keyword with NOT VISIBLE
.
pkg/sql/catalog/descpb/index.go
Outdated
// IsInvisible returns whether the index is invisible or not. | ||
func (desc *IndexDescriptor) IsInvisible() bool { | ||
return desc.Invisible | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this method necessary? This isn't super obvious but we try to keep the generated protobuf structs lightweight and instead move as much business logic as possible to the catalog.Index
interface and its implementation.
NextColumnID: 2, | ||
NextFamilyID: 1, | ||
NextIndexID: 2, | ||
}}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding this test 👍
Descriptor validation helps us prevent bugs which otherwise are time-consuming to fix, by catching inconsistencies before they are committed to the cluster.
non_unique::BOOL, | ||
seq_in_index, | ||
column_name, | ||
direction, | ||
storing::BOOL, | ||
implicit::BOOL` | ||
implicit::BOOL, | ||
idx.is_invisible::BOOL` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider idx.is_invisible::BOOL AS hidden
instead to keep the column names consistent, not just with the invisible -> hidden
rename I'm advocating, but also for consistency within this table. The other boolean columns don't have an is_
prefix here.
pkg/sql/delegate/show_table.go
Outdated
@@ -111,7 +113,8 @@ SELECT | |||
column_name, | |||
direction, | |||
storing::BOOL, | |||
implicit::BOOL` | |||
implicit::BOOL, | |||
idx.is_invisible::BOOL` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as previous comment: idx.is_invisible::BOOL AS hidden
pkg/sql/opt/cat/index.go
Outdated
@@ -56,6 +56,9 @@ type Index interface { | |||
// IsInverted returns true if this is an inverted index. | |||
IsInverted() bool | |||
|
|||
// IsInvisible returns true if this is an invisible index. | |||
IsInvisible() bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Naming consistency: consider renaming to IsHidden()
.
|
||
// is_invisible specifies whether this index is invisible. Note that primary | ||
// index cannot be invisible. | ||
bool is_invisible = 23; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In addition to suggesting to renaming this for consistency, you'll need to update the logic in scdecomp
to populate this field based on what it finds in the index descriptor. It should be straightforward. I bring it up because I just saw some that some of the scdecomp
tests failed in your latest CI run.
6f1c80d
to
01eb55d
Compare
96b2e62
to
77e6e66
Compare
d51efc7
to
7de4965
Compare
I removed |
Previously, wenyihu6 (Wenyi Hu) wrote…
Hmm I'm not sure why the changes I made is not showing here in Reviewable. But they are under |
@postamar apologies for re-opening this discussion, but I'm worried that using the word "hidden" for parts of the feature when all of the user-facing interactions with the feature use the term "visible" make this unnecessarily confusing. Do you feel that naming consistent with existing "hidden" features is more important than consistency with the "not visible" feature being added? |
@postamar @mgartner @michae2 @knz @vy-ton @otan Tagging people who have commented on the syntax discussion so far. Feel free to ignore me if this is not in your zone : ) It’s been a long time since we last talked about the syntax and naming conventions for this feature. Sorry about this! This paragraph summarizes our discussion about this topic so far. Invisible index feature is introducing three new user facing syntaxes. The second user-facing syntax is related to SQL statements like The third user-facing syntax is with We are also introducing another field in the index descriptor (just for internal use). The options are Another thing to note is that MySQL introduces a new column in |
Naming is hard! 🙂
Yes, I agree with the current usage of
The
I don't feel strongly about this one. I'm fine with
Again, I don't feel strongly about this one. I'm fine with
MySQL uses |
I see now. Sorry about the confusion. The link I got for |
Can we support
I prefer We should help users understand the difference between hidden columns vs indexes, which is why I prefer the different naming for SHOW. For columns, being hidden is a presentation decision. For indexes, being hidden impacts query behavior.
|
Thanks for looking into this! We can choose |
I don't think that's a good idea. There are two different arguments, one that's user-facing and one that's technical.
|
I agree with @knz that adding I agree with the others on the columns for introspection tables. Avoiding "invisible" and favoring column names with the word "visible" has the benefit of matching the syntax. We may consider referring to the feature publicly (i.e. in docs) as "index visibility" rather than "invisible indexes" to avoid inconsistency with the syntax. |
For migrations, it would be better UX if users could focus SQL rewrites on unsupported syntax or different db requirements, e.g. primary key design. Regardless, I understand the rationale for no alias. @otan Can you open an issue for creating a schema migration tool rule to convert syntax. |
Thanks everyone for the comments and sorry for the email spam! Conclusion on invisible index syntax discussion: Michael, Vy, and Marcus's opinions on the second and third user-facing syntax are the same. SQL statements related to For the first user-facing syntax, Vy wants to support I will also make another PR to add a new column in |
Sorry I should have clarified that I'm ok with no support for |
I don't feel too strongly about that. I'm biased towards maintaining towards existing patterns and addressing the naming inconsistencies already present with columns falls way out of scope of this PR. Furthermore, that's neither urgent nor difficult, so I doesn't worry me. |
This PR added a new field `NotVisible` to the struct `IndexDescriptor`. Since primary indexes cannot be not visible, it added another test in `pkg/sql/catalog/tabledesc/validate.go`. Since the invisible index feature has not been introduced yet, all indexes created should be visible. See also: cockroachdb#83471 Assists: cockroachdb#72576 Release note: none
7de4965
to
ad9b92c
Compare
This PR takes the second step for the invisible index feature. The previous PR, cockroachdb#83388, should be merged before this PR is merged. The first commit of this pr comes from the previous pr. This PR added parsing support for CREATE INDEX … NOT VISIBLE and CREATE TABLE (... INDEX() NOT VISIBLE). Note that this PR does not add any logic to the optimizer, and executing it returns an “unimplemented” error immediately. Assists: cockroachdb#72576 See also: cockroachdb#83388 Release note: None
This PR takes the second step for the invisible index feature. The previous PR, cockroachdb#83388, should be merged before this PR is merged. The first commit of this pr comes from the previous pr. This PR added parsing support for CREATE INDEX … NOT VISIBLE and CREATE TABLE (... INDEX() NOT VISIBLE). Note that this PR does not add any logic to the optimizer, and executing it returns an “unimplemented” error immediately. Assists: cockroachdb#72576 See also: cockroachdb#83388 Release note: None
This PR takes the first step for the invisible index feature. It added a boolean
field
Hidden
to the structIndexDescriptor
. Since primary indexes cannot behidden, this pr also added a check in
pkg/sql/catalog/tabledesc/validate.go
for that. In addition, this PR also added a new column
visible
tocrdb_internal.table_indexes
and also to the output of following SQL statements:Since the invisible index feature has not been introduced yet, all indexes
created should be visible. It is expected for all test cases to output
true
for all
visible
columns.See also: next PR for invisible index feature: #83471
Assists: #72576
Note that this issue has not been resolved yet, and this pr only takes the first step.
Release note (sql change): A new column
visible
has been added to the tablecrdb_internal.table_indexes
and to the output of SQL statements related toSHOW INDEX
,SHOW INDEXES
, andSHOW KEYS
. Thevisible
column indicateswhether the index is visible to the optimizer. An invisible index is an index
that is up-to-date but is ignored by the optimizer unless explicitly specified
with index hinting.