Aggregation type load differs from normal queries

[Wigny]

Elixir version

1.15.4

Database and Version

MySQL 5.7

Ecto Versions

3.10.3

Database Adapter and Versions (postgrex, myxql, etc)

myxql 0.6.3

Current behavior

When I created a custom Ecto.ParameterizedType for handling ISO 8601 Durations with Timex, I defined it to be of type :float, later when I tried to apply this custom type on an already existent schema field of type :decimal, I noticed that loading and dumping into the DB worked fine in normal queries, but it fails when aggregating.

Mix.install([
  {:ecto_sql, "~> 3.10"},
  {:myxql, ">= 0.0.0"},
  {:timex, "~> 3.7"}
])

Application.put_env(:my_app, MyApp.Repo,
  username: "root",
  password: "root",
  hostname: "localhost",
  database: "my_app_dev"
)

defmodule MyApp.Repo do
  use Ecto.Repo,
    adapter: Ecto.Adapters.MyXQL,
    otp_app: :my_app
end

defmodule MyApp.Repo.Migrations.CreateTravels do
  use Ecto.Migration

  def change do
    create table("travels") do
      add(:time, :decimal)
    end
  end
end

defmodule MyApp.Type.Duration do
  use Ecto.ParameterizedType

  alias Timex.Duration

  defguardp is_duration(value) when is_struct(value, Duration)

  @impl true
  def type(_params), do: :float

  @impl true
  def init(opts), do: %{unit: opts[:unit]}

  @impl true
  def cast(value, _params) when is_nil(value), do: {:ok, nil}
  def cast(value, _params) when is_duration(value), do: {:ok, value}
  def cast(value, params) when is_float(value), do: {:ok, from_float(value, params.unit)}

  @impl true
  def load(value, _, _params) when is_nil(value), do: {:ok, nil}
  def load(value, _, params) when is_float(value), do: {:ok, from_float(value, params.unit)}

  @impl true
  def dump(value, _, _params) when is_nil(value), do: {:ok, nil}
  def dump(value, _, params) when is_duration(value), do: {:ok, to_float(value, params.unit)}

  defp from_float(value, :microseconds), do: Duration.from_microseconds(value)
  defp from_float(value, :milliseconds), do: Duration.from_milliseconds(value)
  defp from_float(value, :seconds), do: Duration.from_seconds(value)
  defp from_float(value, :minutes), do: Duration.from_minutes(value)
  defp from_float(value, :hours), do: Duration.from_hours(value)

  defp to_float(value, :microseconds), do: Duration.to_microseconds(value)
  defp to_float(value, :milliseconds), do: Duration.to_milliseconds(value)
  defp to_float(value, :seconds), do: Duration.to_seconds(value)
  defp to_float(value, :minutes), do: Duration.to_minutes(value)
  defp to_float(value, :hours), do: Duration.to_hours(value)
end

defmodule MyApp.Travels.Travel do
  use Ecto.Schema

  schema "travels" do
    field :duration, MyApp.Type.Duration, unit: :minutes, source: :time
  end
end

defmodule MyApp.Travels do
  alias MyApp.Repo
  alias MyApp.Travels.Travel

  def test do
    _ = Repo.__adapter__().storage_down(Repo.config())
    :ok = Repo.__adapter__().storage_up(Repo.config())

    {:ok, _} = Supervisor.start_link([Repo], strategy: :one_for_one)

    Ecto.Migrator.run(Repo, [{0, Repo.Migrations.CreateTravels}], :up,
      all: true,
      log_migrations_sql: :debug
    )

    Repo.insert(%Travel{duration: Timex.Duration.from_minutes(30)})
    Repo.insert(%Travel{duration: Timex.Duration.from_hours(4.2)})

    IO.inspect(Repo.all(Travel), label: :travels)
    IO.inspect(Repo.aggregate(Travel, :sum, :duration), label: :travels_duration)
  end
end

MyApp.Travels.test()

For the code above, the Repo.aggregate/3 function throws:

