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: fix ALTER INDEX IF EXISTS for non-existing index #42797

Merged
merged 2 commits into from
Nov 27, 2019

Conversation

rafiss
Copy link
Collaborator

@rafiss rafiss commented Nov 26, 2019

Previously, if ALTER INDEX IF EXISTS was used on an
unqualified index name that did not match any index, the database would
return an "index does not exist" error.

Now, the statement succeeds, but does nothing.

Also, test edge cases around ambiguous, non-existing, and/or unqualified
index names, in both alter_index and drop_index logic tests.

Fixes #42399

Release note (bug fix): ALTER INDEX IF EXISTS no longer fails
when using an unqualified index name that does not match any existing
index. Now it is a no-op.

@rafiss rafiss requested review from jordanlewis, apantel and a team November 26, 2019 19:13
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@rafiss rafiss force-pushed the rename-index-exists branch from e079377 to 94b5e79 Compare November 26, 2019 19:23
@rafiss rafiss changed the title sql: fix ALTER INDEX IF NOT EXISTS for non-existing index sql: fix ALTER INDEX IF EXISTS for non-existing index Nov 26, 2019
Copy link
Member

@jordanlewis jordanlewis left a comment

Choose a reason for hiding this comment

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

:lgtm: seems backport worthy!

Reviewed 3 of 3 files at r1.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @apantel)

@@ -36,6 +36,10 @@ type renameIndexNode struct {
func (p *planner) RenameIndex(ctx context.Context, n *tree.RenameIndex) (planNode, error) {
_, tableDesc, err := expandMutableIndexName(ctx, p, n.Index, true /* requireTable */)
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you accomplish the same thing by passing requireTable=false here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I actually ended up making this more similar to drop_index.go, and set requireTable=!n.IfExists

Previously, if ALTER INDEX IF EXISTS (AIIE) was used on an
unqualified index name that did not match any index, the database would
return an "index does not exist" error.

Now, the statement succeeds, but does nothing.

Release note (bug fix): ALTER INDEX IF EXISTS no longer fails
when using an unqualified index name that does not match any existing
index. Now it is a no-op.
Test edge cases around ambiguous, non-existing, and/or unqualified
index names, to reach test parity with alter_index logic test.

Release note: None
@rafiss rafiss force-pushed the rename-index-exists branch from 94b5e79 to fa4cbe5 Compare November 26, 2019 21:25
@rafiss rafiss requested a review from apantel November 26, 2019 21:45
Copy link
Contributor

@apantel apantel 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! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @apantel and @jordanlewis)

@rafiss
Copy link
Collaborator Author

rafiss commented Nov 26, 2019

thanks for the reviews!

bors r+

@rafiss
Copy link
Collaborator Author

rafiss commented Nov 27, 2019

bors r-

@rafiss
Copy link
Collaborator Author

rafiss commented Nov 27, 2019

bors r+

craig bot pushed a commit that referenced this pull request Nov 27, 2019
42797: sql: fix ALTER INDEX IF EXISTS for non-existing index r=rafiss a=rafiss

Previously, if ALTER INDEX IF EXISTS was used on an
unqualified index name that did not match any index, the database would
return an "index does not exist" error.

Now, the statement succeeds, but does nothing.

Also, test edge cases around ambiguous, non-existing, and/or unqualified
index names, in both alter_index and drop_index logic tests.

Fixes #42399 

Release note (bug fix): ALTER INDEX IF EXISTS no longer fails
when using an unqualified index name that does not match any existing
index. Now it is a no-op.

42831: colexec: fix a bug with top K sort not handling K > 1024 r=yuzefovich a=yuzefovich

Previously, top K sorter when emitting the data would copy always from
the beginning of the stored vectors. This is incorrect behavior when
K is greater than coldata.BatchSize(), and now this has been fixed.

Fixes: #42816.

Release note (bug fix): A bug with incorrect handling of Top K sort by
the vectorized engine when K is greater than 1024 has been fixed.

42835: workload, roachtest: skip TPCH Q4 for vec on r=yuzefovich a=yuzefovich

TPCH query 4 in some cases can hit a memory limit error, so we will skip
it for now (until the disk spilling is put in place).

Release note: None

Co-authored-by: Rafi Shamim <[email protected]>
Co-authored-by: Yahor Yuzefovich <[email protected]>
@craig
Copy link
Contributor

craig bot commented Nov 27, 2019

Build succeeded

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.

ALTER INDEX IF EXISTS does not seem to work
4 participants