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

ALTER TYPE support #8022

Merged
merged 1 commit into from
Aug 25, 2020
Merged

ALTER TYPE support #8022

merged 1 commit into from
Aug 25, 2020

Conversation

ericharmeling
Copy link
Contributor

Fixes #4753.
Fixes #7852.
Fixes #7633.

@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link

@RichardJCai RichardJCai left a comment

Choose a reason for hiding this comment

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

General comment, some of the pages are called ALTER TYPE but we'll actually have ALTER TYPE operations for user defined types soon - I think these ALTER TYPEs should be renamed to ALTER COLUMN TYPE

@@ -19,6 +19,7 @@ The table below lists the experimental session settings that are available. For

| Variable | Default Value | Description |
|-------------------------------------+---------------+---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
| `enable_experimental_alter_column_type_general` | `'false'` | If set to `'true'`, enables [column type altering](#alter-column-type). |

Choose a reason for hiding this comment

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

I think it would be useful to specify it enables ALTER COLUMN TYPE support for general cases with some limitations or something along those lines.

Also I don't think the hyperlink is working properly here.

@@ -31,7 +35,7 @@ The user must have the `CREATE` [privilege](authorization.html#assign-privileges
|-----------|-------------
| `table_name` | The name of the table with the column whose data type you want to change.
| `column_name` | The name of the column whose data type you want to change.
| `typename` | The new [data type](data-types.html) you want to use.
| `typename` | The new data type you want to use.<br>`ALTER TABLE ... ALTER TYPE` and [`ALTER TABLE ... ALTER COLUMN ... SET DATA TYPE`](alter-column.html) are aliases.

Choose a reason for hiding this comment

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

I think ALTER TABLE ... ALTER TYPE should be replaced with ALTER TABLE ... ALTER ... TYPE since a col name should go between.
Applies to the other pages as well

Also [SET DATA] is optional to provide.

## Considerations
## Limitations

In CockroachDB versions < v20.2, support for altering column types is limited to increasing the precision of the current type of a column. You cannot convert the column type to another data type, or decrease the precision of the column type.

Choose a reason for hiding this comment

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

Is it worth mentioning there is no errors for no-ops as well - changing the column type from the current type to itself.

Copy link
Contributor Author

@ericharmeling ericharmeling left a comment

Choose a reason for hiding this comment

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

TFTR, @RichardJCai !

we'll actually have ALTER TYPE operations for user defined types soon

Since this is the case, I've consolidated the alter-type and alter-column pages, since ALTER TYPE is actually a subcommand of ALTER COLUMN. Please rereview.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @RichardJCai)


v19.2/alter-type.md, line 15 at r1 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

Is it worth mentioning there is no errors for no-ops as well - changing the column type from the current type to itself.

Done.


v19.2/alter-type.md, line 38 at r1 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

I think ALTER TABLE ... ALTER TYPE should be replaced with ALTER TABLE ... ALTER ... TYPE since a col name should go between.
Applies to the other pages as well

Also [SET DATA] is optional to provide.

Done.


v20.2/experimental-features.md, line 22 at r1 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

I think it would be useful to specify it enables ALTER COLUMN TYPE support for general cases with some limitations or something along those lines.

Also I don't think the hyperlink is working properly here.

Done.

Copy link

@RichardJCai RichardJCai left a comment

Choose a reason for hiding this comment

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

Looks mostly good to me, some minor comments.

(1 row)
~~~

### Create a new computed column value

Choose a reason for hiding this comment

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

I would title this as Change a columns type using an expression


You can use `ALTER COLUMN TYPE` if the following conditions are met:

- The on-disk representation of the column remains unchanged. For example, you cannot change the column data type from `STRING` to an `INT`, even if the string is just a number.

Choose a reason for hiding this comment

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

I would just remove the note on even if the string is just a number.

@@ -19,9 +19,7 @@ Subcommand | Description | Can combine with other subcommands?
[`ADD COLUMN`](add-column.html) | Add columns to tables. | Yes
[`ADD CONSTRAINT`](add-constraint.html) | Add constraints to columns. | Yes
[`ALTER COLUMN`](alter-column.html) | Change or drop a column's [`DEFAULT` constraint](default-value.html) or [`NOT NULL` constraint](not-null.html). | Yes

Choose a reason for hiding this comment

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

This is sort of out dated now. The description only covers some cases. Maybe we should actually have the description be more vague and have them click through to the ALTER COLUMN page.

Copy link
Contributor Author

@ericharmeling ericharmeling left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @RichardJCai)


v20.1/alter-column.md, line 48 at r2 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

I would just remove the note on even if the string is just a number.

Done.


v20.2/alter-column.md, line 202 at r2 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

I would title this as Change a columns type using an expression

Done.


v20.2/alter-table.md, line 21 at r2 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

This is sort of out dated now. The description only covers some cases. Maybe we should actually have the description be more vague and have them click through to the ALTER COLUMN page.

Done.

Copy link
Contributor

@lnhsingh lnhsingh left a comment

Choose a reason for hiding this comment

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

LGTM - just a couple of minor edits!

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @ericharmeling, @lnhsingh, and @RichardJCai)


v19.2/alter-column.md, line 11 at r3 (raw file):

- Set, change, or drop a column's [`DEFAULT` constraint](default-value.html)
- Set or drop a column's [`NOT NULL` constraint](not-null.html)
- Increase the precision of the column's [data type](data-types.html).

nit: Remove period (or add periods to the other bullets)


v19.2/alter-column.md, line 57 at r3 (raw file):

- `ALTER TABLE ... ALTER COLUMN SET DATA TYPE`

For an example of `ALTER COLUMN TYPE`, [Increase a column type's precision](#increase-a-column-types-precision).

nit: "For an example of ALTER COLUMN TYPE, see Increase..."


v20.1/alter-column.md, line 11 at r3 (raw file):

- Set, change, or drop a column's [`DEFAULT` constraint](default-value.html)
- Set or drop a column's [`NOT NULL` constraint](not-null.html)
- Increase the precision of the column's [data type](data-types.html).

nit: Remove period (or add to other bullets)


v20.1/alter-column.md, line 57 at r3 (raw file):

- `ALTER TABLE ... ALTER COLUMN SET DATA TYPE`

For an example of `ALTER COLUMN TYPE`, [Increase a column type's precision](#increase-a-column-types-precision).

nit: add "see Increase..."


v20.1/alter-column.md, line 83 at r3 (raw file):

### Set `NOT NULL` constraint

<span class="version-tag">New in v19.2:</span> Setting the  [`NOT NULL` constraint](not-null.html) specifies that the column cannot contain `NULL` values.

Should this flag be here?


v20.2/alter-column.md, line 10 at r3 (raw file):

- Set, change, or drop a column's [`DEFAULT` constraint](default-value.html)
- Set or drop a column's [`NOT NULL` constraint](not-null.html)

Should this also have the new bullet you added on other versions?

- Increase the precision of the column's [data type](data-types.html)

Copy link
Contributor Author

@ericharmeling ericharmeling left a comment

Choose a reason for hiding this comment

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

TFTR @lnhsingh !

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @lnhsingh and @RichardJCai)


v19.2/alter-column.md, line 11 at r3 (raw file):

Previously, lnhsingh (Lauren Hirata Singh) wrote…

nit: Remove period (or add periods to the other bullets)

Done.


v19.2/alter-column.md, line 57 at r3 (raw file):

Previously, lnhsingh (Lauren Hirata Singh) wrote…

nit: "For an example of ALTER COLUMN TYPE, see Increase..."

Done.


v20.1/alter-column.md, line 11 at r3 (raw file):

Previously, lnhsingh (Lauren Hirata Singh) wrote…

nit: Remove period (or add to other bullets)

Done.


v20.1/alter-column.md, line 57 at r3 (raw file):

Previously, lnhsingh (Lauren Hirata Singh) wrote…

nit: add "see Increase..."

Done.


v20.1/alter-column.md, line 83 at r3 (raw file):

Previously, lnhsingh (Lauren Hirata Singh) wrote…

Should this flag be here?

Nooo - thanks! Removed.


v20.2/alter-column.md, line 10 at r3 (raw file):

Previously, lnhsingh (Lauren Hirata Singh) wrote…

Should this also have the new bullet you added on other versions?

- Increase the precision of the column's [data type](data-types.html)

It should not, because the other versions only support increasing the precision of the column data type, which was not clearly documented before (hence the added bullet in v19.2 and v20.1). v20.2 supports changing the column data type (which includes changing its precision).

@ericharmeling ericharmeling merged commit 687a980 into master Aug 25, 2020
@ericharmeling ericharmeling deleted the alter-type branch August 25, 2020 18:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants