re-work get_multi_columns#298
Conversation
|
ping @rafiss |
rafiss
left a comment
rafiss
left a comment
There was a problem hiding this comment.
Thanks for working on this! I had a few questions/comments about this, some due to my lack of familiarity.
Also some potential additional test cases that might be useful to add:
- A few type mapping tests: BYTEA→BLOB, NUMERIC→DECIMAL precision/scale propagation, REAL→FLOAT, SMALLINT→INTEGER, and all 8 ARRAY variants (those mutate fields in place, so worth having some specific testing for that).
- Non-default schema path for
get_multi_columns(schema=...). - Computed column reflection (
Computed("a + b")). The old code emittedcomputed={sqltext, persisted};so we could verify that happens still
| # Check if a sequence is being used and adjust the default value. | ||
| autoincrement = False | ||
| if default is not None: | ||
| nextval_match = re.search(r"""(nextval\(')([^']+)('.*$)""", default) |
There was a problem hiding this comment.
It looks like the old code rewrote bare nextval('seq') to become nextval('"schema".seq') when a schema was provided. The new code drops it entirely, unless I missed somehing.
Was it intentional? If so, a note in the commit message explaining why would be useful. And possibly even a mention in CHANGES.md?
Tools that depend on schema-qualified sequence names might get affected.
There was a problem hiding this comment.
IIUC, cockroachdb/cockroach#170049 should take care of that.
| (None, "spatial_ref_sys"), | ||
| ]: | ||
| table_columns = ( | ||
| connection.execute( |
There was a problem hiding this comment.
This is doing a per-table information_schema.columns lookup inside the loop, on top of the one PGDialect already ran. Could we replace with one bulk SELECT keyed by (schema, table) that we do outside the loop and add to a Python dict?
There was a problem hiding this comment.
I'd like to keep it simple for now. We can re-visit this if user feedback (re: performance) warrants.
| self._test("inet", INET) | ||
|
|
||
|
|
||
| class UnknownTypeTest(fixtures.TestBase): |
There was a problem hiding this comment.
Why do we no longer need this test?
There was a problem hiding this comment.
That test was really to verify _type_map
sqlalchemy-cockroachdb/sqlalchemy_cockroachdb/base.py
Lines 24 to 27 in a2665dc
for completeness. The previous version of get_multi_columns() replaced the method in PGDialect, and it used _type_map to infer more generic column types from specific column types. The new version of get_multi_columns() extends the method in PGDialect and it doesn't use _type_map. Our extended version really just
- handles hidden columns, and
- enforces legacy type mappings like BIGINT → INT, which are necessary to prevent existing users of Alembic from getting hammered with spurious "changes" of column type.
In fact, now it appears that _type_map is no longer used at all, and should be removed too.
There was a problem hiding this comment.
In fact, now it appears that _type_map is no longer used at all, and should be removed too.
Done.
|
Switching this to draft pending resolution of cockroachdb/cockroach#170049 cc: @rafiss |
# Conflicts: # dev-requirements.txt # test-requirements.txt
|
re:
We have tests like that in test/test_introspection.py . In fact, they are what tipped me off to the need tor our own type mapping in the first place.
Done. |
2 participants
Fixes: #297
Fixes: #303