sql: support alter column SET VISIBLE and NOT VISIBLE to unhide or hide column #63052

kurokochin · 2021-04-03T04:34:28Z

Addresses #62892

cockroach-teamcity · 2021-04-03T04:34:30Z

All committers have signed the CLA.

blathers-crl · 2021-04-03T04:34:31Z

Thank you for contributing to CockroachDB. Please ensure you have followed the guidelines for creating a PR.

Before a member of our team reviews your PR, I have some potential action items for you:

Please ensure your git commit message contains a release note.
When CI has completed, please ensure no errors have appeared.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.}

cockroach-teamcity · 2021-04-03T04:34:37Z

This change is

blathers-crl · 2021-04-03T10:41:25Z

Thank you for updating your pull request.

Before a member of our team reviews your PR, I have some potential action items for you:

We notice you have more than one commit in your PR. We try break logical changes into separate commits, but commits such as "fix typo" or "address review commits" should be squashed into one commit and pushed with --force
Please ensure your git commit message contains a release note.
When CI has completed, please ensure no errors have appeared.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.}

blathers-crl · 2021-04-03T10:43:32Z

Thank you for updating your pull request.

Before a member of our team reviews your PR, I have some potential action items for you:

Please ensure your git commit message contains a release note.
When CI has completed, please ensure no errors have appeared.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.}

otan

thanks! this is looking close, just a couple of places for adding tests.

can you also add the syntax to

cockroach/pkg/sql/parser/parse_test.go

Line 1456 in 6e5dfc4

{`ALTER TABLE a ALTER COLUMN b SET DEFAULT NULL`},

(both VISIBLE and NOT VISIBLE variants?)

otan · 2021-04-05T09:40:12Z

pkg/ccl/importccl/read_import_pgdump.go

