sql/coltypes: remove the field Name in TArray
#28959
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This change is |
knz
force-pushed
the
20180822-c13-simplify-tarray
branch
from
229eb78
to
d98e93a
Compare
BramGruneir
approved these changes
Aug 22, 2018
There was a problem hiding this comment.
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, 31 of 31 files at r8, 23 of 23 files at r9, 16 of 16 files at r10, 2 of 2 files at r11.
Reviewable status:complete! 0 of 0 LGTMs obtained (and 1 stale)
pkg/sql/coltypes/arrays.go, line 43 at r11 (raw file):
} else { node.ParamType.Format(buf, f) buf.WriteString("[]")
Is there a reason why this isn't:
node.ParamType.Format(buf, f)
buf.WriteString("[]")
if collation, ok := node.ParamType.(*TCollatedString); ok {
buf.WriteString(" COLLATE ")
lex.EncodeUnrestrictedSQLIdent(buf, collation.Locale, f)
}
BramGruneir
reviewed
Aug 22, 2018
There was a problem hiding this comment.
But I did have one question.
Reviewable status:
This patch corrects two long-standing bugs in CockroachDB which were confusing both 3rd party tools/ORMs and users: - the SQL standard type called "CHAR" is supposed to have a default maximum width of 1 character. Prior to this patch, CockroachDB did not support this and instead set no default maximum width on CHAR. This would cause CockroachDB to fail validating the width of values inserted in tables, fail to constrain the width of values during casts to CHAR, and report incorrect information in introspection tables. - PostgresSQL's dialect distinguishes the table column types TEXT, CHAR, VARCHAR and a special thing called `"char"` (this is a legacy PostgreSQL type which is, unfortunately, still used by some pg_catalog tables). Prior to this patch, CockroachDB would map all of these types into the same column type "STRING". In turn this caused them to show up as "text" in introspection. While this was not incorrect from the perspective of value handling (all these types are, indeed, handled with about the same value semantics in both PostgreSQL and CockroachDB), ORMs do pay attention to this distinction and become confused if they see a different type in introspection (e.g. "text") than what the application model requires (e.g. "char"). The result of this confusion was to trigger a schema change to adapt the type, which in turn fails due to missing features in ALTER COLUMN TYPE. This patch corrects both problems by giving the appropriate default width to CHAR and preserving the distinction between the various string types in column descriptors and thus introspection. Additionally, a width constraint check on collated string columns was missing. This is now fixed too. Tables and columns created prior to this patch are unaffected. Release note (bug fix): CockroachDB now distinguishes "CHAR" and "VARCHAR" as mandated by the SQL standard and PostgreSQL compatibility. When a width is not specified (e.g. `CHAR(3)`), the maximum width of `VARCHAR` remains unconstrained whereas the maximum width for `CHAR` is now 1 character. Release note (bug fix): CockroachDB now properly checks the width of strings inserted in a collated string column with a specified width. Release note (sql change): CockroachDB now preserves the distinction between different column types for string values like in PostgreSQL, for compatibility with 3rd party tools and ORMs.
The distinction between the type names "JSON" and "JSONB" is inconsequential. The types are already reported properly in introspection using the right alias. This patch removes this distinction in code. Release note: None
Prior to this patch the canonical name for the timestamptz type in CockroachDB was "TIMESTAMP WITH TIME ZONE" with spaces and all glorious 24 characters. This is unfortunate because this is a pretty common type, and is moreover used to disambiguate the return type of `now()` and thus will be emitted pretty often in distsql physical plans. Having such a long type name is unnecessary and indeed adds 50%+ storage, communication and parsing overhead for every occurrence of this type in distsql plans. This patch shortens the canonical CockroachDB name to "TIMESTAMPTZ". The representation of the type in introspection tables (pg_catalog, information_schema) is unaffected. Release note: None
It was not needed. This simplifies. Release note: None
knz
force-pushed
the
20180822-c13-simplify-tarray
branch
from
d98e93a
to
f64a081
Compare
knz
commented
Aug 23, 2018
There was a problem hiding this comment.
Reviewable status:
pkg/sql/coltypes/arrays.go, line 43 at r11 (raw file):
Previously, BramGruneir (Bram Gruneir) wrote…
Is there a reason why this isn't:
node.ParamType.Format(buf, f) buf.WriteString("[]") if collation, ok := node.ParamType.(*TCollatedString); ok { buf.WriteString(" COLLATE ") lex.EncodeUnrestrictedSQLIdent(buf, collation.Locale, f) }
This was my first idea, but unfortunately it is incorrect: if the element type is a collated string, ParamType.Format() would include the COLLATE keyword. However, the keyword must appear after the square brackets.
I did however reuse the ParamType.Format instead of TypeName inside the if case, which is nicer to read.
Also added a comment to clarify.
Also added some tests.
|
TFYR! bors r+ |
|
temporary doc site failure bors r- |
Canceled |
|
bors r+ |
Build succeeded |
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
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
First commits from #28955 and priors.
Forked from #28690.
It was not needed. This simplifies.
Release note: None