Ecto: Decimal type always changes

[atomkirk]

• Elixir version (elixir -v): 1.6.4
• Database and version (PostgreSQL 9.4, MongoDB 3.2, etc.): Mysql 5.7.18
• Ecto version (mix deps): 2.2.9
• Database adapter and version (mix deps): mariaex 0.8.3
• Operating system: Mac OS Sierra

Current behavior

When I inspect changesets for schemas with :decimal type, :amount is always in changes when its value did not change.

[
  #Ecto.Changeset<
    action: :update,
    changes: %{amount: #Decimal<599>},
    errors: [],
    data: #ZB.JournalEntryLine<>,
    valid?: true
  >,
  #Ecto.Changeset<
    action: :update,
    changes: %{amount: #Decimal<599>, chart_account_id: 7377718},
    errors: [],
    data: #ZB.JournalEntryLine<>,
    valid?: true
  >
]

Expected behavior

In that case above, the only thing that should be in the changes is chart_account_id: 7377718 cause its the only thing that is different

[josevalim]

I think this is because we are doing structural comparison. @michalmuskala should we add compare to Ecto.Type?

[michalmuskala]

I'm pretty sure the reason for this is that decimals can't be compared with ==, yet that's what we're doing to decide if a field changed or not.

I'm not really sure what could be a solution here that wouldn't require doing a lot of type-specific stuff (and then, how to make it generic...).

[atomkirk]

Yeah, you're suppose to use Decimal.equal? or Decimal.cmp

I got the impression that :decimal was officially supported as a type by ecto so thats why I submitted the issue here. I hope its appropriate

[michalmuskala]

Oh yeah, the issue is definitely appropriate. This shouldn't be happening.

[josevalim]

@michalmuskala what about my suggestion of adding compare/2 to Ecto.Type? :)

[michalmuskala]

Yup, I did miss it.

Adding a callback would be an option. It should probably be an optional callback, though, which means we'd have a lot of function_exported? guards.

[wojtekmach]

I tried reproducing it and for a simple use case it looks good, perhaps it's only broken in more complex cases. @atomkirk are you using associations or something?

iex> Ecto.Changeset.change(%Post{decimal: Decimal.new(599)}, decimal: Decimal.new(599)).changes
%{}

Having Ecto.Type.compare is probably the right thing to do anyway but would be good to reproduce it first.

[atomkirk]

Maybe try passing it in as an integer param, like from a client, and having ecto cast it to decimal

[wojtekmach]

that's not it:

iex> Ecto.Changeset.cast(%Post{decimal: Decimal.new(599)}, %{decimal: 599}, ~w(decimal)a).changes
%{}

[atomkirk]

Oh, because, when its pulled from the database, it has decimal places:

iex(5)> Decimal.new(9) == Decimal.new(9)
true
iex(6)> Decimal.new(9) == Decimal.new(9.000)
false
iex(7)> 9 == 9.000
true
iex(8)> Decimal.new(9) |> Decimal.cmp(Decimal.new(9.000))
:eq
iex(9)> Decimal.new(9) |> Decimal.equal?(Decimal.new(9.000))
true

Example:

iex(3)> pc = ZB.Repo.get(ZB.PostalCode, "84025")
%ZB.PostalCode{
  __meta__: #Ecto.Schema.Metadata<:loaded, "postal_codes">,
  lat: #Decimal<40.98000000>,
  lng: #Decimal<-111.90000000>,
  percent_graduate_degree: 17.5731,
}
iex(4)> cs = ZB.PostalCode.changeset(pc, %{
...(4)> "lat" => 40.98000000,
...(4)> "percent_graduate_degree" => 17.5731,
...(4)> })
#Ecto.Changeset<
  action: nil,
  changes: %{lat: #Decimal<40.98>},
  errors: [],
  data: #ZB.PostalCode<>,
  valid?: true
>

[josevalim]

@atomkirk but for decimal Decimal. new(9) and Decimal.new(9.000) are definitely not equivalent.

In any case, I still think there is a bug here just waiting to happen because depending on the amount of operations you do on decimal, two numbers that are printed the same, such as 599 in the originally reported issue, may not be represented exactly the same in decimal.

[atomkirk]

@josevalim yes but the point I was trying to make was that if the user passes 9 for a float, 9 == 9.0 works. So it should work that way with decimal. Also as you can see in the bottom example, it comes from the database as 40.98000000 and I pass exactly 40.98000000 to the changeset and it still thinks its not equal.

[atomkirk]

Oh you mean that because the of how decimal type is special, 9 == 9.0 should not be considered equal? Ok, maybe… 🤔 but in the case of the user sending info, at least in our case, we'd like them to be :)

Regardless, the 40.98000000 bottom example shows that ecto doesn't consider them equal even when the decimal places are exactly the same.

[josevalim]

@atomkirk We are talking about different types. You cannot use integer and floats to dictate how a decimal behaves. Different types give different guarantees. For decimal, 48.90 may not the same as 48.90000 because they give different guarantees regarding the precision.

48.90 implies a precision of +-0.005 while 48.90000 implies precision of +-0.0000005.

Regardless, the 40.98000000 bottom example shows that ecto doesn't consider them equal even when the decimal places are exactly the same.

What happens when you pass 40.98000000 as a string instead of a float in that same example?

[wojtekmach]

but for decimal Decimal.new(9) and Decimal.new(9.000) are definitely not equivalent.

worth mentioning that:

iex> Decimal.cmp(Decimal.new(9), Decimal.new(9.000)) # same with 9.0, "9.0000" etc
:eq

[josevalim]

@wojtekmach oh, so Decimal doesn't consider the precision? Interesting.

[atomkirk]

Well, I suspect you are right that precision matters and needs to be remembered, but IMO when comparing, 9 should be considered equal to 9.00

Can you think of a logical reason those shouldn’t be considered equal?

[wojtekmach]

fwiw Time.compare(~T[00:00:00], ~T[00:00:00.000]) => :eq :) I remember the subject of precision was brought up when adding calendar types so I'll make sure to read up on that but imho it'd be practical to use Decimal.cmp for comparisons.

[josevalim]

@wojtekmach that's from where my confusion came from. I thought we used precision everywhere but we agreed to use precision nowhere.

Yeah, in any case, we need to use Decimal.compare/2. :)

