Ecto: Repo.aggregate/3 does not typecast the result

[kipcole9]

Environment

• Elixir version: 1.9.2 and 1.10.0-dev
• Database and version (PostgreSQL 9.4, MongoDB 3.2, etc.): Postgres 9.6.3
• Ecto version (mix deps): 3.1.x, 3.2.0
• Database adapter and version (mix deps): Postgres 0.15.1
• Operating system: MacOS X Mojave

Current behavior

Repo.aggregate/3 does not support type casting the result. In my example, the ex_money_sql package supports serialising a Money type to Postgres as a composite type. For example these are returned as {"USD",#Decimal<100>} from Postgrex.

It also includes a custom aggregate function to sum money types. The following example illustrates the error:

# Correct value is returned but it is not typecast event though `:payroll` is defined as a `:money_with_currency` type
iex> Repo.aggregate(Organization, :sum, :payroll)
{"USD", #Decimal<300>}

# Now try forced typecasting
iex> Repo.aggregate(Organization, :sum, type(:payroll, :money_with_currency))
== Compilation error in file test/db_test.exs ==
** (CompileError) test/db_test.exs:17: undefined function type/2

Expected behavior

Ecto.aggregate/3 to support typecasting the result as the other ecto_sql macros/function do. This is preferably done automatically since the type is known at compile time. Otherwise support the type/2 macro.

[josevalim]

@kipcole9 does it work if you write the aggregate query by hand (without the type)? I want to figure out it is an Repo.aggregate issue or a general query issue.

[josevalim]

Btw could you please provide a sample app that reproduces the error?

[kipcole9]

@jose, the aggregate function works correctly and there is a test for that in the repo below.

1. Clone https://github.com/kipcole9/money_sql
2. mix deps.get
3. Update the postgres connection information in config/test.exs
4. mix test

The particular tests are in test/db_test.exs. The final test is commented out since it creates a compile error. Its the test where I try to use the type/2 macro in Repo.aggregate/3.

[Eiji7]

@josevalim If I'm not wrong then @kipcole9 missed it in documentation:

aggregate(
  queryable :: Ecto.Queryable.t(),
  aggregate :: :avg | :count | :max | :min | :sum,
  field :: atom(),
  opts :: Keyword.t()
) :: term() | nil

Source: https://hexdocs.pm/ecto/Ecto.Repo.html#c:aggregate/4

This means that ecto does not support type() casting in this place.

Since we have an opts here we can add [type: :money_with_currency] support if you don't mind.

If everything I wrote is correct then I may take a look and create PR for that as it does not looks really hard.

[kipcole9]

@Eiji7, I think that Repo.aggregate/3 should typecast the result automatically since it can already determine the type of the field based upon the schema (for schema queries). Doing so would be in conformance to the typespec.

The option to force a typecast is, in my opinion, a less satisfactory option that I offered as only as an alternative idea that may be required for schemaless queries.

[Eiji7]

@kipcole9 I'm not 100% sure about this. What you wrote is generally correct, but only in case ecto function would return a struct containing such key. This function is just a shortcut for often used queries and not replacement for complex Ecto.Query.API with all its features.

Just for example how about summing by 2 different currencies? Current implementation is a really simple and not over complicated solution. If you would go to work with sum for more than one currency then you most likely would need group_by (money by currency) and therefore you would not be able to call Repo.aggregate/4 (as it does not allows queries with group_by field set).

Look that such change may be considered as breaking change and affect other projects as well. My proposition however allows enhance this function without causing any other problems.

What do you think about it?

[kipcole9]

I recognise that Repo.aggregate/3 isn't a full replacement for Ecto.Query's api. But it does allow an queryable as its first argument so I do think it's a fully fledged member of the family. Its also clear in the guides that it has a special place in the api:

At first, it looks like the implementation of aggregate/4 is quite straight-forward. You could even start to wonder why it was added to Ecto in the first place. However, complexities start to arise on queries that rely on limit, offset or distinct clauses.

I also think its more a surprise that a result isn't typecast than to find that it is not since the rest of the query API does cast (whenever it can detect the the type).

Just for example how about summing by 2 different currencies? ....

My custom postgres aggregate function raises an exception if you try to sum monies that have different currencies :-). As it should - its a domain issue. Not an issue related to Repo.aggregate/3 in my view. I'm not asking for a change in API, or a change in semantics.

[Eiji7]

I also think its more a surprise that a result isn't typecast than to find that it is not since the rest of the query API does cast (whenever it can detect the the type).

Ok, that's the 2nd time you most probably misunderstood documentation. :smiley:

Ecto.Repo.aggregate/4 is of course in Ecto.Repo module and therefore it's not a part of Ecto.Query.API. Personally I was more surprised seeing type/2 macro call in your example.

Of course there is no sense to limit Ecto.Repo.aggregate/4 that much to not support Ecto.Query, but I believe that the most used feature of it is where and type casting is probably a really edge case here.

I'm not asking for a change in API, or a change in semantics.

That depends … There were cases when (probably in Elixir core) API allows to do something that its maintainers not expected and even if that was a breaking change it was considered as bug fix, because such behaviour should never happen.

However this does not mean that if someone expect some behaviour from specific API then it always needs to be a bug. I was a bit confused seeing this issue for first time, because again personally I was used to integer (like count and sum) and float (like avg and sum) results and I never have a use case in which I even had in mind type casting at this place. Also after quickly looking at documentation I did not found an explicit example or mention that it may return custom types. There is only any in typespec which does not say much.

Also I found this part:

Any preload or select in the query will be ignored in favor of the column being aggregated.

If I remember correctly the type casting you expecting for Ecto.Query happens exactly on select field. Again I may be wrong, but just seeing documentation I do not see a behaviour that in some way does not much documentation.

In short everything depends how @josevalim and others would see this. If this would be considered as a bug then it would be fixed as you said. However in other case my proposition is probably better, because as said somebody else in some project may already be in similar use case and did not expect such behaviour. If you would ask for "fix" when this issue won't be considered as a "bug" then it means that it becomes a breaking change as now test for other developers may fail in favor of your tests.

[kipcole9]

Hmmm, i don't think I'm misunderstanding the documentation. I didn't say "part of Ecto.Query", because I know it's not there. But most definitely Repo.aggregate/3 is part of the query API otherwise it wouldn't actually do anything! Its take a queryable as an argument, and it generates a queryable under the covers so I think that qualifies.

I do agree that this can be interpreted as a bug or as a feature request. My position is clear I hope - I believe the user expects, and the typespecs allow, for the returned result to be typecast where possible. Like the rest of the query API (small 'q' query, not Ecto.Query).

As you say, lets see what @josevalim and co think.

[Eiji7]

But most definitely Repo.aggregate/3 is part of the query API otherwise it wouldn't actually do anything! Its take a queryable as an argument, and it generates a queryable under the covers so I think that qualifies.

Yes, it takes Ecto.Query and to be honest I was also really confused on any type in typespec.

Its take a queryable as an argument, and it generates a queryable under the covers so I think that qualifies.

Oh, that's 100% way too generic. There are lots of things you may want to do with Ecto.Query struct. You may want to work with just one field (like select) in order to for example extract some information from query and forget about all other fields. In such case accepting Ecto.Query is easier for few main reasons:

1. There is no need to define new struct and typespec
2. Making API developer-friendly i.e. there is no need to explicitly call query.some_field - simply pass query and function will just have one extra pattern matching.
3. As there is passed already verified struct there is less things to check for exmaple in tests

and having all of that in mind please see again that select with type casting are even omitted at that point as well as group_by field which matches what I just said.

I would like to be well understood, so let's take an example completely outside of it. If there is a conflict of interpretations of specific law paragraph then judge things for what cases such law was introduced. Again go back to documentation and take a look at examples. Of course everything depends to maintainers, but in my opinion it's clear for what purposes it was created at least at first. I just want to let you know the way I think and they way why by default I expect integer and float as "typical" result. Hope you now better understand my point of view.

My position is clear I hope - I believe the user expects, and the typespecs allow, for the returned result to be typecast where possible.

Just to point that. If we would add type option then result would also match typespec and code change would not affect possible other projects.

Anyway I have hope for you as well. I also remember when few times I reported something which I was not sure if such thing is a bug or not. :smiley:

In any case I can make a PR and I just need to know "what and where".

[josevalim]

Fixed on master. Now you don't need the type in any situation. Thanks for the discussion!