[Migration] Change column type use change_type not change the optional of column?

[zw963]

Following is one file script.

require "pg"
require "avram"

class AppDatabase < Avram::Database
end

AppDatabase.configure do |settings|
  settings.credentials = Avram::Credentials.new(database: "test_db", username: "postgres", hostname: "localhost")
end

Avram.configure do |settings|
  settings.database_to_migrate = AppDatabase
  settings.lazy_load_enabled = true
end

class CreateFoo::V20240420041991 < Avram::Migrator::Migration::V1
  def migrate
    create table_for(Foo), if_not_exists: true do
      primary_key id : Int64
      add number : Int32?
      add_timestamps
    end
  end

  def rollback
    drop table_for(Foo)
  end
end

class CreateFoo::V20240420041992 < Avram::Migrator::Migration::V1
  def migrate
    alter table_for(Foo) do
      change_type number : Float64, precision: 10, scale: 5
    end
  end

  def rollback
  end
end

Avram::Migrator::Runner.drop_db(true)
Avram::Migrator::Runner.create_db(true)
Avram::Migrator::Runner.new.run_next_migration
Avram::Migrator::Runner.new.run_next_migration

abstract class BaseModel < Avram::Model
  def self.database : Avram::Database.class
    AppDatabase
  end
end

class Foo < BaseModel
  table do
    column number : Float64?
  end
end

Foo::SaveOperation.create!(number: 70884.70074)
Foo::SaveOperation.create!

query = Foo::BaseQuery.new

p! query.select_count # => 2

As you can see, i change my number column from Int32? into Float64, precision: 10, scale: 5, but underneath, this column still optional.

Thanks.

[davidepaolotua]

I had a quick look at it but I'm kinda perplexed on how to proceed.
I'd honestly add a new statement - like it was done for the default. Three reasons for that:

1. Postgres does not allow for alter column set data .... not null; (and I guess it's a good enough reason)
2. it would be a mess to implement the inversed operation otherwise (not null -> null)

I'll try having a look at it.
@jwoertink, do u see any reasons not to add a new statement?

[jwoertink]

Yeah, this is a tricky one. What are you thinking for a new statement?

[davidepaolotua]

mmmh... that's an even-trickier one, now that I'm double-checking.
The only good news is that this statement set/drop not null is idempotent.
In a nutshell, I can do two things:

• try to reuse as much as possible the fill_with_existing that is already used for the statement - but this would entail that I also need to take care of the opposite case (alter ... drop not null). So, it would become something like
  change_type whatever : Int32?, fill_with_existing: 42. (nilable would be set directly from the type declaration. The problem is that I then get screwed with the default value, as I would need to add a change_default in between :/ nevermind - this would overwrite also pre-existing not nulls, so it's not the way to go
• implement something like make_(not_)null whatever, which could be called after the migration.

Is there a reason for which the change_default is separated from the other changes?
I'd honestly merge them altogether, but I'm pretty sure that if they've been split there must have been a pretty good reason for that.
I mean, having something like
change_type x : Int32, default: 44, fill_nulls_with: 11 would be the best but the fact that they were pretty obviously intentionally split is scaring me

[jwoertink]

We had them split up just for the sake of safety in that it was doing a single operation so you always knew what was happening. When you start adding more options, there's a higher chance of screwing it up like meaning to do one thing but forgetting an option. Like for example, if the method doesn't require passing in default, and you have it set to nil so it can be ignored, then how do we know when you're wanting the default to be nil (NULL) vs when you're just not passing in that argument because you're just updating the type from say Int32? to Int64? or whatever...

I'm not completely opposed to merging them. Or maybe making a new method that's all encompassing while still leaving the separate options?

[davidepaolotua]

I see your point, and it's a pretty valid one :)
Let's do it like this:

• for now I'll create 1/2 new methods just to manage the set not null/ drop not null operations. In this way, you can also do more complicated stuff like "change type - migrate the missing records with the result of a different query - set not null", which is the bare minimum to fix this issue.
• later, if the train does not strand me in the middle of nowhere, I'll try to think about a method to merge them all together, and we keep it separate from the preexisting.