allow changes to the array base column type in migrations

[sinisaos]

Related to #1076

[dantownsend]

Thanks for trying to fix this - I would really like to get it done.

[Skelmis]

This pull request has not worked for my test case seen below

1. Table

from piccolo.columns import Array, Text, Integer
from piccolo.table import Table

class TestTable(Table):
    my_col = Array(base_column=Integer())

2. Create and run migration
3. Modify table to

from piccolo.columns import Array, Text, Integer
from piccolo.table import Table

class TestTable(Table):
    my_col = Array(base_column=Text())

4. Create and run migrations resulting in the following traceback

near "ALTER": syntax error
Traceback (most recent call last):
  File "/home/skelmis/tmp/piccolo_array_pr/venv/lib/python3.12/site-packages/targ/__init__.py", line 449, in run
    command.call_with(arg_class)
  File "/home/skelmis/tmp/piccolo_array_pr/venv/lib/python3.12/site-packages/targ/__init__.py", line 230, in call_with
    asyncio.run(self.command(**cleaned_kwargs))
  File "/usr/lib/python3.12/asyncio/runners.py", line 195, in run
    return runner.run(main)
           ^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.12/asyncio/runners.py", line 118, in run
    return self._loop.run_until_complete(task)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "uvloop/loop.pyx", line 1518, in uvloop.loop.Loop.run_until_complete
  File "/home/skelmis/tmp/piccolo_array_pr/piccolo/apps/migrations/commands/forwards.py", line 161, in forwards
    response = await run_forwards(
               ^^^^^^^^^^^^^^^^^^^
  File "/home/skelmis/tmp/piccolo_array_pr/piccolo/apps/migrations/commands/forwards.py", line 122, in run_forwards
    response = await manager.run()
               ^^^^^^^^^^^^^^^^^^^
  File "/home/skelmis/tmp/piccolo_array_pr/piccolo/apps/migrations/commands/forwards.py", line 99, in run
    return await self.run_migrations(app_config)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/skelmis/tmp/piccolo_array_pr/piccolo/apps/migrations/commands/forwards.py", line 83, in run_migrations
    await response.run()
  File "/home/skelmis/tmp/piccolo_array_pr/piccolo/apps/migrations/auto/migration_manager.py", line 1048, in run
    await self._run_alter_columns(backwards=backwards)
  File "/home/skelmis/tmp/piccolo_array_pr/piccolo/apps/migrations/auto/migration_manager.py", line 505, in _run_alter_columns
    await self._run_query(
  File "/home/skelmis/tmp/piccolo_array_pr/piccolo/apps/migrations/auto/migration_manager.py", line 426, in _run_query
    await query.run()
  File "/home/skelmis/tmp/piccolo_array_pr/piccolo/query/base.py", line 440, in run
    return await engine.run_ddl(self.ddl[0], in_pool=in_pool)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/skelmis/tmp/piccolo_array_pr/piccolo/engine/sqlite.py", line 820, in run_ddl
    response = await self._run_in_existing_connection(
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/skelmis/tmp/piccolo_array_pr/piccolo/engine/sqlite.py", line 756, in _run_in_existing_connection
    async with connection.execute(query, args) as cursor:
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/skelmis/tmp/piccolo_array_pr/venv/lib/python3.12/site-packages/aiosqlite/context.py", line 41, in __aenter__
    self._obj = await self._coro
                ^^^^^^^^^^^^^^^^
  File "/home/skelmis/tmp/piccolo_array_pr/venv/lib/python3.12/site-packages/aiosqlite/core.py", line 183, in execute
    cursor = await self._execute(self._conn.execute, sql, parameters)
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/skelmis/tmp/piccolo_array_pr/venv/lib/python3.12/site-packages/aiosqlite/core.py", line 122, in _execute
    return await future
           ^^^^^^^^^^^^
  File "/home/skelmis/tmp/piccolo_array_pr/venv/lib/python3.12/site-packages/aiosqlite/core.py", line 105, in run
    result = function()
             ^^^^^^^^^^
sqlite3.OperationalError: near "ALTER": syntax error

Configured with DB = SQLiteEngine(). I did not yet test with postgres or regular migrations

[sinisaos]

@Skelmis Thanks for your effort but automatic migrations aren't fully supported for Sqlite because Sqlite has limited support for ALTER table. With Postgres and Cockroach works. Feel free to try this PR with Postgres or Cockroach. Thanks again.

[Skelmis]

My bad! I'll give it another go in the next couple days.

[sinisaos]

@Skelmis No problem. Thanks for the comment.

[Skelmis]

Trying to go from Integer to Serial yields a stack trace when I'd expected it to instead say it was unsupported. I'm unsure if this stack trace is a blocking issue tho

Expected: asyncpg.exceptions.FeatureNotSupportedError: array of serial is not implemented
Actual:

`_table` isn't defined - the Table Metaclass should set it.
Traceback (most recent call last):
  File "/home/skelmis/tmp/piccolo_array_pr/venv/lib/python3.12/site-packages/targ/__init__.py", line 449, in run
    command.call_with(arg_class)
  File "/home/skelmis/tmp/piccolo_array_pr/venv/lib/python3.12/site-packages/targ/__init__.py", line 230, in call_with
    asyncio.run(self.command(**cleaned_kwargs))
  File "/usr/lib/python3.12/asyncio/runners.py", line 195, in run
    return runner.run(main)
           ^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.12/asyncio/runners.py", line 118, in run
    return self._loop.run_until_complete(task)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "uvloop/loop.pyx", line 1518, in uvloop.loop.Loop.run_until_complete
  File "/home/skelmis/tmp/piccolo_array_pr/piccolo/apps/migrations/commands/forwards.py", line 161, in forwards
    response = await run_forwards(
               ^^^^^^^^^^^^^^^^^^^
  File "/home/skelmis/tmp/piccolo_array_pr/piccolo/apps/migrations/commands/forwards.py", line 122, in run_forwards
    response = await manager.run()
               ^^^^^^^^^^^^^^^^^^^
  File "/home/skelmis/tmp/piccolo_array_pr/piccolo/apps/migrations/commands/forwards.py", line 99, in run
    return await self.run_migrations(app_config)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/skelmis/tmp/piccolo_array_pr/piccolo/apps/migrations/commands/forwards.py", line 83, in run_migrations
    await response.run()
  File "/home/skelmis/tmp/piccolo_array_pr/piccolo/apps/migrations/auto/migration_manager.py", line 1048, in run
    await self._run_alter_columns(backwards=backwards)
  File "/home/skelmis/tmp/piccolo_array_pr/piccolo/apps/migrations/auto/migration_manager.py", line 511, in _run_alter_columns
    new_column.column_type,
    ^^^^^^^^^^^^^^^^^^^^^^
  File "/home/skelmis/tmp/piccolo_array_pr/piccolo/columns/column_types.py", line 2638, in column_type
    return f"{self.base_column.column_type}[]"
              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/skelmis/tmp/piccolo_array_pr/piccolo/columns/column_types.py", line 778, in column_type
    engine_type = self._meta.engine_type
                  ^^^^^^^^^^^^^^^^^^^^^^
  File "/home/skelmis/tmp/piccolo_array_pr/piccolo/columns/base.py", line 227, in engine_type
    engine = self.table._meta.db
             ^^^^^^^^^^
  File "/home/skelmis/tmp/piccolo_array_pr/piccolo/columns/base.py", line 209, in table
    raise ValueError(
ValueError: `_table` isn't defined - the Table Metaclass should set it.

But going from Text to Integer works as expected :)

[sinisaos]

@Skelmis I also encountered that error which I wrote in a comment on the issue. Postgres allows creating arrays with the Serial column as the base column, but this is not recommended and it is advised to use Integer as the type instead. I don't know the real reason why is this not implemented in asyncpg, but this PR doesn't work with Serial arrays (Varchar, Text, Integer etc. column works). If you both have any ideas, I love to hear them.

[Skelmis]

Yea, I was more so curious if we should deal with the ValueError: _table isn't defined - the Table Metaclass should set it. error or leave as is. The not implemented existing error is fine

[Skelmis]

Some weird side case errors that are unhandled but as I see it there is nothing blocking this merge

[dantownsend]

There's no need for an f-string here.

This works fine:

column._meta.name = column_class_name.upper()

Why do we have to set the column's name?

[sinisaos]

I'm resetting base_column here because if we don't, ValueError: _name is not defined - Table Metaclass should set it is raised when we run the migration. If you have a better solution, please change this.

[dantownsend]

@Skelmis Thanks for reviewing. I haven't had a chance to test much, but I think it's getting there.

[sinisaos]

@dantownsend Thanks for the review. I've made changes and implemented your suggestions and tried to explain (in the comments) what I mean by those changes.

[dantownsend]

This will probably fail in CockroachDB - I need to figure out what it is in CochroachDB.

[sinisaos]

I tried this PR locally with CockroachDB and everything works fine. The changes to base_column in the migrations are correct. The problem with this test is that in CockroachDB column_default is ARRAY[], not '{}'::text[] like in Postgres. I think we have two options here. Skip that test for CockroachDB or make two different tests with the engines_only decorator like this

@engines_only("postgres")
def test_array_base_column_change_postgres(self):
    """
    There was a bug when trying to change the base column of an array:

    https://github.com/piccolo-orm/piccolo/issues/1076

    It wasn't importing the base column, e.g. for ``Array(Text())`` it
    wasn't importing ``Text``.

    """
    self._test_migrations(
        table_snapshots=[
            [self.table(column)]
            for column in [
                Array(base_column=Varchar()),
                Array(base_column=Text()),
            ]
        ],
        test_function=lambda x: all(
            [
                x.data_type == "ARRAY",
                x.is_nullable == "NO",
                x.column_default == "'{}'::text[]",
            ]
        ),
    )

@engines_only("cockroach")
def test_array_base_column_change_cockroach(self):
    """
    There was a bug when trying to change the base column of an array:

    https://github.com/piccolo-orm/piccolo/issues/1076

    It wasn't importing the base column, e.g. for ``Array(Text())`` it
    wasn't importing ``Text``.

    """
    self._test_migrations(
        table_snapshots=[
            [self.table(column)]
            for column in [
                Array(base_column=Varchar()),
                Array(base_column=Text()),
            ]
        ],
        test_function=lambda x: all(
            [
                x.data_type == "ARRAY",
                x.is_nullable == "NO",
                x.column_default == "ARRAY[]",
            ]
        ),
    )

What do you think?

[dantownsend]

@sinisaos Which version of CockroachDB are you running locally?

It's still failing for me locally, even if I expect column_default == 'ARRAY[]'.

[sinisaos]

@dantownsend Locally I have v24.3.5.

[sinisaos]

I pushed commit to see if work with workflows which have v24.1.0

[dantownsend]

I need to try running the ALTER commands manually, to see if it's something we're doing wrong, or CockroachDB just doesn't allow us to change the type of an array column.

[sinisaos]

I pushed a commit with workflow changed to use CockroachDB v24.3.5 and the test passed as you can see. Now the question is whether we want to limit ourselves to just one version.

[dantownsend]

Interesting - thanks!

I'm happy with upgrading our CI to use this version of CockroachDB because v24.3 seems to be their LTS version.

[sinisaos]

I agree and I think you can merge this PR. If there is a problem with that test in the future, we can always skip it because only that test does not work in other versions. The code works without any problems in all versions if we actually use it in some application.

[dantownsend]

I think it's close. I want to test it a bit though - there may be edge cases with converting from certain array types to other array types. Once we can use the playground with Cockroach, that will help testing.