plausible / ecto_ch

Ecto ClickHouse adapter
MIT License
72 stars 10 forks source link

Can't insert data when using `source` option for schema field #135

Closed hkrutzer closed 10 months ago

hkrutzer commented 10 months ago

I'm using the source field option to map a field to a differently named table column. This works when selecting data, but not when inserting data:

Test:

  describe "issue" do
    setup do
      query!("""
      create table if not exists field_source (
        id String,
        field_name String
      ) engine MergeTree
      ORDER BY (id)
      """)

      query!("""
      INSERT INTO field_source VALUES ('insert', 'value');
      """)
      :ok
    end

    defmodule Stuff do
      use Ecto.Schema

      @primary_key false
      schema "field_source" do
        field(:id, Ch, type: "String")
        field(:name, Ch, type: "String", source: :"field_name")
      end
    end

    test "foobar" do
      all(Stuff)
      |> dbg()

      insert_all(Stuff, [
        %{id: "foo", name: "bar"},
        %{id: "foobar", name: "bax"},
      ])
      |> dbg()
    end
  end

Output

[test/ch/type_test.exs:767: Ch.TypeTest."test issue foobar"/1]
all(Stuff) #=> [
  %Ch.TypeTest.Stuff{
    __meta__: #Ecto.Schema.Metadata<:loaded, "field_source">,
    id: "insert",
    name: "value"
  }
]

  1) test issue foobar (Ch.TypeTest)
     test/ch/type_test.exs:765
     ** (RuntimeError) missing type for field :field_name
     code: insert_all(Stuff, [
     stacktrace:
       (ecto_ch 0.3.0) lib/ecto/adapters/clickhouse/schema.ex:229: anonymous fn/2 in Ecto.Adapters.ClickHouse.Schema.extract_types/2
       (elixir 1.15.7) lib/enum.ex:1693: Enum."-map/2-lists^map/1-1-"/2
       (elixir 1.15.7) lib/enum.ex:1693: Enum."-map/2-lists^map/1-1-"/2
       (ecto_ch 0.3.0) lib/ecto/adapters/clickhouse/schema.ex:44: Ecto.Adapters.ClickHouse.Schema.insert_rows/5
       (ecto_ch 0.3.0) lib/ecto/adapters/clickhouse/schema.ex:33: Ecto.Adapters.ClickHouse.Schema.insert_all/8
       (ecto 3.10.3) lib/ecto/repo/schema.ex:59: Ecto.Repo.Schema.do_insert_all/7
       test/ch/type_test.exs:769: (test)
ruslandoga commented 10 months ago

👋 @hkrutzer

The error comes from this part where we try to extract the types (for RowBinary encoding) from the Ecto schema https://github.com/plausible/ecto_ch/blob/36348a1a838f74efadd8346d0982c39e3079955a/lib/ecto/adapters/clickhouse/schema.ex#L229

Seems like we'd need to check for schema.__schema__(:type, __schema__(:field_source, field)) as well. Or would it be the other way around? Eh...

iex> Mix.install [:ecto, :ch]
iex> defmodule Stuff do
  use Ecto.Schema
  @primary_key false
  schema "field_source" do
    field :id, Ch, type: "String"
    field :name, Ch, type: "String", source: :field_name
  end
end
iex> Stuff.__schema__(:type, :name)
{:parameterized, Ch, :string}
iex> Stuff.__schema__(:type, :field_name) # this is what happens
nil
iex> Stuff.__schema__(:field_source, :name)
:field_name
iex> Stuff.__schema__(:field_source, :field_name)
nil
# seems like we'd need to do something like this unless we add our own reflection
iex> field = :field_name
iex> if reverse_source = Enum.find(Stuff.__schema__(:fields), fn other_field -> Stuff.__schema__(:field_source, other_field) == field end) do
  Stuff.__schema__(:type, reverse_source)
end
{:parameterized, Ch, :string}
hkrutzer commented 10 months ago

Not sure, I don't know how other adapters do this, but I couldn't easily find it. I guess they don't have to look up the type like ecto_ch has to?

ruslandoga commented 10 months ago

Yes, other adapters don't need to do this kind of reflection.

ruslandoga commented 10 months ago

https://github.com/plausible/ecto_ch/pull/137 should fix it. It's included in v0.3.1 release.

hkrutzer commented 10 months ago

It's fixed, thanks very much!