sql: add notice for Alter TYPE ADD VALUE IF NOT EXIST command #52534

himanshuchawla009 · 2020-08-08T07:48:55Z

Added a notice message similar to postgres if user tries to add a value in enum which already exists using IF NOT EXIST command.

root@:26257/defaultdb> alter type color ADD VALUE IF NOT EXISTS 'black';
ALTER TYPE

root@:26257/defaultdb> alter type color ADD VALUE IF NOT EXISTS 'black';
NOTICE: enum label "black" already exists, skipping

However, I noticed similar behaviour in other command like ALTER TABLE command. Altering existing table with if not exist
command doesn't shows any notice.

Release note (sql change): Show notice to user when user tries to add value which already exists in enum .

cockroach-teamcity · 2020-08-08T07:48:58Z

All committers have signed the CLA.

blathers-crl · 2020-08-08T07:49:00Z

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.

I have added a few people who may be able to assist in reviewing:

@awoods187 (author of sql: improve HINT when using IF NOT EXISTS and ENUMs #52327)

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

cockroach-teamcity · 2020-08-08T07:49:03Z

This change is

rohany · 2020-08-08T19:22:49Z

pkg/sql/sqlbase/type_desc.go

@@ -268,7 +269,7 @@ func (desc *MutableTypeDescriptor) AddEnumValue(node *tree.AlterTypeAddValue) er
 	}
 	if found {
 		if node.IfNotExists {
-			return nil
+			return pgnotice.Newf("enum label %q already exists, skipping", node.NewVal)


We don't want to return an error here. Instead, we want to use p.SendClientNotice and return nil. Also add a test in pkg/sql/logictest/testdata/logic_test/notice

ohh... thanks for your review @rohany . i have noticed that sqlbase package is getting removed here #52519. Also planner struct is not accessible in sqlbase package. So using p.SendClientNotice directly doesn't seems feasible to me.
I have just started with this project and i am reading architecture docs in details. So your guidelines will be really helpful for me as a beginner to opensource world as i really want to learn and contribute to this project.

i have noticed that sqlbase package is getting removed here #52519.

This isn't a worry -- all of the code will still be here, the package is just getting split up into smaller packages.

Also planner struct is not accessible in sqlbase package. So using p.SendClientNotice directly doesn't seems feasible to me.

There are a couple options here:

return an error here, and then catch it and convert it into a notice in the statement execution path

pass a closure into this function that does the work of sending a notice

In the statement (alter_type.go) just check if the enum value exists or not. Then this function can just error if it finds the value already present in the enum.

Overall, I think that the third is the cleanest

blathers-crl · 2020-08-08T19:36:59Z

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.}

rohany · 2020-08-08T22:03:19Z

pkg/sql/logictest/testdata/logic_test/alter_type

@@ -154,7 +154,7 @@ ALTER TYPE build ADD VALUE 'a' BEFORE 'b'
 statement error pq: \"b\" is not an existing enum label
 ALTER TYPE build ADD VALUE 'a' AFTER 'b'

-statement ok
+statement error pq: enum label \"c\" already exists, skipping


Please look at the test I mentioned. It isn't correct to return an error here! The idea of a notice is that the statement executes successfully, but just prints out a notice to the user.

this is pushed before i saw your reviews, i have submitted the changes as requested by you. I have added logic test in notice file and sent a client notice in alter_type.go file.

rohany · 2020-08-08T22:07:34Z

pkg/sql/sqlbase/type_desc.go

@@ -268,7 +269,7 @@ func (desc *MutableTypeDescriptor) AddEnumValue(node *tree.AlterTypeAddValue) er
 	}
 	if found {
 		if node.IfNotExists {
-			return nil
+			return pgnotice.Newf("enum label %q already exists, skipping", node.NewVal)


i have noticed that sqlbase package is getting removed here #52519.

This isn't a worry -- all of the code will still be here, the package is just getting split up into smaller packages.

Also planner struct is not accessible in sqlbase package. So using p.SendClientNotice directly doesn't seems feasible to me.

There are a couple options here:

return an error here, and then catch it and convert it into a notice in the statement execution path

pass a closure into this function that does the work of sending a notice

In the statement (alter_type.go) just check if the enum value exists or not. Then this function can just error if it finds the value already present in the enum.

Overall, I think that the third is the cleanest

blathers-crl · 2020-08-09T07:47:34Z

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.}

blathers-crl · 2020-08-09T08:49:10Z

Thank you for updating your pull request.

My owl senses detect your PR is good for review. Please keep an eye out for any test failures in CI.

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

rohany

Looking good! Small comments now.

rohany · 2020-08-09T15:57:23Z

pkg/sql/alter_type.go

+	found := false
+	for _, member := range n.desc.EnumMembers {
+		if member.LogicalRepresentation == node.NewVal {
+			found = true


Not sure why it wasn't written this way in the first place, but we don't need a new variable and a break -- just do the notice/error return in here.

my bad, i have updated this now.

rohany · 2020-08-09T15:58:13Z

pkg/sql/sqlbase/type_desc.go

@@ -255,24 +255,6 @@ func (desc *MutableTypeDescriptor) IsNew() bool {

 // AddEnumValue adds an enum member to the type.
 func (desc *MutableTypeDescriptor) AddEnumValue(node *tree.AlterTypeAddValue) error {


Update the comment here now to say that "AddEnumValue assumes that the type is an enum, and that the new value doesn't exist already in the enum"

updated comment, i will make sure to do such descriptive code comments in future contributions.

rohany

Some more nits.

Please update your commit title and message with the following:

The commit title should be prefaced with the packages touched (i.e sql: add notice for ALTER TYPE ADD VALUE ...)
The release note should also be a properly formatted sentence, and should use the correct release note tag -- in this case, it would be sql change, not just sql

rohany · 2020-08-09T20:42:18Z

pkg/sql/alter_type.go

+				return nil
+			}
+			return pgerror.Newf(pgcode.DuplicateObject, "enum label %q already exists", node.NewVal)
+


remove this newline

rohany · 2020-08-09T20:43:11Z

pkg/sql/sqlbase/type_desc.go

@@ -253,26 +253,8 @@ func (desc *MutableTypeDescriptor) IsNew() bool {
 	return desc.ClusterVersion == nil
 }

-// AddEnumValue adds an enum member to the type.
+//AddEnumValue assumes that the type is an enum, and that the new value doesn't exist already in the enum


make sure that your comment lines don't exceed 80 characters, and are full sentences (with a period at the end). You also must add a space after the //

rohany · 2020-08-10T14:19:56Z

pkg/sql/sqlbase/type_desc.go

@@ -253,26 +253,9 @@ func (desc *MutableTypeDescriptor) IsNew() bool {
 	return desc.ClusterVersion == nil
 }

-// AddEnumValue adds an enum member to the type.
+// AddEnumValue assumes that the type is an enum, and that the new value
+// doesn't exist already in the enum.


Last nit -- don't also remove the comment that actually tells what the function does!

Release note (sql change): Show notice to user when user tries to add value which already exists in enum.

rohany · 2020-08-10T18:55:58Z

Thanks for the contribution! Merging.

bors r=rohany

craig · 2020-08-10T19:26:03Z

Build succeeded:

Compile Build (Cockroach)

blathers-crl bot added O-community Originated from the community X-blathers-triaged blathers was able to find an owner labels Aug 8, 2020

blathers-crl bot requested a review from awoods187 August 8, 2020 07:49

rohany requested review from rohany and removed request for awoods187 August 8, 2020 19:21

rohany suggested changes Aug 8, 2020

View reviewed changes

himanshuchawla009 force-pushed the sql-command-notices branch from 5e6d5d0 to e61ff04 Compare August 9, 2020 07:47

himanshuchawla009 force-pushed the sql-command-notices branch from e61ff04 to d8f17e1 Compare August 9, 2020 08:49

rohany suggested changes Aug 9, 2020

View reviewed changes

himanshuchawla009 force-pushed the sql-command-notices branch from d8f17e1 to f7ff5c4 Compare August 9, 2020 17:49

rohany suggested changes Aug 9, 2020

View reviewed changes

himanshuchawla009 changed the title ~~notice/hint added for Alter TYPE ADD VALUE IF NOT EXIST command~~ sql: add notice for Alter TYPE ADD VALUE IF NOT EXIST command Aug 10, 2020

himanshuchawla009 force-pushed the sql-command-notices branch from f7ff5c4 to f033923 Compare August 10, 2020 05:40

rohany reviewed Aug 10, 2020

View reviewed changes

sql: add notice for ALTER TYPE ADD VALUE IF NOT EXIST command

98e7deb

Release note (sql change): Show notice to user when user tries to add value which already exists in enum.

himanshuchawla009 force-pushed the sql-command-notices branch from f033923 to 98e7deb Compare August 10, 2020 14:51

craig bot merged commit 7e4e0bf into cockroachdb:master Aug 10, 2020

lnhsingh mentioned this pull request Aug 26, 2020

sql: add notice for Alter TYPE ADD VALUE IF NOT EXIST command cockroachdb/docs#8157

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sql: add notice for Alter TYPE ADD VALUE IF NOT EXIST command #52534

sql: add notice for Alter TYPE ADD VALUE IF NOT EXIST command #52534

himanshuchawla009 commented Aug 8, 2020 •

edited

cockroach-teamcity commented Aug 8, 2020 •

edited

blathers-crl bot commented Aug 8, 2020

cockroach-teamcity commented Aug 8, 2020

rohany Aug 8, 2020

himanshuchawla009 Aug 8, 2020 •

edited

rohany Aug 8, 2020

blathers-crl bot commented Aug 8, 2020

rohany Aug 8, 2020

himanshuchawla009 Aug 9, 2020 •

edited

rohany Aug 8, 2020

blathers-crl bot commented Aug 9, 2020

blathers-crl bot commented Aug 9, 2020

rohany left a comment

rohany Aug 9, 2020

himanshuchawla009 Aug 9, 2020

rohany Aug 9, 2020

himanshuchawla009 Aug 9, 2020

rohany left a comment

rohany Aug 9, 2020

himanshuchawla009 Aug 10, 2020

rohany Aug 9, 2020

himanshuchawla009 Aug 10, 2020

rohany Aug 10, 2020

himanshuchawla009 Aug 10, 2020

rohany commented Aug 10, 2020

craig bot commented Aug 10, 2020

		@@ -255,24 +255,6 @@ func (desc *MutableTypeDescriptor) IsNew() bool {

		// AddEnumValue adds an enum member to the type.
		func (desc MutableTypeDescriptor) AddEnumValue(node tree.AlterTypeAddValue) error {

sql: add notice for Alter TYPE ADD VALUE IF NOT EXIST command #52534

sql: add notice for Alter TYPE ADD VALUE IF NOT EXIST command #52534

Conversation

himanshuchawla009 commented Aug 8, 2020 • edited

cockroach-teamcity commented Aug 8, 2020 • edited

blathers-crl bot commented Aug 8, 2020

cockroach-teamcity commented Aug 8, 2020

Choose a reason for hiding this comment

himanshuchawla009 Aug 8, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blathers-crl bot commented Aug 8, 2020

Choose a reason for hiding this comment

himanshuchawla009 Aug 9, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blathers-crl bot commented Aug 9, 2020

blathers-crl bot commented Aug 9, 2020

rohany left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rohany left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rohany commented Aug 10, 2020

craig bot commented Aug 10, 2020

himanshuchawla009 commented Aug 8, 2020 •

edited

cockroach-teamcity commented Aug 8, 2020 •

edited

himanshuchawla009 Aug 8, 2020 •

edited

himanshuchawla009 Aug 9, 2020 •

edited