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

sql: desugar TEXT to STRING #28950

Merged
merged 2 commits into from
Aug 23, 2018
Merged

Conversation

knz
Copy link
Contributor

@knz knz commented Aug 22, 2018

First commits from #28949 and priors.
Forked off from #28690.

The distinction between "TEXT" and "STRING" inside CockroachDB is
inconsequential. Remove it. The data type is already properly reported
in pg_catalog and information_schema as "text".

Release note: None

@knz knz requested a review from BramGruneir August 22, 2018 10:43
@knz knz requested a review from a team as a code owner August 22, 2018 10:43
@knz knz requested review from a team August 22, 2018 10:43
@knz knz requested a review from a team as a code owner August 22, 2018 10:43
@knz knz requested review from a team August 22, 2018 10:43
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@knz knz force-pushed the 20180822-c9-desugar-text branch 3 times, most recently from dee2493 to 73136f9 Compare August 22, 2018 15:41
Copy link
Member

@BramGruneir BramGruneir 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 some questions on leaving in tests for TEXT.

Reviewed 4 of 4 files at r1, 1 of 1 files at r2, 18 of 18 files at r3, 2 of 2 files at r4, 7 of 7 files at r5, 3 of 3 files at r6, 13 of 13 files at r7.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale)


pkg/ccl/importccl/read_import_pgdump_test.go, line 126 at r7 (raw file):

		fmt.Fprintf(&sb, "%s;\n", s)
	}
	const expect = `CREATE TABLE public.second (i INTEGER NOT NULL, s STRING);

Does pgdump ever dump a TEXT column? If so, this should still be tested.


pkg/sql/sem/tree/col_types_test.go, line 57 at r7 (raw file):

		{"VARCHAR", &coltypes.TString{Name: "VARCHAR"}},
		{"CHAR(11)", &coltypes.TString{Name: "CHAR", N: 11}},
		{"TEXT", &coltypes.TString{Name: "TEXT"}},

Shouldn't this now just report STRING instead? Why remove the test and not just fix the expected?


pkg/sql/sem/tree/testdata/pretty/join1.align-deindent.golden, line 1 at r7 (raw file):

1:

We need to mark this file as auto-generated somehow.

@knz knz force-pushed the 20180822-c9-desugar-text branch from 73136f9 to a196c70 Compare August 23, 2018 10:21
@knz knz requested a review from a team August 23, 2018 10:21
Copy link
Contributor Author

@knz knz 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! 1 of 0 LGTMs obtained


pkg/ccl/importccl/read_import_pgdump_test.go, line 126 at r7 (raw file):

Previously, BramGruneir (Bram Gruneir) wrote…

Does pgdump ever dump a TEXT column? If so, this should still be tested.

This is tested, the input SQL uses text as type; we're verifying here that it is desugared to string.


pkg/sql/sem/tree/col_types_test.go, line 57 at r7 (raw file):

Previously, BramGruneir (Bram Gruneir) wrote…

Shouldn't this now just report STRING instead? Why remove the test and not just fix the expected?

No, CHAR is now a separate type from TEXT/STRING (and also separate from VARCHAR, and separate from "char").


pkg/sql/sem/tree/testdata/pretty/join1.align-deindent.golden, line 1 at r7 (raw file):

Previously, BramGruneir (Bram Gruneir) wrote…

We need to mark this file as auto-generated somehow.

Agreed. Done.

@knz knz force-pushed the 20180822-c9-desugar-text branch from a196c70 to b839d82 Compare August 23, 2018 10:31
@knz
Copy link
Contributor Author

knz commented Aug 23, 2018

bors r+

@craig
Copy link
Contributor

craig bot commented Aug 23, 2018

Build failed

knz added 2 commits August 23, 2018 13:06
The distinction between "TEXT" and "STRING" inside CockroachDB is
inconsequential. Remove it. The data type is already properly reported
in pg_catalog and information_schema as "text".

Release note: None
@knz knz force-pushed the 20180822-c9-desugar-text branch from b839d82 to d1d5995 Compare August 23, 2018 11:07
@knz
Copy link
Contributor Author

knz commented Aug 23, 2018

bors just detected a legitimate merge skew! good bors

bors r+

craig bot pushed a commit that referenced this pull request Aug 23, 2018
28950: sql: desugar TEXT to STRING r=knz a=knz

First commits from #28949 and priors.
Forked off from #28690.

The distinction between "TEXT" and "STRING" inside CockroachDB is
inconsequential. Remove it. The data type is already properly reported
in pg_catalog and information_schema as "text".

Release note: None

Co-authored-by: Raphael 'kena' Poss <knz@cockroachlabs.com>
@craig
Copy link
Contributor

craig bot commented Aug 23, 2018

Build succeeded

@craig craig bot merged commit d1d5995 into cockroachdb:master Aug 23, 2018
@knz knz deleted the 20180822-c9-desugar-text branch August 23, 2018 11:55
craig bot pushed a commit that referenced this pull request Aug 23, 2018
29006: backport-2.1: fix the handling of SQL types r=knz a=knz

Backport 1/1 commits from #28937.
Backport 1/1 commits from #28942.
Backport 17/17 commits from #28943.
Backport 1/1 commits from #28944.
Backport 1/1 commits from #28945.
Backport 1/1 commits from #28949.
Backport 2/2 commits from #28950.
Backport 1/1 commits from #28951 (#28959).
Backport 1/1 commits from #28952 (#28959).
Backport 1/1 commits from #28955 (#28959).
Backport 1/1 commits from #28959.
Backport 1/1 commits from #28959.
Backport 1/1 commits from #28690.

cc @cockroachdb/release 

Co-authored-by: Raphael 'kena' Poss <knz@cockroachlabs.com>
craig bot pushed a commit that referenced this pull request Aug 24, 2018
29006: backport-2.1: fix the handling of SQL types r=knz a=knz

Backport 1/1 commits from #28937.
Backport 1/1 commits from #28942.
Backport 17/17 commits from #28943.
Backport 1/1 commits from #28944.
Backport 1/1 commits from #28945.
Backport 1/1 commits from #28949.
Backport 2/2 commits from #28950.
Backport 1/1 commits from #28951 (#28959).
Backport 1/1 commits from #28952 (#28959).
Backport 1/1 commits from #28955 (#28959).
Backport 1/1 commits from #28959.
Backport 1/1 commits from #28959.
Backport 1/1 commits from #28690.

cc @cockroachdb/release 

Co-authored-by: Raphael 'kena' Poss <knz@cockroachlabs.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants