sql: add CREATE … NOT VISIBLE to parser#84783

Merged

craig[bot] merged 1 commit intocockroachdb:masterfrom

wenyihu6:2-add-CREATE-INVISIBLE-new

Jul 30, 2022

Contributor

wenyihu6 commented Jul 21, 2022 •

edited

Loading

This commit adds parsing support for CREATE INDEX … NOT VISIBLE
and CREATE TABLE (... INDEX() NOT VISIBLE).

This commit does not add any logic to the optimizer, and executing it returns an
“unimplemented” error immediately.

Assists: #72576

See also: #84912

Release note (sql change): Parser now supports creating an index with the option
to mark them as invisible. But no implementation has done yet, and executing it
returns an “unimplemented” error immediately.

Member

cockroach-teamcity commented Jul 21, 2022

This change is

wenyihu6 mentioned this pull request

sql: added CREATE … NOT VISIBLE to parser #83471

Closed

wenyihu6 marked this pull request as ready for review

July 21, 2022 03:59

wenyihu6 requested a review from a team as a code owner

July 21, 2022 03:59

wenyihu6 requested review from a team

July 21, 2022 03:59

wenyihu6 requested a review from a team as a code owner

July 21, 2022 03:59

wenyihu6 requested a review from a team

July 21, 2022 03:59

wenyihu6 requested a review from a team as a code owner

July 21, 2022 03:59

Contributor Author

wenyihu6 commented Jul 21, 2022 •

edited

Loading

The first commit comes from #84763. The second commit comes from #84776. Update: the first commit has been merged into master, and this branch has been rebased.

wenyihu6 changed the title ~~sql: added CREATE … NOT VISIBLE to parser~~ sql: add CREATE … NOT VISIBLE to parser

knz reviewed

View reviewed changes

pkg/sql/delegate/show_database_indexes.go Outdated

               	storing::BOOL,
-              	implicit::BOOL`
+              	implicit::BOOL,
+                is_visible::BOOL AS visible`

Contributor

knz Jul 21, 2022

nit: use tabs not spaces for consistency

(alternatively, replace the other tabs with spaces)

pkg/sql/delegate/show_table.go

                   AND table_name=%[2]s
               ORDER BY
-, 2, 3, 4, 5, 6, 7, 8;`
+, 2, 4;`

Contributor

knz Jul 21, 2022

was it on purpose you removed the other ordering columns?

Looks to me like this will make the output non-deterministic.

Collaborator

michae2 Jul 21, 2022 •

edited

Loading

I asked Wenyi to make this change (here). Columns 1, 2, 4 are the natural key of the query, and will be unique.

pkg/sql/opt/testutils/testcat/create_table.go Outdated

               	}
