sql: populate NotVisible to index definitions by wenyihu6 · Pull Request #84912 · cockroachdb/cockroach

wenyihu6 · 2022-07-22T16:32:03Z

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/testutils/testcat/testdata/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: #84783

Assists: #72576

Release note: none

cockroach-teamcity · 2022-07-22T16:32:17Z

This change is

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.

84783: sql: add CREATE … NOT VISIBLE to parser r=wenyihu6 a=wenyihu6 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. 84927: opt: permit coexistance of inv, forward histograms r=jordanlewis a=jordanlewis Closes #84529. Previously, when trying to use persisted histograms to filter an inverted column, if the histogram read from disk was a "forward histogram", the operation would fail with an internal error. This is because inverted histograms and forward histograms use different datatypes on disk, and it's not possible to compare a forward histogram (represented as the datatype of the column) with the inverted constraint (represented as DBytes). Now, this problem is corrected by making sure that we only store forward histogram data in forward histogram column sets, and likewise for inverted histogram data being only stored in inverted histogram column sets. Release note: None 85321: dev: a couple `testlogic` improvements r=rail a=rickystewart 1. Make sure `dev generate testlogic` is documented in `dev generate -h`. 2. Only default to running the sqlite logic tests if `--bigtest` is passed. 3. If a `--config` is passed, check whether the directory exists before running the test. This fixes cases like `dev testlogic --config=local-mixed-21.2-22.1` where not all logic test categories support the config. 4. Warn if no test directories are found. Release note: None Co-authored-by: wenyihu3 <wenyi.hu@cockroachlabs.com> Co-authored-by: Jordan Lewis <jordanthelewis@gmail.com> Co-authored-by: Ricky Stewart <ricky@cockroachlabs.com>

michae2

Nice work! Very thorough.

Reviewed 15 of 15 files at r1, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @mgartner, @rhu713, and @wenyihu6)

pkg/internal/sqlsmith/alter.go line 354 at r1 (raw file):

		Inverted:     inverted,
		Concurrently: s.coin(),
		NotVisible:   s.d6() == 1, // NotVisible index is rare 1/6 chance.

Thank you for adding this!

pkg/sql/opt_catalog.go line 864 at r1 (raw file):

	// Build the indexes.
	// TODO(wenyihu6): check if need to populate NotVisible property here

I don't think we need to put a NotVisible property into optIndex. All we need is for optIndex.IsNotVisible() to work, which it should, assuming IsNotVisible can be called on its idx.

pkg/sql/importer/read_import_mysql.go line 499 at r1 (raw file):

		}
		// TODO(wenyihu6): support importing mysql CREATE TABLE statement with not
		// visible index.

Cool idea!

pkg/sql/randgen/schema.go line 205 at r1 (raw file):

		// TODO(wenyihu6): check if we should support not visible index here
		// Creating a not visible unique index in table def here might cause error
		// in parseCreateStatement.

I think it's fine to leave not-visible indexes out of randgen for now.

wenyihu6 · 2022-08-02T13:27:30Z

pkg/sql/opt_catalog.go line 864 at r1 (raw file):

Previously, michae2 (Michael Erickson) wrote…

I don't think we need to put a NotVisible property into optIndex. All we need is for optIndex.IsNotVisible() to work, which it should, assuming IsNotVisible can be called on its idx.

That makes sense. 👍

mgartner

Just some minor nits.

Reviewed 13 of 15 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (and 1 stale) (waiting on @michae2, @rhu713, and @wenyihu6)

pkg/sql/randgen/schema.go line 205 at r1 (raw file):

Previously, michae2 (Michael Erickson) wrote…

I think it's fine to leave not-visible indexes out of randgen for now.

For now we could make not-visible non-unique indexes only.

pkg/sql/opt/xform/testdata/rules/not_visible_index line 10 at r2 (raw file):


exec-ddl
SHOW CREATE t1

I think these tests belong better in pkg/sql/opt/testutils/testcat/testdata/index.

wenyihu6 · 2022-08-02T19:00:29Z

pkg/sql/randgen/schema.go line 205 at r1 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

For now we could make not-visible non-unique indexes only.

Done.

wenyihu6 · 2022-08-02T19:00:46Z

pkg/sql/opt/xform/testdata/rules/not_visible_index line 10 at r2 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

I think these tests belong better in pkg/sql/opt/testutils/testcat/testdata/index.

Done.

mgartner

Great work!

Reviewed 5 of 5 files at r3, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 2 stale) (waiting on @michae2, @rhu713, and @wenyihu6)

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

wenyihu6 · 2022-08-02T21:22:44Z

Some GitHub CI failed due to pq: unimplemented: creating a not visible index is not supported yet because we are not supporting not visible index yet. I had to comment out the part where we randomly generate invisible index under sqlsmith and randgen to fix that. I added a todo to uncomment them after we fully support this feature.

michae2

Makes sense, thank you!

Reviewed 1 of 2 files at r2, 1 of 5 files at r3, 3 of 3 files at r4, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 2 stale) (waiting on @michae2, @rhu713, and @wenyihu6)

wenyihu6 · 2022-08-03T01:45:09Z

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

bors r+

craig · 2022-08-03T02:50:34Z

Build succeeded:

GitHub CI (Cockroach)

wenyihu6 force-pushed the 2.5-populate-data-new branch from 6430488 to e8a1396 Compare July 22, 2022 23:15

wenyihu6 changed the title ~~sql: populates NotVisible to index definitions~~ sql: populate NotVisible to index definitions Jul 24, 2022

wenyihu6 force-pushed the 2.5-populate-data-new branch from e8a1396 to 607e729 Compare July 25, 2022 13:39

wenyihu6 mentioned this pull request Jul 25, 2022

sql: add CREATE … NOT VISIBLE to parser #84783

Merged

wenyihu6 force-pushed the 2.5-populate-data-new branch from 607e729 to 54ed019 Compare July 25, 2022 13:48

wenyihu6 force-pushed the 2.5-populate-data-new branch 3 times, most recently from 904601f to dc4be74 Compare July 26, 2022 17:35

wenyihu6 force-pushed the 2.5-populate-data-new branch 5 times, most recently from 6c75846 to b56520e Compare July 27, 2022 12:47

wenyihu6 marked this pull request as ready for review July 27, 2022 17:01

wenyihu6 requested a review from a team as a code owner July 27, 2022 17:01

wenyihu6 requested review from a team and rhu713 July 27, 2022 17:01

wenyihu6 force-pushed the 2.5-populate-data-new branch from b56520e to 25bad04 Compare July 27, 2022 20:05

wenyihu6 force-pushed the 2.5-populate-data-new branch from 0f6fc6f to d1c361b Compare July 29, 2022 16:49

wenyihu6 force-pushed the 2.5-populate-data-new branch from d1c361b to 11fdc8a Compare July 29, 2022 17:08

wenyihu6 force-pushed the 2.5-populate-data-new branch from 11fdc8a to 7bb42dc Compare July 30, 2022 01:01

This was referenced Jul 30, 2022

sql: add DisableNotVisibleIndex to ScanFlags #85239

Merged

sql: notices for NotVisible Indexes #85354

Merged

wenyihu6 requested review from mgartner and michae2 August 1, 2022 14:06

michae2 approved these changes Aug 2, 2022

View reviewed changes

wenyihu6 force-pushed the 2.5-populate-data-new branch from 7bb42dc to 8591cdd Compare August 2, 2022 13:25

wenyihu6 force-pushed the 2.5-populate-data-new branch from 8591cdd to 9ce15f4 Compare August 2, 2022 13:34

mgartner approved these changes Aug 2, 2022

View reviewed changes

wenyihu6 force-pushed the 2.5-populate-data-new branch from 9ce15f4 to a50aba2 Compare August 2, 2022 16:54

wenyihu6 force-pushed the 2.5-populate-data-new branch from a50aba2 to 2d912be Compare August 2, 2022 20:51

mgartner approved these changes Aug 2, 2022

View reviewed changes

wenyihu6 force-pushed the 2.5-populate-data-new branch from 2d912be to 2524f94 Compare August 2, 2022 21:18

michae2 approved these changes Aug 2, 2022

View reviewed changes

craig bot merged commit 7326bbb into cockroachdb:master Aug 3, 2022

wenyihu6 deleted the 2.5-populate-data-new branch September 1, 2022 20:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sql: populate NotVisible to index definitions#84912

sql: populate NotVisible to index definitions#84912
craig[bot] merged 1 commit intocockroachdb:masterfrom
wenyihu6:2.5-populate-data-new

wenyihu6 commented Jul 22, 2022 •

edited

Loading

Uh oh!

cockroach-teamcity commented Jul 22, 2022

Uh oh!

michae2 left a comment

Uh oh!

wenyihu6 commented Aug 2, 2022

Uh oh!

mgartner left a comment

Uh oh!

wenyihu6 commented Aug 2, 2022

Uh oh!

wenyihu6 commented Aug 2, 2022

Uh oh!

mgartner left a comment

Uh oh!

wenyihu6 commented Aug 2, 2022

Uh oh!

michae2 left a comment

Uh oh!

wenyihu6 commented Aug 3, 2022

Uh oh!

craig bot commented Aug 3, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

wenyihu6 commented Jul 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cockroach-teamcity commented Jul 22, 2022

Uh oh!

michae2 left a comment

Choose a reason for hiding this comment

Uh oh!

wenyihu6 commented Aug 2, 2022

Uh oh!

mgartner left a comment

Choose a reason for hiding this comment

Uh oh!

wenyihu6 commented Aug 2, 2022

Uh oh!

wenyihu6 commented Aug 2, 2022

Uh oh!

mgartner left a comment

Choose a reason for hiding this comment

Uh oh!

wenyihu6 commented Aug 2, 2022

Uh oh!

michae2 left a comment

Choose a reason for hiding this comment

Uh oh!

wenyihu6 commented Aug 3, 2022

Uh oh!

craig bot commented Aug 3, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

wenyihu6 commented Jul 22, 2022 •

edited

Loading