allow changes to the array base column type in migrations #1199

sinisaos · 2025-05-17T05:56:29Z

Related to #1076

dantownsend

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

piccolo/apps/migrations/auto/migration_manager.py

dantownsend · 2025-05-17T14:34:33Z

piccolo/apps/migrations/auto/migration_manager.py

+                            alter_column.db_column_name
+                        )
+                        # first drop array column
+                        await self._run_query(


We can't drop the column and recreate it because there might be data in it.

Yes, you're right. I couldn't get drop_default and set_column_type to work, because I missed using set_default as well. This works in my case

if ( column_class.value_type == list and old_column_class.value_type == list ): # drop default await self._run_query( _Table.alter().drop_default(old_column) ) using_expression = "{}::{}".format( new_column._meta.db_column_name, new_column.column_type, ) # set new base column type await self._run_query( _Table.alter().set_column_type( old_column=old_column, new_column=new_column, using_expression=using_expression, ) ) # we also need to set a new default value which I was missing await self._run_query( _Table.alter().set_default( column=new_column, value=new_column.get_default_value(), ) )

I made a new commit. I hope it's better.

Skelmis

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

Table

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

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

Create and run migration
Modify table to

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

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

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 · 2025-06-10T10:40:02Z

@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 · 2025-06-10T10:42:57Z

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

sinisaos · 2025-06-10T11:03:45Z

@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 · 2025-06-11T08:04:39Z

@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 · 2025-06-11T08:12:01Z

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

piccolo/apps/migrations/auto/migration_manager.py

piccolo/apps/migrations/auto/diffable_table.py

dantownsend · 2025-07-15T16:44:30Z

piccolo/apps/migrations/auto/serialisation.py


+            column_class_name = column.__class__.__name__
+            # we need to manually re-assign the base column
+            column._meta.name = f"{column_class_name.upper()}"


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?

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.

tests/apps/migrations/auto/integration/test_migrations.py

dantownsend · 2025-07-15T16:47:08Z

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

sinisaos · 2025-07-16T06:59:43Z

@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 · 2025-07-17T17:35:34Z

tests/apps/migrations/auto/integration/test_migrations.py

+                [
+                    x.data_type == "ARRAY",
+                    x.is_nullable == "NO",
+                    x.column_default == "'{}'::text[]",


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

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?

@sinisaos Which version of CockroachDB are you running locally?

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

@dantownsend Locally I have v24.3.5.

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

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.

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.

Interesting - thanks!

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

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.

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.

allow changes to the array base column type in migrations

e626de9

sinisaos mentioned this pull request May 17, 2025

Array type migrations fail to import all required items #1076

Closed

sinisaos requested review from Skelmis and dantownsend May 17, 2025 06:13

dantownsend reviewed May 17, 2025

View reviewed changes

sinisaos added 2 commits May 17, 2025 18:53

applied @dantownsend suggestions

b47798c

Merge branch 'piccolo-orm:master' into array_column_migrations

16fd330

sinisaos mentioned this pull request Jun 9, 2025

add enum support to custom forms piccolo-orm/piccolo_admin#436

Open

sinisaos added 2 commits June 9, 2025 15:24

Merge branch 'piccolo-orm:master' into array_column_migrations

c53bd0f

Merge branch 'piccolo-orm:master' into array_column_migrations

5e40946

Skelmis requested changes Jun 10, 2025

View reviewed changes

Skelmis reviewed Jun 11, 2025

View reviewed changes

Skelmis approved these changes Jul 13, 2025

View reviewed changes

sinisaos added the bug Something isn't working label Jul 13, 2025

dantownsend reviewed Jul 15, 2025

View reviewed changes

sinisaos added 2 commits July 16, 2025 06:25

Merge branch 'piccolo-orm:master' into array_column_migrations

612e82c

applied @dantownsend suggestions

64adb74

dantownsend added 6 commits July 17, 2025 18:23

Merge branch 'master' into pr/1199

f417121

use deserialise_params again now it's fixed

af33bca

fix typo

df48959

remove unused var

cc53b9a

merge tests

0995335

remove unused imports

2631ca6

remove more imports

599bb13

dantownsend reviewed Jul 17, 2025

View reviewed changes

sinisaos added 6 commits July 18, 2025 16:50

change tests

9f8b240

try Cockroach v24.3.5 in workflows

021df7f

Merge branch 'piccolo-orm:master' into array_column_migrations

a138af2

Merge branch 'piccolo-orm:master' into array_column_migrations

2372d4c

Merge branch 'piccolo-orm:master' into array_column_migrations

6940f22

update comments

8ab6641

allow changes to the array base column type in migrations #1199

Are you sure you want to change the base?

allow changes to the array base column type in migrations #1199

Uh oh!

Conversation

sinisaos commented May 17, 2025

Uh oh!

dantownsend left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Skelmis left a comment

Choose a reason for hiding this comment

Uh oh!

sinisaos commented Jun 10, 2025

Uh oh!

Skelmis commented Jun 10, 2025

Uh oh!

sinisaos commented Jun 10, 2025

Uh oh!

Skelmis left a comment

Choose a reason for hiding this comment

Uh oh!

sinisaos commented Jun 11, 2025

Uh oh!

Skelmis commented Jun 11, 2025

Uh oh!

Skelmis left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dantownsend commented Jul 15, 2025

Uh oh!

sinisaos commented Jul 16, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants