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: add support for ALTER TABLE/INDEX SPLIT AT #38585

Merged
merged 2 commits into from
Jul 2, 2019

Conversation

RaduBerinde
Copy link
Member

@RaduBerinde RaduBerinde commented Jul 1, 2019

opt: return index ordinal from ResolveTableIndex

Release note: None

optbuilder: add wrapper for building typed scalar

Release note: None

opt: add support for ALTER TABLE/INDEX SPLIT AT

Adding optimizer support for tree.Split. This statement has a
relational input, so it can't be implemented as an "opaque".

Release note: None

Informs #34848.

@RaduBerinde RaduBerinde requested review from justinj, rytaft and a team July 1, 2019 16:22
@RaduBerinde RaduBerinde requested a review from a team as a code owner July 1, 2019 16:22
@RaduBerinde RaduBerinde requested review from a team July 1, 2019 16:22
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Contributor

@justinj justinj left a comment

Choose a reason for hiding this comment

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

:lgtm:, feels to me like maybe cat.Indexs want to know their own ordinal index, which would let ResolveTableIndex keep its (imo more natural?) signature. I guess if this is the only case where it matters then it's unclear.

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

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.

Column also doesn't have a method returning its Ordinal, instead we have to store it alongside in IndexColumn. I'm not sure if that was because of a high-level principle or it would have just been hard to implement. For optIndex it should be easy, we just need to add the ordinal as a member. CC @andy-kimball - do you have any thoughts on Index having an Ordinal() method?

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

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.

It's problematic to store an ordinal in Column, because it would force the creation of a new wrapper object. Right now, we implement the opt.Column interface directly on sqlbase.ColumnDescriptor. I don't see any issue with having an Index remember its ordinal, since we already have a wrapper in that case.

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

@RaduBerinde
Copy link
Member Author

Ok, dropped the first commit and added an Index.Ordinal instead.

Copy link
Contributor

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

Adding optimizer support for `tree.Split`. This statement has a
relational input, so it can't be implemented as an "opaque".

Note that as part of this change, we move all the "fixed" planNode
column definitions to `sqlbase` and we add an `Ordinal()` method to
`cat.Index`.

Release note: None
@RaduBerinde
Copy link
Member Author

bors r+

craig bot pushed a commit that referenced this pull request Jul 2, 2019
38585: opt: add support for ALTER TABLE/INDEX SPLIT AT r=RaduBerinde a=RaduBerinde

#### opt: return index ordinal from ResolveTableIndex

Release note: None

#### optbuilder: add wrapper for building typed scalar

Release note: None

#### opt: add support for ALTER TABLE/INDEX SPLIT AT

Adding optimizer support for `tree.Split`. This statement has a
relational input, so it can't be implemented as an "opaque".

Release note: None

Informs #34848.

Co-authored-by: Radu Berinde <[email protected]>
@craig
Copy link
Contributor

craig bot commented Jul 2, 2019

Build succeeded

@craig craig bot merged commit 521f24f into cockroachdb:master Jul 2, 2019
@RaduBerinde RaduBerinde deleted the opt-split-at branch July 3, 2019 10:56
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