[josevalim]

@wojtekmach thanks for correcting my misconceptions!

[wojtekmach]

the problem with compare/2 optional callback returning :lt/:gt is it doesn't necessarily make sense for some values like e.g. URIs, ranges etc, so a equal?/2 might be more appropriate. That being said for things like URIs we wouldn't implement the callback because it's safe to rely on structural comparison. I can't think of an example where structural comparison is unsafe and :lt/:gt doesn't make sense so I think we're good but I thought I'd mention it anyway 😅

[michalmuskala]

That's a good point, that for many types it might make sense to have a custom equality implementation, but not necessarily a comparison. Even though we can always compare according to erlang term order, this is supposed to be a semantic comparison.

[josevalim]

On the other hand, having compare could allow us to generalize things like validate_number.

[LostKobrakai]

On the other hand, having compare could allow us to generalize things like validate_number.

Does Ecto.Type really need to be concerned with a :lt/:eq/:gt differenciation? To me this feels more like a Ecto.Type.Orderable behavior, while the core issue of this thread here is detecting change.

Take for example a type for MapSet. There's no inherent order to one set vs. another, but there's MapSet.equals?/1 for relyable checking of equality.

[atomkirk]

Wanted to post this here for anyone wanting a temporary fix for this issue, also feedback if anyone has any :)

  @doc """
  Ecto doesn't compare Decimal types with `Decimal.equal?` so this fixes that.

  ## Examples
    iex> Ecto.Changeset.cast(%ZB.JournalEntryLine{amount: Decimal.new(3.00)}, %{"amount" => "3.00000"}, [:amount])
    ...> |> Utils.Changeset.fix_decimal_equal()
    ...> |> Map.get(:changes)
    %{}

    iex> Ecto.Changeset.cast(%ZB.JournalEntryLine{amount: Decimal.new(3.00)}, %{"amount" => "3.01000"}, [:amount])
    ...> |> Utils.Changeset.fix_decimal_equal()
    ...> |> Map.get(:changes)
    %{amount: Decimal.new("3.01000")}
  """
  def fix_decimal_equal(%{data: %{__struct__: schema}} = changeset) do
    schema.__schema__(:fields)
    |> Enum.reduce(changeset, fn field, cs ->
      if schema.__schema__(:type, field) == :decimal do
        old = Map.get(cs.data, field)
        new = Map.get(cs.changes, field)
        # if a decimal field changes, make sure its truly different, otherwise remove change
        if old && new && Decimal.equal?(old, new) do
          delete_change(cs, field)
        end
      end || cs
    end)
  end

