cli,sql: add another crutch to the handling of column types in dump
#28949
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-c8-unbreak-dump
branch
2 times, most recently
from
494737e
to
0d3bec1
Compare
knz
force-pushed
the
20180822-c8-unbreak-dump
branch
from
0d3bec1
to
e4f172f
Compare
maddyblue
approved these changes
Aug 22, 2018
pkg/cli/dump.go
Outdated
| strings.HasPrefix(md.columnTypes[cols[si]], "TEXT") || | ||
| strings.HasPrefix(md.columnTypes[cols[si]], "CHAR") || | ||
| strings.HasPrefix(md.columnTypes[cols[si]], "VARCHAR") || | ||
| strings.HasPrefix(md.columnTypes[cols[si]], `"char"`) { |
There was a problem hiding this comment.
This needs to be upper and lower cased?
BramGruneir
previously requested 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.
Reviewable status:complete! 0 of 0 LGTMs obtained
pkg/cli/dump.go, line 700 at r6 (raw file):
} // parseArrayElementTypeString returns a column type given a string
Moving this from tree/parse_array to here seems wrong.
It really should interface with the parser. Can that be done easily?
knz
force-pushed
the
20180822-c8-unbreak-dump
branch
from
e4f172f
to
771adc6
Compare
There are major problems in `cockroach dump` described separately in cockroachdb#28948. Part of this is `cockroach dump` trying to reverse-engineering a datum type from the announced column type in `information_schema.columns`, by parsing the type name. Prior to this patch, this parsing was incomplete (it went out of sync with the SQL parser over time, naturally as it was bound to do). This entire approach is flawed, but this is no time to redesign `cockroach dump`. Instead, this patch re-syncs the dump-specific type parser with the general SQL parser. Release note (bug fix): `cockroach dump` is now again better able to operate across multiple CockroachDB versions.
knz
force-pushed
the
20180822-c8-unbreak-dump
branch
from
771adc6
to
2363d44
Compare
knz
commented
Aug 23, 2018
There was a problem hiding this comment.
Reviewable status:
pkg/cli/dump.go, line 642 at r6 (raw file):
Previously, mjibson (Matt Jibson) wrote…
This needs to be upper and lower cased?
Bram just made this question obsolete :)
pkg/cli/dump.go, line 700 at r6 (raw file):
Previously, BramGruneir (Bram Gruneir) wrote…
Moving this from tree/parse_array to here seems wrong.
It really should interface with the parser. Can that be done easily?
You just had to ask! 💖
|
Bram thanks again for being critical and making me think this through. Your review made this PR distinctly better. bors r+ |
craig bot
pushed a commit
that referenced
this pull request
Aug 23, 2018
28949: cli,sql: add another crutch to the handling of column types in `dump` r=knz a=knz All commits but the last part of #28945 and priors. PR forked from #28690. There are major problems in `cockroach dump` described separately in #28948. Part of this is `cockroach dump` trying to reverse-engineer a datum type from the announced column type in `information_schema.columns`, by parsing the type name. Prior to this patch, this parsing was incomplete (it went out of sync with the SQL parser over time, naturally as it was bound to do). This entire approach is flawed, but this is no time to redesign `cockroach dump`. Instead, this patch re-syncs the dump-specific type parser with the general SQL parser. It also ensures this parser is still able to parse pre-2.1 type name aliases. Release note (bug fix): `cockroach dump` is now again better able to operate across multiple CockroachDB versions. Co-authored-by: Raphael 'kena' Poss <knz@cockroachlabs.com>
Build succeeded |
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 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.
All commits but the last part of #28945 and priors.
PR forked from #28690.
There are major problems in
cockroach dumpdescribed separately in#28948. Part of this is
cockroach dumptrying to reverse-engineer a datumtype from the announced column type in
information_schema.columns,by parsing the type name. Prior to this patch, this parsing was
incomplete (it went out of sync with the SQL parser over time,
naturally as it was bound to do).
This entire approach is flawed, but this is no time to redesign
cockroach dump. Instead, this patch re-syncs the dump-specific typeparser with the general SQL parser. It also ensures this parser is
still able to parse pre-2.1 type name aliases.
Release note (bug fix):
cockroach dumpis now again better able tooperate across multiple CockroachDB versions.