@@ -616,6 +616,23 @@ func readPostgresStmt(
 				if !found {
 					return colinfo.NewUndefinedColumnError(cmd.Column.String())
 				}
+			case *tree.AlterTableSetVisible:


can you add a test case for this?

somewhere in

cockroach/pkg/ccl/importccl/import_stmt_test.go

Line 5559 in 6e5dfc4

func TestImportPgDump(t *testing.T) {

!

otan · 2021-04-05T09:40:42Z

pkg/sql/alter_table.go

+	case *tree.AlterTableSetVisible:
+		col, err := tableDesc.FindActiveOrNewColumnByName(col.ColName())
+		if err != nil {
+			return err


can you add a logic test that tries to alter a column that does not exist?

otan · 2021-04-05T09:41:20Z

pkg/sql/logictest/testdata/logic_test/alter_table

+CREATE TABLE visible_table (a int primary key)
+
+statement ok
+ALTER TABLE visible_table ALTER COLUMN a SET VISIBLE


can you add a

query TT SHOW CREATE TABLE ----

after both ALTER statements, and then run make testccllogic FILES='alter_table_locality' TESTFLAGS='-rewrite' to make sure they match expectations?

can you also add tests that SET VISIBLE/NOT VISIBLE on already VISIBLE/NOT VISIBLE columns respectively (i.e. check they are idempotent)

otan · 2021-04-05T09:42:35Z

pkg/sql/sem/tree/alter_table.go

+	ctx.WriteString(" ALTER COLUMN ")
+	ctx.FormatNode(&node.Column)
+	if node.Visible {
+		ctx.WriteString(" SET VISIBLE")


there is an extra space here. may i suggest:

ctx.WriteString(" ALTER COLUMN SET ") ctx.FormatNode(&node.Column) if !node.Visible { ctx.WriteString(" NOT ") } ctx.WriteString("VISIBLE")

otan · 2021-04-05T11:35:12Z

You'll also need to ensure your git comment has a release note, e.g.

Release note (sql change): Add ALTER TABLE ... ALTER COLUMN ...
SET [VISIBLE|NOT VISIBLE], which sets the visibility of the column
on the table. If NOT VISIBLE, these columns will not appears on
`SELECT *` queries.

…de column Release note (sql change): Introduce ALTER TABLE ... ALTER COLUMN SET [VISIBLE|NOT VISIBLE], which marks columns as visible/not visible.

otan

i've fixed them up and merging this!

bors r+

craig · 2021-04-09T00:20:02Z

Build succeeded:

GitHub CI (Cockroach)

Previously we were unable to make CREATE TABLE AS run on a REGIONAL BY ROW TABLE end up with a table that was (nearly) identical to the original REGIONAL BY ROW table. This was due to the fact that we didn't support setting a column to be NOT VISIBLE. With cockroachdb#63052 we added the support for NOT VISIBLE on ALTER COLUMN which makes getting pretty close to a REGIONAL BY ROW TABLE now possible when starting from CREATE TABLE AS. This commit adds a test case to illustrate this fact. Resolves cockroachdb#62326. Release note: None

kurokochin requested review from a team and miretskiy and removed request for a team April 3, 2021 04:34

blathers-crl bot added the O-community Originated from the community label Apr 3, 2021

kurokochin force-pushed the kurokochin/alter-column-set-visible branch from 9c384e5 to 07f719c Compare April 3, 2021 10:43

otan self-requested a review April 5, 2021 09:37

otan suggested changes Apr 5, 2021

View reviewed changes

sql: support alter column SET VISIBLE and NOT VISIBLE to unhide or hi…

0e5bf29

…de column Release note (sql change): Introduce ALTER TABLE ... ALTER COLUMN SET [VISIBLE|NOT VISIBLE], which marks columns as visible/not visible.

otan force-pushed the kurokochin/alter-column-set-visible branch from 07f719c to 0e5bf29 Compare April 8, 2021 22:55

otan approved these changes Apr 8, 2021

View reviewed changes

craig bot merged commit b8fa9b1 into cockroachdb:master Apr 9, 2021

rafiss mentioned this pull request Apr 9, 2021

sql: implement ALTER TABLE ... ALTER COLUMN SET [VISIBLE|NOT VISIBLE] #62892

Closed

rafiss added the backport-21.1.x label Apr 15, 2021

otan mentioned this pull request Apr 19, 2021

release-21.1: sql: support alter column SET VISIBLE and NOT VISIBLE to unhide or hide column #63881

Merged

rafiss added this to the 21.1 milestone Apr 22, 2021

rafiss mentioned this pull request Apr 22, 2021

sql: permit user-defined hidden columns #53428

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sql: support alter column SET VISIBLE and NOT VISIBLE to unhide or hide column #63052

sql: support alter column SET VISIBLE and NOT VISIBLE to unhide or hide column #63052

kurokochin commented Apr 3, 2021

cockroach-teamcity commented Apr 3, 2021 •

edited

Loading

blathers-crl bot commented Apr 3, 2021

cockroach-teamcity commented Apr 3, 2021

blathers-crl bot commented Apr 3, 2021

blathers-crl bot commented Apr 3, 2021

otan left a comment

otan Apr 5, 2021

otan Apr 5, 2021 •

edited

Loading

otan Apr 5, 2021

otan Apr 5, 2021

otan Apr 5, 2021

otan commented Apr 5, 2021

otan left a comment

craig bot commented Apr 9, 2021

sql: support alter column SET VISIBLE and NOT VISIBLE to unhide or hide column #63052

sql: support alter column SET VISIBLE and NOT VISIBLE to unhide or hide column #63052

Conversation

kurokochin commented Apr 3, 2021

cockroach-teamcity commented Apr 3, 2021 • edited Loading

blathers-crl bot commented Apr 3, 2021

cockroach-teamcity commented Apr 3, 2021

blathers-crl bot commented Apr 3, 2021

blathers-crl bot commented Apr 3, 2021

otan left a comment

Choose a reason for hiding this comment

otan Apr 5, 2021

Choose a reason for hiding this comment

otan Apr 5, 2021 • edited Loading

Choose a reason for hiding this comment

otan Apr 5, 2021

Choose a reason for hiding this comment

otan Apr 5, 2021

Choose a reason for hiding this comment

otan Apr 5, 2021

Choose a reason for hiding this comment

otan commented Apr 5, 2021

otan left a comment

Choose a reason for hiding this comment

craig bot commented Apr 9, 2021

cockroach-teamcity commented Apr 3, 2021 •

edited

Loading

otan Apr 5, 2021 •

edited

Loading