[wojtekmach]

When we solve this bug we may remove some use cases e.g. people may want to actually differentiate between 9 and 9.000 in the DB. I'm not sure how big of a deal this is.

In pg, numeric type without precision & scale stores arbitrary data [1]:

without any precision or scale creates a column in which numeric values of any precision and scale can be stored, up to the implementation limit on precision.

In mysql, numeric is implemented as decimal [2] [3] and defaults to precision/scale: (10,0):

In standard SQL, the syntax DECIMAL(M) is equivalent to DECIMAL(M,0). Similarly, the syntax DECIMAL is equivalent to DECIMAL(M,0), where the implementation is permitted to decide the value of M. MySQL supports both of these variant forms of DECIMAL syntax. The default value of M is 10.

I wrote this integration test:

    created = TestRepo.insert!(%Post{arbitrary_decimal: Decimal.new("1.0")})
    assert created.arbitrary_decimal == Decimal.new("1.0")
    loaded = TestRepo.get!(Post, created.id)
    assert loaded.arbitrary_decimal == Decimal.new("1.0")

and unsurprisingly it passes on pg but fails on mysql.

I think we should move forward with fixing this bug in any case.

[1] https://www.postgresql.org/docs/current/static/datatype-numeric.html
[2] https://dev.mysql.com/doc/refman/5.7/en/fixed-point-types.html
[3] https://dev.mysql.com/doc/refman/5.7/en/precision-math-decimal-characteristics.html

[josevalim]

@wojtekmach I think our defautl comparison should be strict. I.e. the same number of places, but if somebody wants relaxed rules they can have their own decimal type. Thoughts?

[wojtekmach]

@josevalim I think being strict and explicit is good but then folks will run into issues like described in this issue. Also, as was mentioned in this ticket:

iex(4)> Time.compare(~T[09:00:00.000000], ~T[09:00:00])
:eq
iex(5)> Decimal.cmp(1, "1.000000")
:eq

so I honetly don't know.

edit: I mention Time.compare/Decimal.cmp not as implementation details (that we may use) but rather as people's expectations how this should work.

[josevalim]

I see. Let’s go with the semantics of Decimal.cmp then.

José Valimwww.plataformatec.com.br
http://www.plataformatec.com.br/Founder and Director of R&D

[LostKobrakai]

Can't people who care about changing e.g. Decimal.new("1.0") => Decimal.new("1.00") use force_change()? Sure it would be an additional step, but I feel like sticking with Decimal.cmp is the expectation of users.

[fertapric]

@LostKobrakai Or define their own decimal type (i.e. StrictDecimal).

Regarding this:

the problem with compare/2 optional callback returning :lt/:gt is it doesn't necessarily make sense for some values like e.g. URIs, ranges etc, so a equal?/2 might be more appropriate. That being said for things like URIs we wouldn't implement the callback because it's safe to rely on structural comparison.

And this:

On the other hand, having compare could allow us to generalize things like validate_number.

Optional callback compare/2? or equal?/2?

As an Ecto.Type user, would compare/2 provide any benefit over equal?/2? From the user's perspective, I think equal?/2 would be simpler to implement and to understand (to include a change or not) :)

[josevalim]

It should be equals? for the reasons @LostKobrakai said above.

José Valimwww.plataformatec.com.br
http://www.plataformatec.com.br/Founder and Director of R&D