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.
| 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. |
|
@gordthompson @rafiss Any chance to get this merged anytime soon? |
This change depends in part on the fix for cockroachdb/cockroach#170049 which is included in CRDB v26.3 (verified with v26.3.0-alpha.1), so final merge will likely wait until v26.3.0 is officially released. In the meantime, we can use to get the updated version of this dialect. |
3 participants
Fixes: #297
Fixes: #303