Issues with parameter handling

[hyphenPaul]

I'm getting different results using variables with Ecto. It seems to be related to the way bound parameters are being used.

Example of a working query without variables for offset and limit values:

iex(36)> (from x in RPCoreModel.Store, select: x.store_code) |> offset(3) |>  limit(1) |> Repo.all()
[debug] QUERY OK source="store" db=49.8ms queue=0.1ms idle=1621.7ms
SELECT s0.store_code FROM store s0 OFFSET 3 ROWS FETCH NEXT 1 ROWS ONLY []
["SEL"]

The results are correct, returning the 1 value passed to the limit function.

If I try to use variables with the same code I get different results:

iex(37)> offset = 3
3
iex(38)> limit = 1
1
iex(39)> (from x in RPCoreModel.Store, select: x.store_code) |> offset(^offset) |>  limit(^limit) |> Repo.all()
[debug] QUERY OK source="store" db=46.6ms idle=1180.6ms
SELECT s0.store_code FROM store s0 OFFSET :2 ROWS FETCH NEXT :1 ROWS ONLY [1, 3]
["001", "CMP", "SEL"]

It seems like the bound parameters aren't working as expected. I don't have any Oracle experience, but I do use Ecto quite often. Please let me know if you'd like me to debug some more. Thanks!

[hyphenPaul]

I found the issue and it will most likely need an Ecto update. Because Oracle and other SQL languages differ in their implementation of offset and limit syntax, a change needs to be made in Ecto. In Postgres for example, the limit call comes before offset. In short, limit and offset need to be swapped like this: https://github.com/Comoto-Tech/ecto/commit/92c370b32842b05629d905c10359dadd6ecd4e20

I'll create an issue with Ecto, as they'll need to expose this module attribute to the adapter so it can be overridden.

[vstavskyi]

Maybe fragment could be used limit(fragment("OFFSET ? ROWS ?",3,1))

[vstavskyi]

One function for limit and offset doesn't work also. Offset value always is last in parameters list.

  defp limit(%{limit: nil, offset: nil}, _sources), do: []
  defp limit(%{limit: nil, offset: %QueryExpr{expr: expr}} = query, sources) do
    [" OFFSET ", expr(expr, sources, query), " ROWS"]
  end
  defp limit(%{limit: %QueryExpr{expr: expr}, offset: nil} = query, sources) do
    [" FETCH NEXT ", expr(expr, sources, query), " ROWS ONLY"]
  end
  defp limit(%{limit: %QueryExpr{expr: expr_last}, offset: %QueryExpr{expr: expr}} = query, sources) do
    [" OFFSET ", expr(expr_last, sources, query), " ROWS", " FETCH NEXT ", expr(expr, sources, query), " ROWS ONLY"]
  end

[hyphenPaul]

That's exactly what I noticed. It comes down to the order of the parameter list, which happens in this private function using the @all_expr module attribute.

https://github.com/elixir-ecto/ecto/blob/master/lib/ecto/query/planner.ex#L1807-L1820

  @all_exprs [with_cte: :with_ctes, distinct: :distinct, select: :select, from: :from, join: :joins,
              where: :wheres, group_by: :group_bys, having: :havings, windows: :windows,
              combination: :combinations, order_by: :order_bys, limit: :limit, offset: :offset]

  defp traverse_exprs(query, operation, acc, fun) do
    exprs =
      case operation do
        :all -> @all_exprs
        :insert_all -> @all_exprs
        :update_all -> @update_all_exprs
        :delete_all -> @delete_all_exprs
      end

    Enum.reduce exprs, {query, acc}, fn {kind, key}, {query, acc} ->
      {traversed, acc} = fun.(kind, query, Map.fetch!(query, key), acc)
      {%{query | key => traversed}, acc}
    end
  end

Maybe there's another place to handle this downstream in the adapter, but I couldn't find anything.

[vstavskyi]

Named parameters handling in raw sql statements now in stage

 params = %Ecto.Query.Tagged{value: %{offset: 1, limit: 2}, type: :map}

 Ecto.Adapters.SQL.query(YourApp.Repo, "SELECT store_code FROM store OFFSET :offset ROWS FETCH NEXT :limit ROWS ONLY ", [params])

[hyphenPaul]

Thanks @vstavskyi, I'll take a look. Is the expectation that I should be able to reorder the parameters in one of the adapter hooks?

[vstavskyi]

You could use raw sql instead of ecto syntax from the beginning. Named parameters are just a bonus.

[hyphenPaul]

Sorry @vstavskyi, I've been busy this week and I haven't had a chance to take a look outside of testing limit and offset, which I'm still seeing the same issue. Are you suggesting using raw SQL instead of the Ecto syntax for limit and offset?

[vstavskyi]

Yes, raw sql. Or ecto syntax, but limit must be constant value, not variable.

[greg-rychlewski]

Are you sure there is not a problem with whatever is handling the SQL text generated by Ecto? This is the query causing issues:

SELECT s0.store_code FROM store s0 OFFSET :2 ROWS FETCH NEXT :1 ROWS ONLY [1, 3]

If you substitute the parameters it is OFFSET 3 ROWS FTCH NEXT 1 ROWS ONLY. If the result is having 3 rows it seems like it's not parsing the SQL text correctly? Maybe it is processing the parameters in the order the appear as opposed to their index?

[vstavskyi]

in ecto L1789

@all_exprs [with_cte: :with_ctes, distinct: :distinct, select: :select, from: :from, join: :joins, where: :wheres, group_by: :group_bys, having: :havings, windows: :windows, combination: :combinations, order_by: :order_bys, limit: :limit, offset: :offset]

limit and offset should be reversed for Oracle

@all_exprs [with_cte: :with_ctes, distinct: :distinct, select: :select, from: :from, join: :joins, where: :wheres, group_by: :group_bys, having: :havings, windows: :windows, combination: :combinations, order_by: :order_bys, offset: :offset, limit: :limit]

[greg-rychlewski]

But it doesn't have to be reversed in the planner because you know the order they should be right? You have :2, :1, etc...

That tells you the order in the parameter list.

[vstavskyi]

in adapter L32

[cte, select, hints, fields, window, from, join, where, group_by, having, combinations, order_by, offset, limit | lock]

not

[cte, select, hints, fields, window, from, join, where, group_by, having, combinations, order_by, limit,offset | lock]

imho adapter and planner should have the same order

[greg-rychlewski]

If different adapters have different orders (which is the case), then how would it be possible?

I'm not sure if I am making sense. I'll try one more time to ask then bow out :):

If you already have the index of the parameters :1, :2, etc... then you can reorder the parameter list, can't you?

[vstavskyi]

Please try stage branch commit