idxtype: add idxtype package #139786
idxtype: add idxtype package #139786
Conversation
andy-kimball
requested review from
jeffswenson
and removed request for
a team
|
This change is |
|
Note that there are 3 commits in this PR: the 1st commit contains mostly Foundations changes and the 2nd commit mostly Queries changes. The 3rd commit adds |
There was a problem hiding this comment.
Reviewed 101 of 101 files at r1, 40 of 40 files at r2, 13 of 13 files at r3, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @andy-kimball and @rafiss)
pkg/sql/sem/idxtype/idxtype.proto line 21 at r1 (raw file):
FORWARD = 0; // INVERTED can contain multiple entries for each row in the table, in order // to index collection or composite data types like JSONB, ARRAY, and
nit: missing a word in "... in order to index collection..."?
pkg/sql/sem/idxtype/idxtype.go line 11 at r3 (raw file):
// contains unique keys sorted according to the primary ordering of the table. // Secondary indexes refer to rows in the primary index by unique key value. func CanBePrimary(t T) bool {
I'm curious if you thought about making these methods of T instead of functions. It'd make calling them slightly more succinct. Maybe there is an benefit to functions I'm not seeing. Feel free to leave as-is—I don't feel strongly.
pkg/sql/opt/optbuilder/groupby.go line 993 at r3 (raw file):
for i := 1; i < tab.IndexCount(); i++ { index := tab.Index(i) if !index.IsUnique() || idxtype.CanBeUnique(index.Type()) {
I think this should be !idxtype.CanBeUnique(index.Type()).
pkg/sql/schemachanger/scbuild/internal/scbuildstmt/create_index.go line 311 at r3 (raw file):
if columnNode.OpClass != "" && (!lastColIdx || !idxtype.SupportsOpClass(n.Type)) { panic(pgerror.New(pgcode.DatatypeMismatch, "operator classes are only allowed for the last column of an inverted index"))
nit: "inverted" should be the index type name like above
pkg/sql/schemachanger/scbuild/internal/scbuildstmt/create_index.go line 316 at r3 (raw file):
if idxtype.AllowExplicitDirection(n.Type) && columnNode.Direction == tree.Descending && lastColIdx { panic(pgerror.New(pgcode.FeatureNotSupported, "the last column in an inverted index cannot have the DESC option"))
nit: "inverted" should be the index type name like above
pkg/sql/randgen/schema.go line 264 at r3 (raw file):
continue } if idxtype.CanBePrimary(indexDef.Type) && pk != nil {
nit: !idxtype.CanBePrimary(indexDef.Type)
andy-kimball
force-pushed
the
idxtype
branch
2 times, most recently
from
5730419 to
982504d
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/sql/opt/optbuilder/groupby.go line 993 at r3 (raw file):
Previously, mgartner (Marcus Gartner) wrote…
I think this should be
!idxtype.CanBeUnique(index.Type()).
You're right. This caused a test failure as well, which shows we have good test coverage here.
pkg/sql/randgen/schema.go line 264 at r3 (raw file):
Previously, mgartner (Marcus Gartner) wrote…
nit:
!idxtype.CanBePrimary(indexDef.Type)
Done.
pkg/sql/schemachanger/scbuild/internal/scbuildstmt/create_index.go line 311 at r3 (raw file):
Previously, mgartner (Marcus Gartner) wrote…
nit: "inverted" should be the index type name like above
This one is trickier, b/c it uses "an", where vector needs to use "a". In a future PR where I enable vector indexes, I'll look more closely at this and other cases. For now, I changed the error messages to use "an inverted or vector index".
pkg/sql/schemachanger/scbuild/internal/scbuildstmt/create_index.go line 316 at r3 (raw file):
Previously, mgartner (Marcus Gartner) wrote…
nit: "inverted" should be the index type name like above
Same here.
pkg/sql/sem/idxtype/idxtype.go line 11 at r3 (raw file):
Previously, mgartner (Marcus Gartner) wrote…
I'm curious if you thought about making these methods of
Tinstead of functions. It'd make calling them slightly more succinct. Maybe there is an benefit to functions I'm not seeing. Feel free to leave as-is—I don't feel strongly.
Yeah, I went back and forth. I worry a bit about setting the precedent of having complex methods on a simple idxtype enum. While these methods are simple right now, I worry that people will add more and more complexity over time, e.g. by adding methods that involve coltypes. I was also thinking about a future where we have a much more elaborate index type abstraction, similar to what PG has. But maybe we never get there, and we should just do what is most ergonomic right now - I went ahead and switched it.
pkg/sql/sem/idxtype/idxtype.proto line 21 at r1 (raw file):
Previously, mgartner (Marcus Gartner) wrote…
nit: missing a word in "... in order to index collection..."?
I reworded this.
|
What do people think about defining the alias |
There was a problem hiding this comment.
I think we should get rid of the alias.
Reviewed 51 of 51 files at r4, 38 of 38 files at r5, 14 of 16 files at r6, 3 of 3 files at r7, all commit messages.
Reviewable status:
pkg/sql/schemachanger/scbuild/internal/scbuildstmt/create_index.go line 311 at r3 (raw file):
Previously, andy-kimball (Andy Kimball) wrote…
This one is trickier, b/c it uses "an", where vector needs to use "a". In a future PR where I enable vector indexes, I'll look more closely at this and other cases. For now, I changed the error messages to use "an inverted or vector index".
SGTM.
pkg/sql/sem/idxtype/idxtype.go line 11 at r3 (raw file):
Previously, andy-kimball (Andy Kimball) wrote…
Yeah, I went back and forth. I worry a bit about setting the precedent of having complex methods on a simple idxtype enum. While these methods are simple right now, I worry that people will add more and more complexity over time, e.g. by adding methods that involve coltypes. I was also thinking about a future where we have a much more elaborate index type abstraction, similar to what PG has. But maybe we never get there, and we should just do what is most ergonomic right now - I went ahead and switched it.
👍
pkg/sql/sem/idxtype/idxtype.proto line 21 at r1 (raw file):
Previously, andy-kimball (Andy Kimball) wrote…
I reworded this.
Ahh I see now that it was grammatically correct the first time, I was confusing "index" as a noun instead of a verb. It's easier to read now though.
There was a problem hiding this comment.
I'm fine with not having the alias. We definitely should be pretty aggressive with the disallowed_imports in the idxtype/BUILD.bazel file to ensure that it remains a lightweight leaf package.
tree already has its own disallowed list. the new package should at least match that, but perhaps be even more aggressive if it can be.
cockroach/pkg/sql/sem/tree/BUILD.bazel
Lines 307 to 320 in fd09f27
Reviewable status:
andy-kimball
requested review from
DarrylWong,
angles-n-daemons,
arjunmahishi,
aa-joshi,
jbowens and
wenyihu6
and removed request for
a team
![blathers[crl]](https://avatars.githubusercontent.com/in/59700?s=40&u=373d9101a12edc3847d2fb6ae78e3adcfb4ca41e&v=4){kind=avatar}
|
Your pull request contains more than 1000 changes. It is strongly encouraged to split big PRs into smaller chunks. It looks like your PR touches SQL parser code but doesn't add or edit parser tests. Please make sure you add or edit parser tests if you edit the parser. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
|
It looks like your PR touches SQL parser code but doesn't add or edit parser tests. Please make sure you add or edit parser tests if you edit the parser. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
|
I added a commit that uses |
andy-kimball
force-pushed
the
idxtype
branch
2 times, most recently
from
a1711fc to
86d982a
Compare
andy-kimball
force-pushed
the
idxtype
branch
2 times, most recently
from
774b98f to
523ee6c
Compare
There was a problem hiding this comment.
Reviewed 1 of 301 files at r14, 3 of 56 files at r15.
Reviewable status:
-- commits line 44 at r18:
nit: Looks like the leaf package check was included in the previous commit, not this one.
andy-kimball
force-pushed
the
idxtype
branch
2 times, most recently
from
5993bd5 to
0ffa011
Compare
Add new sql/sem/idxtype package that defines an index type enum with values like forward, inverted, and vector. Code across SQL needs to check the index type and previously it used multiple different enums and bools. This commit continues the process of unifying these mechanisms (future commits will do even more). Epic: CRDB-42943 Release note: None
Now that we have the new idxtype.T enumeration that specifies the type of index (e.g. forward, inverted, vector), use that in the opt packages rather than IsInverted and IsVector boolean functions. This will make our code more extensible, such that adding new index types is easier, starting with vector indexes. Epic: CRDB-42943 Release note: None
Add helper functions that specify the capabilities of various index types. For example, only FORWARD indexes can be primary or unique, and only INVERTED and VECTOR indexes support prefix columns. Use these new helper functions across the SQL packages. Epic: CRDB-42943 Release note: None
Update the tree package to use idxtype.T directly rather than using an alias for it. Add a check to the idxtype BUILD.bazel package that ensures it stays a leaf package, so that the tree package doesn't take on additional dependencies beyond what idxtype needs. Epic: CRDB-42943 Release note: None
|
bors=rafiss,mgartner |
|
bors r=rafiss,mgartner |
Add new sql/sem/idxtype package that defines an index type enum with values
like forward, inverted, and vector, as well as "capability" functions describing
what the index can or cannot do. Code across SQL needs to check the index
type and previously it used multiple different enums and bools. This commit
continues the process of unifying these mechanisms (future commits will do
even more).
Epic: CRDB-42943
Release note: None