Update the type comparison code used for schema autogeneration. Compare #619

pbecotte · 2019-11-08T01:19:24Z

the output text for the type to look for changes. In addition, allow
schemas to define sets of types that are functionally equivalent, such
as BOOL and TINYINT(1).
#605

sqla-tester

OK, this is sqla-tester setting up my work to try to get revision e4aeb65 of this pull request into gerrit so we can run tests and reviews and stuff

sqla-tester · 2019-11-08T14:18:16Z

New Gerrit review created for change e4aeb65: https://gerrit.sqlalchemy.org/#/c/sqlalchemy/alembic/+/1561

zzzeek · 2019-11-08T14:19:00Z

the jenkins runner will run some openstack suites which in the case of neutron might exercise the type comparison stuff a bit. let's see what happens.

sqla-tester · 2019-11-08T15:22:45Z

mike bayer (zzzeek) wrote:

Let me start off with, thank you very much for this work so far, it looks amazing! This is a really critical area where we get a lot of complaints and I am really excited if we can finally make it work for real.

so there's a bunch of failures but they look to be all in the realm of unexpected positives for the comparison logic.

in the main suite, we have for example, an unexpected change in type between VARCHAR and String (this is likely because on Oracle, it's rendered as VARCHAR2 ? )

Traceback (most recent call last):
File "/home/jenkins/workspace/alembic_gerrit/1d95c17e/tests/test_autogen_comments.py", line 101, in test_alter_table_comment
eq_(diffs[0][0], "add_table_comment")
File "/home/jenkins/workspace/alembic_gerrit/1d95c17e/.tox/py37-sqlamaster-sqlite-postgresql-mysql-oracle/lib/python3.7/site-packages/sqlalchemy/testing/assertions.py", line 237, in eq_
assert a == b, msg or "%r != %r" % (a, b)
AssertionError: ('modify_type', None, 'some_table', 'test', {'existing_nullable': None, 'existing_server_default': False, 'existing_comment': None}, VARCHAR(length=10), String(length=10)) != 'add_table_comment'

over in openstack nova, where they run tests that compare schemas to be the same on MySQL, where the VARCHAR datatype has a lot of flags, we see:

AssertionError: Models and migration scripts aren't in sync:
[ [ ( 'modify_type',
None,
'instance_type_extra_specs',
'key',
{ 'existing_comment': None,
'existing_nullable': True,
'existing_server_default': False},
VARCHAR(collation='utf8_bin', length=255),
String(length=255))],
[ ( 'modify_type',
None,
'instance_type_extra_specs',
'value',
{ 'existing_comment': None,
'existing_nullable': True,
'existing_server_default': False},
VARCHAR(collation='utf8_bin', length=255),
String(length=255))],
[ ( 'modify_type',
None,
'resource_providers',
'name',
{ 'existing_comment': None,
'existing_nullable': True,
'existing_server_default': False},
VARCHAR(charset='utf8', length=200),
Unicode(length=200))]]

those are all false positives; Unicode / String render VARCHAR on MySQL and there are default values for collation and charset. I think a good rule here might be if the DB-reflected column has some flags that are set, and the model-side has no flag set for that value at all, that means no change.

so this opens up a new subject area which is that when we add new comparison features to autogenerate, especially when we added indexes/unique constraints, the vast majority of issues reported were related to false positives. The index/unique thing, I must have had 20 issues over the course of several years with that. So, we might want to build this so that it's being very conservative about reporting a change based on keyword arguments that are different.

as always, let me know how you are doing and if you have motivation / resources to keep going. I can always pick up sooner rather than later if need be but I am glad if you'd like to keep going!

pbecotte · 2019-11-09T00:48:06Z

That is a WAY better idea. Felt pretty ugly adding in all those default keywords into the synonyms list, and I knew I would miss a bunch. But only comparing keywords if they BOTH have them is exactly the right thing to do!

I can add {"string", "varchar2"} to the oracle impl... but I don't know how to test against oracle. they don't seem to have an easily used docker container? I haven't touched oracle since about 2012 :)

I pushed up an update. I have been testing with postgresql/mysql/sqlite. I think this version of the code is a little easier to follow as well... but still feels too complicated?

pbecotte · 2019-11-09T00:49:20Z

Oh, question- I made the assumption that ( is always the split between the type and any keywords- does that hold true across all the databases/types?

zzzeek · 2019-11-09T19:54:20Z

Oh, question- I made the assumption that ( is always the split between the type and any keywords- does that hold true across all the databases/types?

as far as I know, sure. I would think that all the logic here would be overridable by a subclass impl if they needed to get in there and change things.

sqla-tester

OK, this is sqla-tester setting up my work to try to get revision 94e7ad0 of this pull request into gerrit so we can run tests and reviews and stuff

sqla-tester · 2019-11-11T18:27:42Z

Patchset 94e7ad0 added to existing Gerrit review https://gerrit.sqlalchemy.org/#/c/sqlalchemy/alembic/+/1561

pbecotte · 2019-11-11T22:22:58Z

Added the VARCHAR/VARCHAR2 synonym to oracle

sqla-tester

OK, this is sqla-tester setting up my work to try to get revision 8963755 of this pull request into gerrit so we can run tests and reviews and stuff

sqla-tester · 2019-11-15T15:30:11Z

Patchset 8963755 added to existing Gerrit review https://gerrit.sqlalchemy.org/#/c/sqlalchemy/alembic/+/1561

sqla-tester

OK, this is sqla-tester setting up my work to try to get revision 8963755 of this pull request into gerrit so we can run tests and reviews and stuff

sqla-tester · 2019-11-15T15:30:22Z

Patchset 8963755 added to existing Gerrit review https://gerrit.sqlalchemy.org/#/c/sqlalchemy/alembic/+/1561

sqla-tester

OK, this is sqla-tester setting up my work to try to get revision 17a39d6 of this pull request into gerrit so we can run tests and reviews and stuff

sqla-tester · 2019-11-18T13:02:29Z

Patchset 17a39d6 added to existing Gerrit review https://gerrit.sqlalchemy.org/#/c/sqlalchemy/alembic/+/1561

sqla-tester

OK, this is sqla-tester setting up my work to try to get revision 2edf464 of this pull request into gerrit so we can run tests and reviews and stuff

sqla-tester · 2019-11-22T23:28:40Z

Patchset 2edf464 added to existing Gerrit review https://gerrit.sqlalchemy.org/#/c/sqlalchemy/alembic/+/1561

sqla-tester

OK, this is sqla-tester setting up my work to try to get revision 2c9faf6 of this pull request into gerrit so we can run tests and reviews and stuff

sqla-tester · 2019-12-27T23:21:52Z

Patchset 2c9faf6 added to existing Gerrit review https://gerrit.sqlalchemy.org/#/c/sqlalchemy/alembic/+/1561

sqla-tester

OK, this is sqla-tester setting up my work to try to get revision b80f225 of this pull request into gerrit so we can run tests and reviews and stuff

sqla-tester · 2019-12-27T23:55:00Z

Patchset b80f225 added to existing Gerrit review https://gerrit.sqlalchemy.org/#/c/sqlalchemy/alembic/+/1561

sqla-tester

OK, this is sqla-tester setting up my work to try to get revision 97da348 of this pull request into gerrit so we can run tests and reviews and stuff

sqla-tester · 2019-12-28T02:13:55Z

Patchset 97da348 added to existing Gerrit review https://gerrit.sqlalchemy.org/#/c/sqlalchemy/alembic/+/1561

pbecotte · 2019-12-28T02:23:58Z

alembic/testing/plugin/pytestplugin.py

@@ -50,6 +52,30 @@ def pytest_pycollect_makeitem(collector, name, obj):
        return []


+def vendored_exclusions(compound_object):


As you mentioned, the testing construct isn't in sqlalchemy before 1.3. So I did this janky thing- though I think it may make more sense to just drop the support for older versions as you mentioned.

sqla-tester

mike bayer (zzzeek) wrote:

(4 comments)

haven't reviewed the doc changes yet, just a few quickies...

sqla-tester · 2020-01-18T15:49:57Z

alembic/ddl/impl.py

@@ -39,6 +44,7 @@ class DefaultImpl(with_metaclass(ImplMeta)):

    transactional_ddl = False
    command_terminator = ";"
+    type_synonyms = [{"NUMERIC", "DECIMAL"}]


mike bayer (zzzeek) wrote:

I tend to use tuples for things like this to make sure nothing is mutated...

sqla-tester · 2020-01-18T15:49:58Z

alembic/testing/plugin/pytestplugin.py

@@ -50,6 +52,30 @@ def pytest_pycollect_makeitem(collector, name, obj):
        return []


+def vendored_exclusions(compound_object):


mike bayer (zzzeek) wrote:

I've re-vendored this into alembic again in alembic/testing/exclusions.py so we can remove this

sqla-tester · 2020-01-18T15:49:58Z

tests/requirements.py

@@ -5,6 +5,10 @@


 class DefaultRequirements(SuiteRequirements):
+    @property


mike bayer (zzzeek) wrote:

the def name should be stated in terms of whatever it is that oracle is not doing, not the DB name itself

sqla-tester · 2020-01-18T15:49:58Z

tests/test_autogen_diffs.py

 from alembic.util import CommandError
-from ._autogen_fixtures import AutogenFixtureTest
-from ._autogen_fixtures import AutogenTest
+from ._autogen_fixtures import _default_object_filters, AutogenFixtureTest, \


mike bayer (zzzeek) wrote:

this is all going to reformat w/ black...if you install the .pre-commit-config.yml hook for your next commit

is the project switching to black? I had it in black format and undid it in some places to match the older style haha. I approve! will do

sqla-tester · 2020-01-19T16:25:43Z

mike bayer (zzzeek) wrote:

(1 comment)

pbecotte@github wrote:

alembic has been on black for many months now, actually the lines here would reformat because of the "zimports" tool I wrote first and foremost. Just turn on the pre-commit hooks and it will all be taken care of :).

sqla-tester

mike bayer (zzzeek) wrote:

Code-Review-1

(9 comments)

OK I have a full review on here now.

sqla-tester · 2020-01-21T17:59:27Z

alembic/ddl/impl.py

-            )
-            if comparison is not None:
-                return not comparison
+        for meta, inspect in zip(inspected_params.args, meta_params.args):


mike bayer (zzzeek) wrote:

do we know that these two .args are the same length? What if a VARCHAR has a default length that gets set up on the database side but isn't in the metadata? i think perhaps if len(meta_params.args) == 0, we skip this test.

The reason I picked zip was in case was in case the lists weren't of equal length. We only compare up to the length of the shorter list, ignoring differences in "extra" arguments (that may have been added by the server defaults). If the shorter list is len 0, this loop will just be skipped.

do we have any scenarios that exercise these cases? I';m not sure our datatypes have so much positional arguments for this to matter. I'm fine with it for now.

If I do

if inspected_params.args and meta_params.args and inspected_params.args != meta_params.args: return False

AutogenerateVariantCompareTest_oracle+cx_oracle_11_2_0_2_0::test_variant_no_issue fails comparing BigInteger to BigInteger, while test_autogen_diffs.py::CompareMetadataToInspectorTest_oracle+cx_oracle_11_2_0_2_0::test_introspected_columns_match_metadata_columns[cola10] fails comparing Boolean to Boolean.
This is because the Oracle dialect prints those as NUMERIC(19) ... but the server adds precision to the returned value.

I added an extra test to be a bit more explicit with DECIMAL(10, 2) compared to NUMERIC(10)

sqla-tester · 2020-01-21T17:59:27Z

docs/build/autogenerate.rst

+  specified (such as lengths, precisions, or enumeration members), they will be
+  compared as well.  However, if keywords are only specified for one,
+  changes in these will be ignored. The type comparison logic is extensible to
+  work around these limitations, see :ref:`compare_types` for details.


mike bayer (zzzeek) wrote:

add:

.. versionchanged:: 1.4 type comparison code has been enhanced to compare column types more deeply as well as to take arguments into account.

sqla-tester · 2020-01-21T17:59:27Z

docs/build/autogenerate.rst

@@ -405,18 +407,24 @@ is set to True::
 .. note::

   The default type comparison logic (which is end-user extensible) currently


mike bayer (zzzeek) wrote:

"as of Alembic version 1.4" just to make sure people see this

sqla-tester · 2020-01-21T17:59:28Z

docs/build/autogenerate.rst

+     or ``TEXT``. Dialect implementations can have synonyms that are considered
+     equivalent- this is because some databases support types by converting them
+     to another type. For example, in ``NUMERIC`` and ``DECIMAL`` are considered
+     equivalent.


mike bayer (zzzeek) wrote:

"for example, NUMERIC and DECIMAL are considered equivalent on all backends, while on the Oracle backend the additional synonyms BIGINT, INTEGER, NUMBER, SMALLINT are added to this list of equivalents"

sqla-tester · 2020-01-21T17:59:28Z

docs/build/autogenerate.rst

+   * Next, the arguments within the type, such as the lengths of
+     strings, precision values for numerics, the elements inside of an
+     enumeration are compared. If BOTH columns have arguments AND they are
+     different, a change will be detected. If one column is jsut set to the


mike bayer (zzzeek) wrote:

typo "just"

sqla-tester · 2020-01-21T17:59:28Z

docs/build/autogenerate.rst

+     strings, precision values for numerics, the elements inside of an
+     enumeration are compared. If BOTH columns have arguments AND they are
+     different, a change will be detected. If one column is jsut set to the
+     default and the other has arguments, we don't currently detect this. The


mike bayer (zzzeek) wrote:

"and the other has arguments, Alembic will pass on attempting to compare these. The rationale is that it is difficult to detect what a database backend sets as a default value without generating false positives".

sqla-tester · 2020-01-21T17:59:28Z

docs/build/autogenerate.rst

+     reason here is that it can be hard to know what the database backend sets
+     as the default values, meaning that INTEGER() and INTEGER(10) could
+     actually be the same thing, and we don't want to regenerate a diff every
+     time.



mike bayer (zzzeek) wrote:

add a .. versionchanged:: 1.4 note here also

sqla-tester · 2020-01-21T17:59:28Z

docs/build/autogenerate.rst

@@ -488,6 +496,13 @@ then a basic check for type equivalence is run.
 .. versionadded:: 0.7.6 - added support for the ``compare_against_backend()``
   method.

+For a custom dialect, you could also specify ``impl.type_synonyms``. This


mike bayer (zzzeek) wrote:

this is getting into people making their own "impls" which I think is out of scope for this document, so I would take this blurb out.

sqla-tester · 2020-01-21T17:59:28Z

tests/test_autogen_diffs.py

+            Integer(),
+            Numeric(8, 0),
+            True,
+            config.requirements.integer_subtype_comparisons,


mike bayer (zzzeek) wrote:

this requirement apparently refers to just "is oracle", where we couldn't compare BigInteger to Integer. however, now that we compare the "scale" value, this should work now?

Unfortunately not with the other rules. Integer and Numeric come through as synonyms on Oracle, and the "metadata" column has no arguments...so we ignore the arguments on the "inspect" column.

…re the output text for the type to look for changes. In addition, allow schemas to define sets of types that are functionally equivalent, such as BOOL and TINYINT(1).

pbecotte · 2020-01-24T00:35:19Z

mike bayer (zzzeek) wrote:

(1 comment)
pbecotte@github wrote:

alembic has been on black for many months now, actually the lines here would reformat because of the "zimports" tool I wrote first and foremost. Just turn on the pre-commit hooks and it will all be taken care of 👍

Ah, I see- only new lines are formatted, didn't realize!

sqla-tester

OK, this is sqla-tester setting up my work to try to get revision 1a6a386 of this pull request into gerrit so we can run tests and reviews and stuff

sqla-tester · 2020-01-24T00:36:34Z

Patchset 1a6a386 added to existing Gerrit review https://gerrit.sqlalchemy.org/#/c/sqlalchemy/alembic/+/1561

sqla-tester · 2020-01-31T01:41:58Z

mike bayer (zzzeek) wrote:

next thing this needs is a file docs/build/unreleased/619.rst that describes the change.

View this in Gerrit at https://gerrit.sqlalchemy.org/1561

sqla-tester · 2020-02-04T19:21:19Z

mike bayer (zzzeek) wrote:

Code-Review+2 Workflow+1

View this in Gerrit at https://gerrit.sqlalchemy.org/1561

sqla-tester · 2020-02-04T19:21:20Z

Gerrit review https://gerrit.sqlalchemy.org/1561 has been merged. Congratulations! :)

CaselIT · 2023-08-11T20:43:56Z

This PR has completely removed support for compare_against_backend, but it's still documented and supposedly tested (the test is clearly broken.)

@zzzeek For me we can just drop it since no one noticed

CaselIT · 2023-08-11T20:46:44Z

for reference it was originally added in dabc7f0

zzzeek · 2023-08-12T12:08:53Z

how come tests did not fail?

CaselIT · 2023-08-12T12:25:47Z

I haven't checked it. I'll open issue and fix this, also rewriting the tests

zzzeek requested a review from sqla-tester November 8, 2019 14:18

sqla-tester reviewed Nov 8, 2019

View reviewed changes

pbecotte force-pushed the ALEMBIC-605 branch from e4aeb65 to 0a3914a Compare November 9, 2019 00:44

pbecotte force-pushed the ALEMBIC-605 branch 3 times, most recently from a453c9e to 94e7ad0 Compare November 11, 2019 12:31

zzzeek requested a review from sqla-tester November 11, 2019 18:27

sqla-tester reviewed Nov 11, 2019

View reviewed changes

zzzeek requested a review from sqla-tester November 15, 2019 15:30

sqla-tester reviewed Nov 15, 2019

View reviewed changes

zzzeek requested a review from sqla-tester November 15, 2019 15:30

sqla-tester reviewed Nov 15, 2019

View reviewed changes

pbecotte requested a review from sqla-tester November 18, 2019 13:02

sqla-tester reviewed Nov 18, 2019

View reviewed changes

pbecotte force-pushed the ALEMBIC-605 branch from 2d67713 to 2edf464 Compare November 22, 2019 23:28

pbecotte requested a review from sqla-tester November 22, 2019 23:28

sqla-tester reviewed Nov 22, 2019

View reviewed changes

pbecotte force-pushed the ALEMBIC-605 branch from 2edf464 to 07495dd Compare November 23, 2019 00:35

pbecotte requested a review from sqla-tester December 27, 2019 23:21

sqla-tester reviewed Dec 27, 2019

View reviewed changes

pbecotte force-pushed the ALEMBIC-605 branch from 2c9faf6 to b80f225 Compare December 27, 2019 23:54

pbecotte requested a review from sqla-tester December 27, 2019 23:54

sqla-tester reviewed Dec 27, 2019

View reviewed changes

pbecotte force-pushed the ALEMBIC-605 branch from b80f225 to 97da348 Compare December 28, 2019 02:13

pbecotte requested a review from sqla-tester December 28, 2019 02:13

sqla-tester reviewed Dec 28, 2019

View reviewed changes

pbecotte commented Dec 28, 2019

View reviewed changes

sqla-tester reviewed Jan 18, 2020

View reviewed changes

sqla-tester reviewed Jan 21, 2020

View reviewed changes

Update the type comparison code used for schema autogeneration. Compa…

1a6a386

…re the output text for the type to look for changes. In addition, allow schemas to define sets of types that are functionally equivalent, such as BOOL and TINYINT(1).

pbecotte force-pushed the ALEMBIC-605 branch from 97da348 to 1a6a386 Compare January 24, 2020 00:34

pbecotte requested a review from sqla-tester January 24, 2020 00:36

sqla-tester reviewed Jan 24, 2020

View reviewed changes

sqlalchemy-bot closed this in 3ddf82e Feb 4, 2020

CaselIT mentioned this pull request Aug 12, 2023

PR #619 completely removed support for compare_against_backend, but it's still documented #1293

Closed

		@@ -50,6 +52,30 @@ def pytest_pycollect_makeitem(collector, name, obj):
		return []


		def vendored_exclusions(compound_object):

		@@ -5,6 +5,10 @@


		class DefaultRequirements(SuiteRequirements):
		@property

		@@ -405,18 +407,24 @@ is set to True::
		.. note::

		The default type comparison logic (which is end-user extensible) currently

Update the type comparison code used for schema autogeneration. Compare #619

Update the type comparison code used for schema autogeneration. Compare #619

Conversation

pbecotte commented Nov 8, 2019 • edited Loading

sqla-tester left a comment

Choose a reason for hiding this comment

sqla-tester commented Nov 8, 2019

zzzeek commented Nov 8, 2019

sqla-tester commented Nov 8, 2019

pbecotte commented Nov 9, 2019

pbecotte commented Nov 9, 2019

zzzeek commented Nov 9, 2019

sqla-tester left a comment

Choose a reason for hiding this comment

sqla-tester commented Nov 11, 2019

pbecotte commented Nov 11, 2019

sqla-tester left a comment

Choose a reason for hiding this comment

sqla-tester commented Nov 15, 2019

sqla-tester left a comment

Choose a reason for hiding this comment

sqla-tester commented Nov 15, 2019

sqla-tester left a comment

Choose a reason for hiding this comment

sqla-tester commented Nov 18, 2019

sqla-tester left a comment

Choose a reason for hiding this comment

sqla-tester commented Nov 22, 2019

sqla-tester left a comment

Choose a reason for hiding this comment

sqla-tester commented Dec 27, 2019

sqla-tester left a comment

Choose a reason for hiding this comment

sqla-tester commented Dec 27, 2019

sqla-tester left a comment

Choose a reason for hiding this comment

sqla-tester commented Dec 28, 2019

Choose a reason for hiding this comment

sqla-tester left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sqla-tester commented Jan 19, 2020

sqla-tester left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pbecotte commented Jan 24, 2020

sqla-tester left a comment

Choose a reason for hiding this comment

sqla-tester commented Jan 24, 2020

sqla-tester commented Jan 31, 2020

sqla-tester commented Feb 4, 2020

sqla-tester commented Feb 4, 2020

CaselIT commented Aug 11, 2023 • edited Loading

CaselIT commented Aug 11, 2023

zzzeek commented Aug 12, 2023

CaselIT commented Aug 12, 2023

pbecotte commented Nov 8, 2019 •

edited

Loading

CaselIT commented Aug 11, 2023 •

edited

Loading