** (FunctionClauseError) no function clause matching in MyApp.Type.Duration.load/3    

    The following arguments were given to MyApp.Type.Duration.load/3:

        # 1
        Decimal.new("282")

        # 2
        #Function<34.98793833/2 in Ecto.Type.process_loaders/3>

        # 3
        %{unit: :minutes}

    /home/wigny/workspace/example.exs:49: MyApp.Type.Duration.load/3
    (ecto 3.10.3) lib/ecto/type.ex:591: Ecto.Type.load/3
    (ecto 3.10.3) lib/ecto/type.ex:911: Ecto.Type.process_loaders/3
    (ecto 3.10.3) lib/ecto/repo/queryable.ex:411: Ecto.Repo.Queryable.process/4
    (ecto 3.10.3) lib/ecto/repo/queryable.ex:283: anonymous fn/3 in Ecto.Repo.Queryable.postprocessor/4
    (elixir 1.14.5) lib/enum.ex:1658: Enum."-map/2-lists^map/1-0-"/2
    (ecto 3.10.3) lib/ecto/repo/queryable.ex:235: Ecto.Repo.Queryable.execute/4
    (ecto 3.10.3) lib/ecto/repo/queryable.ex:19: Ecto.Repo.Queryable.all/3

Expected behavior

Since the MyXQL adapter already can load decimals as float, it's desirable that the aggregation result be correctly loaded into the :float type as well:

{:ok, 282.0} = Ecto.Type.adapter_load(Ecto.Adapters.MyXQL, :float, Decimal.new("282"))
{:ok, 282.0} = Ecto.Type.adapter_load(Ecto.Adapters.MyXQL, {:maybe, :float}, Decimal.new("282")) # currently returns decimal instead of float

[josevalim]

This is expected behavior. When you define your type, Ecto bypassed all of its type logic so you have full control. In this case, the database believe decimal is a better return and it is being surfaced up all the way to your Ecto type, so you just handle it accordingly!

[Wigny]

Got it! I'm just a little bit confused on why it's expected this behaviour on the Repo.aggregate/3 function, but it differs from the Repo.one/2 or Repo.all/2 behaviour... The question is why these last two functions loads a decimal as float when using the MyXQL adapter while the aggregate shouldn't? Shouldn't all these functions be consistent on loading from the database?

[greg-rychlewski]

The MyXQL adapter has special behaviour to convert decimals to float before it goes to the generic Ecto conversions

MyXQL Adapter:

  ...
  def loaders(:float, type), do: [&float_decode/1, type]
   ...

  defp float_decode(%Decimal{} = decimal), do: {:ok, Decimal.to_float(decimal)}
  defp float_decode(x), do: {:ok, x}

Ecto:

defp load_float(term) when is_float(term), do: {:ok, term}
defp load_float(term) when is_integer(term), do: {:ok, :erlang.float(term)}
defp load_float(_), do: :error

Maybe the MyXQL adapter should also pattern match on {:maybe. :float}. But not 100% sure the history behind these functions.

[josevalim]

:maybe was added to handle exactly aggregations but I am not sure if goes all the way to adapters, does it? If {:maybe, :float} is a fix, then I am fine with adding it, but perhaps I would rename :maybe to :aggregate.

[Wigny]

Sorry, but the merged PR still doesn't solve the problem I'm facing, right @greg-rychlewski?

I'm still receiving the same error and when trying to understand the problem, it seems that the Ecto.Type.adapter_load/3 is not prepared to receive a {:maybe, {:parameterized, MyApp.Type.Duration, %{unit: :seconds}}} type yet passed by the Ecto.Repo.Queryable.load!/5 function. Could you tell me if that's right?

iex(3)> Ecto.Type.adapter_load(Ecto.Adapters.MyXQL, {:parameterized, MyApp.Type.Duration, %{unit: :seconds}}, Decimal.new("282"))       
{:ok, #<Duration(PT4M42S)>}
iex(4)> Ecto.Type.adapter_load(Ecto.Adapters.MyXQL, {:maybe, {:parameterized, MyApp.Type.Duration, %{unit: :seconds}}}, Decimal.new("282"))
** (FunctionClauseError) no function clause matching in MyApp.Type.Duration.load/3    

    The following arguments were given to MyApp.Type.Duration.load/3:

        # 1
        Decimal.new("282")

        # 2
        #Function<34.13828884/2 in Ecto.Type.process_loaders/3>

        # 3
        %{unit: :seconds}

    iex:49: MyApp.Type.Duration.load/3
    (ecto 3.11.0-dev) lib/ecto/type.ex:607: Ecto.Type.load/3
    (ecto 3.11.0-dev) lib/ecto/type.ex:939: Ecto.Type.process_loaders/3

[greg-rychlewski]

@Wigny Could you try the most up to date main?

[Wigny]

It's working! Thank you!