fix: ensure correct alter table alter column statement is generated on data type changes in alembic

[benvdh-incentro]

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

• Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
• Ensure the tests and linter pass
• Code coverage does not decrease (if any source code was changed)
• Appropriate docs were updated (if necessary)

Fixes #471 🦕

[chalmerlowe]

@benvdh-incentro
I am curious, how much info can you see in terms of the results of the code checks? For example, can you see the details associated with the kokoro failure?
Kokoro >> Details >> Failed Target

[benvdh-incentro]

@chalmerlowe I have the details page available (though only on a desktop, not on my smartphone)... so I can see the system 3.8 test is failing with the following error:

Traceback (most recent call last):
  File "/tmpfs/src/github/python-bigquery-sqlalchemy/tests/system/test_alembic.py", line 55, in alembic_table
    migration_context = alembic.migration.MigrationContext.configure(conn, {})
  File "/tmpfs/src/github/python-bigquery-sqlalchemy/.nox/system-3-8/lib/python3.8/site-packages/alembic/runtime/migration.py", line 269, in configure
    return MigrationContext(dialect, connection, opts, environment_context)
  File "/tmpfs/src/github/python-bigquery-sqlalchemy/.nox/system-3-8/lib/python3.8/site-packages/alembic/runtime/migration.py", line 197, in __init__
    self.impl = ddl.DefaultImpl.get_by_dialect(dialect)(
  File "/tmpfs/src/github/python-bigquery-sqlalchemy/.nox/system-3-8/lib/python3.8/site-packages/alembic/ddl/impl.py", line 120, in get_by_dialect
    return _impls[dialect.name]
KeyError: 'bigquery'
- generated xml file: /tmpfs/src/github/python-bigquery-sqlalchemy/system_3.8_sponge_log.xml -

Yesterday I ran all the unit tests locally, but for the system tests, it seemed to hang locally (probably because it needed to connect to a real bigquery instance; or because it was running silently...). I can imagine it was because I moved some code around (that is specific to alembic), and the test was not updated accordingly... I can have a look tomorrow...

[benvdh-incentro]

@chalmerlowe it turns out the alembic system test indeed failed because I moved the code to a separate module. I added it back to base.py and system test suite for 3.8 ran successfully again.

I've also made an attempt to add an additional system test, that tests altering column types... Somehow I only got that part working with a column set to nullable.

In case of columns that are not NULLABLE (or mode required) - as it is the case with the transactions table in the alembic system test - it failed with the following error:

sqlalchemy.exc.DatabaseError: (google.cloud.bigquery.dbapi.exceptions.DatabaseError) 400 Column `account` cannot change mode via SET DATA TYPE.

Location: US
Job ID: a1783672-fe58-478b-aaae-f86a06ba83f3

[SQL: ALTER TABLE `transactions` ALTER COLUMN `account` SET DATA TYPE NUMERIC]
(Background on this error at: http://sqlalche.me/e/13/4xp6)

I have tried playing around with the nullable and existing_nullable arguments of alter_column, but no luck so far. Perhaps it has something to do with the fact that in bigquery a column with mode REQUIRED (non-nullable), can be changed to NULLABLE but not the other way around, while the visit_column_nullable() method of Alembic's DefaultImpl assumes you can convert from non-nullable to nullable, and vice versa.

Perhaps you can have a look at this one, and re-run the CI?

[benvdh-incentro]

@chalmerlowe I got the coverage error as well locally, I am guessing it's because the ImportError in base.py is never reached, as alembic is usually available... but I have no idea how we could test for such an error, nor how relevant it is in actually solving the problem at hand...

What are your thoughts on this?

[chalmerlowe]

based on the results in coverage, the statement that is not tested appears to be the result of our function visit_column_type() as seen in line 1071:

Name                                     Stmts   Miss Branch BrPart  Cover   Missing
------------------------------------------------------------------------------------
sqlalchemy_bigquery/__init__.py             14      0      0      0   100%
sqlalchemy_bigquery/_helpers.py             43      0     14      0   100%
sqlalchemy_bigquery/_struct.py              47      0     16      0   100%
sqlalchemy_bigquery/_types.py               57      0     18      0   100%
sqlalchemy_bigquery/base.py                487      1    144      0    99%   1071

This snippet:

return "%s %s %s" % (
            alter_table(compiler, element.table_name, element.schema),
            alter_column(compiler, element.column_name),
            "SET DATA TYPE %s" % format_type(compiler, element.type_),
        )

I have not messed with coverage much, I am not sure how coverage goes about calculating "coverage" via test.
If they predominantly look for tests that match based on file name and based on test function name?

I can poke around to see what I can find out, but won't be til later today or tomorrow, prolly.

[chalmerlowe]

@benvdh-incentro thanks your time and effort on this.
Your write-ups and exploration of the issues really helped.
You have been very patient, my apologies for the time it took to process this PR. Juggling a lot of issues across multiple repos.

This should show up in the next release.

[benvdh-incentro]

@chalmerlowe thank you too for the last bit to get the code through the pipeline. It was my pleasure to help and learn a lot along the way.