Question regarding ecto's group_by

[tkaretsos]

Hello! flop is a quite interesting library and I am currently evaluating whether we can use it in our application. So, I have the following simple schema:

defmodule MyApp.Foo do
  use Ecto.Schema

  @derive {Flop.Schema, filterable: [], sortable: []}

  schema "foo" do
    field :period_start, :date
  end
end

In the foo table there are multiple rows with the same period_start and in the application we need to group them and get a list of ids per date. We use postgres and we achieve that like so:

MyApp.Foo
  |> group_by([f], f.period_start)
  |> select([f], %{
    period_start: f.period_start,
    ids: fragment("array_agg(?)", f.id)
  })
  |> MyApp.Repo.all()

which returns the results in the following format:

[
  %{
    ids: [1, 2],
    period_start: ~D[2023-01-01]
  },
  ...
]

When I try to do the same with the recommended Flop.validate_and_run/3:

MyApp.Foo
  |> group_by([f], f.period_start)
  |> select([f], %{
    period_start: f.period_start,
    ids: fragment("array_agg(?)", f.id)
  })
  |> Flop.validate_and_run(%{})

I get the following error:

[error] cannot aggregate on query with group_by in query:
  (ecto 3.9.4) lib/ecto/repo/queryable.ex:522: Ecto.Repo.Queryable.prepare_for_aggregate/1
  (ecto 3.9.4) lib/ecto/repo/queryable.ex:485: Ecto.Repo.Queryable.query_for_aggregate/2
  (ecto 3.9.4) lib/ecto/repo/queryable.ex:117: Ecto.Repo.Queryable.aggregate/4
  (flop 0.17.2) lib/flop.ex:839: Flop.meta/3
  (flop 0.17.2) lib/flop.ex:647: Flop.run/3
  (flop 0.17.2) lib/flop.ex:682: Flop.validate_and_run/3
...

I wasn't able to find any use case examples with group_by in the docs. Is group_by unsupported or am I using the library in an incorrect way?

[woylie]

Interesting, I haven't used group by clauses in combination with Flop before. The error is caused because by default, Flop takes the final query (minus pagination and order parameters) and runs a count query using Ecto.Repo.aggregate, which does not support group by.

You can get around this by passing a custom count query that does not include a group by clause:

count_query = distinct(MyApp.Foo, [f], f.period_start)
Flop.validate_and_run(Foo, params, for: Foo, count_query: count_query)

Or if you need to to apply Flop filters:

with {:ok, flop} <- Flop.validate(params, for: Foo) do
  count_query = MyApp.Foo |> Flop.filter(flop, for: Foo) |> distinct([f], f.period_start)
  {:ok, Flop.run(Foo, flop, for: Foo, count_query: count_query)}
end

See also https://hexdocs.pm/flop/Flop.html#t:option/0 (I just noticed that the option is listed as order_query instead of count_query - I'll fix that) and https://hexdocs.pm/flop/Flop.html#count/3.

The count is only queried for page and offset pagination types. For first or last (cursor-based pagination), Flop will not query the total count.

# force cursor-pagination and ignore page/page_size/offset/limit parameters
Flop.validate_and_run(Foo, params, for: Foo, pagination_types: [:first, :last])

[tkaretsos]

Thank you for the explanation and the quick response.. It worked fine!

[michallepicki]

I see that Scrivener is detecting distinct and group_by usage and calling such queries as a subquery. This relies on internal Ecto details but I think it would be beneficial if something like that landed in Flop as well. I would suggest tiny changes compared to Scrivener's logic:

• distinct detection should ignore empty lists: when expr == true or (is_list(expr) and expr != []) (source)
• we probably don't need to select the first group_by field, instead of select([{x, source_index}], struct(x, ^[field])) I think something like select(%{dont_care: true}) could work as well
  • if we don't need the first group_by field, the group_by detection / function head can be simplified to aggregate(%{group_bys: group_bys} = query) when group_by != []

[woylie]

Sounds like something we could add. Would you be up for a PR?

[michallepicki]

I'll look into it some time in the next 2 weeks!

[michallepicki]

I opened a draft PR with what I described above, will try to find some time to work on tests soon. @a3kov it would be super cool if you could check if this branch works for your use-case!

[a3kov]

Thanks, will test it

[a3kov]

It worked in my short test

[a3kov]

I've ran into the same issues. So now I understand it's not supported, and only workarounds are possible.

I have doubts about using the query without GROUP BY for counting - it's obviously a different query and not a proper solution..

For now I see the only clean workaround is to run all aggregations either with correlated subquery, or joined subquery, both via fragments.

Yeah usually paginators run the same query as a subquery for counting, but it's not so simple with Ecto - I ran into an issue with return values:
** (Ecto.QueryError) atoms, structs, maps, lists, tuples and sources are not allowed as map values in subquery
My code is returning something like %{model: model, some_count: count} . I guess for counting it has to be a separate query which has a different return type - we can't ask the developer to remove models (schemas) from the query, because people don't want to update their queries every time their schema changes.

I also think Flop may need deeper understanding of the query, because filtering before and after aggregation is different (WHERE vs HAVING). A project using Flop may need both - so Flop has to undertstand which is which.

Another option for Flop of course is just to give up - and direct users to fragments, or ignore aggregates in filters..

[a3kov]

So scrivener does what I described above (changing return values for the count query)

[a3kov]

For someone who needs a good workaround in the meantime, this works:

select: %{product: p, review_count: fragment("SELECT count(*) FROM reviews r WHERE r.product_id = ?", p.id)}

It's called a correlated subquery and should have similar performance to joins and aggregation (at least in postgres). The advantage with this approach is that the main query remains un-aggregated and conflict free