+              	if def.Hidden {
+              		panic("unimplemented: creating an invisible index is not supported yet.")

Contributor

knz Jul 21, 2022

nit: panic(unimplemented.New(...))

pkg/sql/sem/tree/create.go Outdated

               	StorageParams    StorageParams
               	Predicate        Expr
               	Concurrently     bool
+              	Hidden           bool

Contributor

knz Jul 21, 2022

Rename this to NotVisible. Your future self will thank you.

pkg/sql/sem/tree/create.go Outdated

               	PartitionByIndex *PartitionByIndex
               	StorageParams    StorageParams
               	Predicate        Expr
+              	Hidden           bool

Contributor

knz Jul 21, 2022

ditto

wenyihu6 force-pushed the 2-add-CREATE-INVISIBLE-new branch 3 times, most recently from 9ad7242 to eb99a47 Compare

July 21, 2022 12:54

Contributor Author

wenyihu6 commented Jul 21, 2022

pkg/sql/delegate/show_database_indexes.go line 40 at r3 (raw file):

Previously, knz (kena) wrote…

nit: use tabs not spaces for consistency

(alternatively, replace the other tabs with spaces)

Done.

Contributor Author

wenyihu6 commented Jul 21, 2022

pkg/sql/delegate/show_table.go line 141 at r3 (raw file):

Previously, knz (kena) wrote…

was it on purpose you removed the other ordering columns?

Looks to me like this will make the output non-deterministic.

I made the change because I thought it would be deterministic if I order them by table_name, index_name, and then seq_in_index as well. Some test cases were failing if they have SELECT c1, c2 FROM [SHOW INDEX FROM ...] without ORDER BY index_name, seq_in_index following them. There could be different values of c2 with same c1, and I was trying to solve it by this.

wenyihu6 force-pushed the 2-add-CREATE-INVISIBLE-new branch from eb99a47 to be9784b Compare

July 21, 2022 14:33

Contributor Author

wenyihu6 commented Jul 21, 2022

pkg/sql/opt/testutils/testcat/create_table.go line 708 at r3 (raw file):

Previously, knz (kena) wrote…

nit: panic(unimplemented.New(...))

Done.

Contributor Author

wenyihu6 commented Jul 21, 2022

pkg/sql/sem/tree/create.go line 235 at r3 (raw file):

Previously, knz (kena) wrote…

Rename this to NotVisible. Your future self will thank you.

Done. I'm sure I will.

Contributor Author

wenyihu6 commented Jul 21, 2022

pkg/sql/sem/tree/create.go line 985 at r3 (raw file):

Previously, knz (kena) wrote…

ditto

Done.

wenyihu6 force-pushed the 2-add-CREATE-INVISIBLE-new branch 2 times, most recently from b803557 to 30e30a8 Compare

July 21, 2022 15:11

wenyihu6 requested review from mgartner and michae2

July 21, 2022 17:19

wenyihu6 force-pushed the 2-add-CREATE-INVISIBLE-new branch 5 times, most recently from d1c0928 to 6c341aa Compare

July 22, 2022 11:07

Contributor Author

wenyihu6 commented Jul 27, 2022

pkg/sql/parser/testdata/create_index line 417 at r12 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

Why does this error? And shouldn't the error be printed here?

This shouldn't error, and that was why the error wasn't printed 😬

wenyihu6 force-pushed the 2-add-CREATE-INVISIBLE-new branch from 83d6f5b to f7bdf28 Compare

July 27, 2022 22:51

Contributor Author

wenyihu6 commented Jul 27, 2022

pkg/sql/parser/testdata/create_table line 2481 at r12 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

nit: There's no such thing as an index constraint so this test case seems unnecessary.

Done.

Code quote:

CREATE TABLE a (b INT8, c STRING, CONSTRAINT d INDEX (b) NOT VISIBLE)

Contributor Author

wenyihu6 commented Jul 27, 2022

pkg/sql/logictest/testdata/logic_test/create_index line 417 at r12 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

nit: you could create a new logic test file specifically for these tests, like not_visible_index, and you could put the CREATE TABLE tests in there too.

Done.

wenyihu6 force-pushed the 2-add-CREATE-INVISIBLE-new branch from f7bdf28 to 566916c Compare

July 28, 2022 17:09

Contributor Author

wenyihu6 commented Jul 29, 2022

bors r+

Contributor Author

wenyihu6 commented Jul 29, 2022

Bazel failure looks irrelevant.

Contributor

craig bot commented Jul 29, 2022

Build failed (retrying...):

GitHub CI (Cockroach)

Member

yuzefovich commented Jul 29, 2022

I think this requires a rebase on top of latest master to regenerate the logic tests given the recent refactor of the logic test framework.

bors r-

Contributor

craig bot commented Jul 29, 2022

Canceled.

wenyihu6 force-pushed the 2-add-CREATE-INVISIBLE-new branch from 566916c to 1d700bc Compare

July 29, 2022 16:48

Contributor Author

wenyihu6 commented Jul 29, 2022

I think this requires a rebase on top of latest master to regenerate the logic tests given the recent refactor of the logic test framework.

bors r-

Thanks for letting me know! I just rebased.

Member

yuzefovich commented Jul 29, 2022

You also need to run ./dev gen bazel for the regeneration to take place (in case you haven't done so already).


          sql: add CREATE … NOT VISIBLE to parser

f07a20d

This commit adds parsing support for `CREATE INDEX … NOT VISIBLE`
and `CREATE TABLE (... INDEX() NOT VISIBLE)`.

This commit does not add any logic to the optimizer, and executing it returns an
“unimplemented” error immediately.

Assists: cockroachdb#72576

See also: cockroachdb#84912

Release note (sql change): Parser now supports creating an index with the option
to mark them as invisible. But no implementation has done yet, and executing it
returns an “unimplemented” error immediately.

wenyihu6 force-pushed the 2-add-CREATE-INVISIBLE-new branch from 1d700bc to f07a20d Compare

July 29, 2022 17:06

Contributor Author

wenyihu6 commented Jul 29, 2022 •

edited

Loading

Bazel failure (TestRules/combo, computed, cycle) is a known issue (irrelevant to this PR).

bors r+

Contributor

craig bot commented Jul 29, 2022

Build failed (retrying...):

GitHub CI (Cockroach)

Contributor

craig bot commented Jul 30, 2022

Build succeeded:

GitHub CI (Cockroach)

craig bot merged commit 25d5253 into cockroachdb:master

wenyihu6 added a commit to wenyihu6/cockroach that referenced this pull request


          sql: populate NotVisible to index definitions

7bb42dc

This commit populates the field `NotVisible` for `IndexDescriptor`,
`Tree.CreateIndex`, and `Tree.TableIndexDef` at different places. In order to
add some test cases for this commit, I included a test case that checks the
output of `SHOW CREATE` under
`pkg/sql/opt/xform/testdata/rules/not_visible_index` to make sure that
the field has been populated correctly. Creating invisible indexes under
`pkg/sql/opt/testutils/testcat` no longer returns an unimplemented error. But
executing `CREATE INDEX ... NOT VISIBLE` in logictests or in a cluster still
returns an unimplemented error.

See also: cockroachdb#84783

Assists: cockroachdb#72576

Release note: none

cockroach-teamcity mentioned this pull request

sql: add CREATE … NOT VISIBLE to parser cockroachdb/docs#14678

Closed

wenyihu6 mentioned this pull request

sql: add ALTER INDEX … NOT VISIBLE to parser #85473

Merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet