sql: various updates for sqlalchemy compatibility #38318
Conversation
|
This change is |
There was a problem hiding this comment.
LGTM with nits. Nice sleuthing to figure out how typmod works.
Reviewed 6 of 6 files at r1, 3 of 3 files at r2, 6 of 6 files at r3, 1 of 1 files at r4, 4 of 4 files at r5.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @jordanlewis)
pkg/sql/pgwire/types.go, line 179 at r5 (raw file):
case *tree.DArray: // Arrays have custom formatting depending on their OID
nit: period at end of sentence.
pkg/sql/sem/builtins/pg_builtins.go, line 882 at r4 (raw file):
), // pg_type returns true if the input oid corresponds to a type
have you checked the comment is still pertaining to this function? asking before the function name doesn't match.
pkg/sql/types/types.go, line 1008 at r2 (raw file):
} case IntervalFamily: // TODO(jordan): intervals can have typmods, but we don't support them yet.
this could be best served with an error return with an unimplemented error.
pkg/sql/types/types.go, line 1049 at r2 (raw file):
return "name" default: panic(errors.AssertionFailedf("unexpected OID: %d", t.Oid()))
ditto panic -> error return
jordanlewis
force-pushed
the
sqlalchemy-updates
branch
from
6a79242 to
f6a8fb2
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/sql/sem/builtins/pg_builtins.go, line 882 at r4 (raw file):
Previously, knz (kena) wrote…
have you checked the comment is still pertaining to this function? asking before the function name doesn't match.
Oops. I did write this comment, but messed up the function name. Done.
pkg/sql/types/types.go, line 1008 at r2 (raw file):
Previously, knz (kena) wrote…
this could be best served with an error return with an unimplemented error.
Actually we want to make sure this always works, because clients will call format_type with interval and not expect an error.
pkg/sql/types/types.go, line 1049 at r2 (raw file):
Previously, knz (kena) wrote…
ditto panic -> error return
I'll leave this larger refactor for another time (lots of callsites to change).
|
bors r+ |
Build failed (retrying...) |
Build failed |
Previously, we output CHECK constraints slightly differently than Postgres, which confused SQLAlchemy. Specifically, Postgres always outputs CHECK constraints with 2 (count 'em) sets of wrapping parentheses. Now, we do that too! Release note (sql change): pg_get_constraintdef now outputs CHECK constraints exactly the same way that Postgres does.
Previously, our implementation of the pg builtin format_type ignored its second argument, the type modifier, which indicates the length, scale, or precision of the datatype. Now, it correctly heeds that argument and produces a formatted type that includes length, scale, or precision, when appropriate. Release note (sql change): the format_type Postgres builtin now properly respects its second argument.
Previously, foreign keys were printed slightly differently in SHOW CREATE TABLE than Postgres prints them, which led to incompatibility with SQLAlchemy. Release note (sql change): SHOW CREATE TABLE now prints foreign keys more like Postgres does.
Release note (sql change): the pg_type_is_visible builtin from Postgres is now implemented.
Previously, casting an int2vector to a string would fail to use the
special int2vector formatting (space-separated elements instead of the
normal {}-wrapped and comma-separated elements format of arrays). This
was causing issues with SQLAlchemy.
This commit solves this problem and simplifies the code by removing the
DOidWrapper usages for int2vector and oidvector, in favor of a field on
DArray that contains a custom oid. This opens the door to teaching
DArray's format method about the special formatting, rather than having
it live only in pgwire, where it fails to be useful in the cast of e.g.
casts.
Release note (sql change): casting an int2vector to a string now
produces a Postgres-compatible result.
jordanlewis
force-pushed
the
sqlalchemy-updates
branch
from
f6a8fb2 to
3f4d1a0
Compare
|
Fixed some test merge skew. bors r+ |
38318: sql: various updates for sqlalchemy compatibility r=jordanlewis a=jordanlewis This PR contains several compatibility fixes for problems discovered by running the SQLAlchemy test suite against CockroachDB. A followup PR will add that test suite to our list of ORM test suites. - make pg_get_constraintdef output identical to Postgres for CHECK constraints - add length, scale, precision output to format_type() when passed a typmod argument - print foreign key constraint strings without an extra space, like Postgres does - add pg_type_is_visible builtin that always returns true if its input is a real type - support int2vector::string casts in the same way Postgres does Co-authored-by: Jordan Lewis <[email protected]>
Build succeeded |
This PR contains several compatibility fixes for problems discovered by running
the SQLAlchemy test suite against CockroachDB. A followup PR will add that test
suite to our list of ORM test suites.