Introduce an AST for preprocessing + postprocessing #2155
Conversation
|
Hi @shdblowers, @jbachhardie, @AndrewDryga! If this pull request is merged, it will require changes in adapters in order to support v2.2. The good news are: those changes can be done in a way to remain compatible with earlier codes and they all simplify the adapter code! I will leave comments in the pull request for you to tag along. |
| "Please specify a schema or specify exactly which fields you want to select") | ||
| end | ||
| intersperse_map(fields, ", ", &[name, ?. | quote_name(&1)]) | ||
| defp expr({:&, _, [idx]}, sources, query) do |
There was a problem hiding this comment.
Ecto 2.2 will no longer pass those weird and invalid {:&, _, list_with_three_elements}. It will pass {:&, [], [counter]} which should behave as the old {:&, _, [counter, nil, nil]}. If you want to remain backwards compatible, you just need to add this new clause and keep the old one around.
| @@ -377,8 +374,8 @@ if Code.ensure_loaded?(Mariaex) do | |||
| ["NOT (", expr(expr, sources, query), ?)] | |||
| end | |||
|
|
|||
| defp expr(%Ecto.SubQuery{query: query, fields: fields}, _sources, _query) do | |||
| query.select.fields |> put_in(fields) |> all() | |||
| defp expr(%Ecto.SubQuery{query: query}, _sources, _query) do | |||
There was a problem hiding this comment.
If you want to remain backwards compatible, use this implementation for subqueries:
defp expr(%Ecto.SubQuery{query: query} = subquery, _sources, _query) do
if Map.has_key?(subquery, :fields) do
query.select.fields |> put_in(subquery.fields) |> all()
else
all(query)
end
end
| @@ -570,33 +538,6 @@ defmodule Ecto.Adapters.SQL do | |||
| end | |||
| end | |||
|
|
|||
| defp process_row(row, process, fields, sources) do | |||
There was a problem hiding this comment.
The difference in this code and the previous code is that adapters are no longer responsible for the whole logic below. To handle this in a backwards compatible fashion, you can change this code in v2.1:
mapper = &process_row(&1, process, fields, sources)
to something like this:
mapper = processor(process, fields, sources)
where
defp processor(process, fields, source) when is_function(process, 3) do
&process_row(&1, process, fields, sources)
end
defp processor(process, _fields, _source) when is_function(process, 1) do
process
end
defp processor(nil, _fields, _source) do
nil
end
![sourcelevel-bot[bot]](https://avatars.githubusercontent.com/in/597?s=60&v=4){kind=small}
| type = | ||
| # TODO: Support the :number type |
lib/ecto/schema.ex
Outdated
| @@ -1405,6 +1405,9 @@ defmodule Ecto.Schema do | |||
| defp type_to_module(:utc_datetime), do: DateTime | |||
| defp type_to_module(other), do: other | |||
|
|
|||
| # TODO: Remove one more variations of this. | |||
| # TODO: Consider making :types a list once all is done. | |||
lib/ecto/schema.ex
Outdated
| @@ -1405,6 +1405,9 @@ defmodule Ecto.Schema do | |||
| defp type_to_module(:utc_datetime), do: DateTime | |||
| defp type_to_module(other), do: other | |||
|
|
|||
| # TODO: Remove one more variations of this. | |||
| end | ||
|
|
||
| defp take!(source, query, fetched, field) do | ||
| defp take!(source, query, fetched, ix, field) do |
There was a problem hiding this comment.
Function is too complex (CC is 13, max is 9).
| {{:value, :map}, [{:&, [], [ix]}]} | ||
|
|
||
| {:error, {_, schema}} -> | ||
| types = schema.__schema__(:types) |> Enum.to_list() |
There was a problem hiding this comment.
Use a function call when a pipeline is only one function long
|
Ebert has finished reviewing this Pull Request and has found:
You can see more details about this review at https://ebertapp.io/github/elixir-ecto/ecto/pulls/2155. |
This reduces the complexity in the adapters, reduces the caching cost per entry and speeds up deserialization.
josevalim
force-pushed
the
jv-expand-select
branch
from
598393a to
caa84f3
Compare
|
❤️ 💚 💙 💛 💜 |
|
Hi @shdblowers, @jbachhardie, @AndrewDryga, @scouten! Just a heads up that Ecto 2.2 will be released tomorrow (this monday). This issue should cover the new formats added to the query AST. I have also invited all of you to the @elixir-ecto/adapters team so it is easier to discuss such changes in the future. |
|
Hello, @josevalim. I will try to find some time over the next week or two to update Mensia adapter and make it compatible with Ecto 2.2. Thanks for this changes. I really feel that there is a huge space for improvements in Adapter behavior to make it easy to build new ones. |
This reduces the complexity in the adapters, reduces
the caching cost per entry and speeds up deserialization.