From 79c21dd06cdb073d3f576c06c519d012996d3c2d Mon Sep 17 00:00:00 2001 From: Philip Munksgaard Date: Mon, 24 Mar 2025 21:20:18 +0100 Subject: [PATCH] Add support for index directions As discussed [on the mailing list], this PR add support for specifying the index direction when creating a new index. [on the mailing list]: https://groups.google.com/g/elixir-ecto/c/uYFE2evwsW4/m/2q5QwQH2BAAJ Most of the changes are pretty straightforward, but in the Tds and Postgres adapters, I did have to split up the old `index_expr/1` into two different functions: one to compute the index expression and one to compute the include expression. Previously they shared the same function, but now that the domains of the functions are different I don't think they should. I also added two more types to `Ecto.Migration` to make sure the type of the `columns` field is correct. Finally, I chose to ignore the sorting direction when generating the index name, so that functionality also had to be slightly amended. --- lib/ecto/adapters/myxql/connection.ex | 23 +++++++++++ lib/ecto/adapters/postgres/connection.ex | 33 +++++++++++++++- lib/ecto/adapters/tds/connection.ex | 28 +++++++++++++- lib/ecto/migration.ex | 49 +++++++++++++++++++----- test/ecto/adapters/myxql_test.exs | 19 +++++++++ test/ecto/adapters/postgres_test.exs | 10 +++++ test/ecto/adapters/tds_test.exs | 21 ++++++++++ test/ecto/migration_test.exs | 6 +++ 8 files changed, 177 insertions(+), 12 deletions(-) diff --git a/lib/ecto/adapters/myxql/connection.ex b/lib/ecto/adapters/myxql/connection.ex index 61e2f40c2..43aaf2b80 100644 --- a/lib/ecto/adapters/myxql/connection.ex +++ b/lib/ecto/adapters/myxql/connection.ex @@ -1312,11 +1312,34 @@ if Code.ensure_loaded?(MyXQL) do defp default_expr(:error), do: [] + defp index_expr({dir, literal}) + when is_binary(literal), + do: index_dir(dir, literal) + + defp index_expr({dir, literal}), + do: index_dir(dir, quote_name(literal)) + defp index_expr(literal) when is_binary(literal), do: literal defp index_expr(literal), do: quote_name(literal) + defp index_dir(dir, str) + when dir in [ + :asc, + :asc_nulls_first, + :asc_nulls_last, + :desc, + :desc_nulls_first, + :desc_nulls_last + ] do + case dir do + :asc -> [str | " ASC"] + :desc -> [str | " DESC"] + _ -> error!(nil, "#{dir} is not supported in indexes in MySQL") + end + end + defp engine_expr(storage_engine), do: [" ENGINE = ", String.upcase(to_string(storage_engine || "INNODB"))] diff --git a/lib/ecto/adapters/postgres/connection.ex b/lib/ecto/adapters/postgres/connection.ex index 13cc10e50..9a7c47836 100644 --- a/lib/ecto/adapters/postgres/connection.ex +++ b/lib/ecto/adapters/postgres/connection.ex @@ -1305,7 +1305,7 @@ if Code.ensure_loaded?(Postgrex) do def execute_ddl({command, %Index{} = index}) when command in @creates do fields = Enum.map_intersperse(index.columns, ", ", &index_expr/1) - include_fields = Enum.map_intersperse(index.include, ", ", &index_expr/1) + include_fields = Enum.map_intersperse(index.include, ", ", &include_expr/1) maybe_nulls_distinct = case index.nulls_distinct do @@ -1704,12 +1704,43 @@ if Code.ensure_loaded?(Postgrex) do ":default may be a string, number, boolean, list of strings, list of integers, map (when type is Map), or a fragment(...)" ) + defp index_expr({dir, literal}) when is_binary(literal), + do: index_dir(dir, literal) + + defp index_expr({dir, literal}), + do: index_dir(dir, quote_name(literal)) + defp index_expr(literal) when is_binary(literal), do: literal defp index_expr(literal), do: quote_name(literal) + defp index_dir(dir, str) + when dir in [ + :asc, + :asc_nulls_first, + :asc_nulls_last, + :desc, + :desc_nulls_first, + :desc_nulls_last + ] do + case dir do + :asc -> [str | " ASC"] + :asc_nulls_first -> [str | " ASC NULLS FIRST"] + :asc_nulls_last -> [str | " ASC NULLS LAST"] + :desc -> [str | " DESC"] + :desc_nulls_first -> [str | " DESC NULLS FIRST"] + :desc_nulls_last -> [str | " DESC NULLS LAST"] + end + end + + defp include_expr(literal) when is_binary(literal), + do: literal + + defp include_expr(literal), + do: quote_name(literal) + defp options_expr(nil), do: [] diff --git a/lib/ecto/adapters/tds/connection.ex b/lib/ecto/adapters/tds/connection.ex index deb6a897e..852e1ef37 100644 --- a/lib/ecto/adapters/tds/connection.ex +++ b/lib/ecto/adapters/tds/connection.ex @@ -1196,7 +1196,7 @@ if Code.ensure_loaded?(Tds) do include = index.include |> List.wrap() - |> Enum.map_intersperse(", ", &index_expr/1) + |> Enum.map_intersperse(", ", &include_expr/1) [ [ @@ -1570,9 +1570,35 @@ if Code.ensure_loaded?(Tds) do defp constraint_name(type, table, name), do: quote_name("#{type}_#{table.prefix}_#{table.name}_#{name}") + defp index_expr({dir, literal}) + when is_binary(literal), + do: index_dir(dir, literal) + + defp index_expr({dir, literal}), + do: index_dir(dir, quote_name(literal)) + defp index_expr(literal) when is_binary(literal), do: literal defp index_expr(literal), do: quote_name(literal) + defp index_dir(dir, str) + when dir in [ + :asc, + :asc_nulls_first, + :asc_nulls_last, + :desc, + :desc_nulls_first, + :desc_nulls_last + ] do + case dir do + :asc -> [str | " ASC"] + :desc -> [str | " DESC"] + _ -> error!(nil, "#{dir} is not supported in indexes in Tds adapter") + end + end + + defp include_expr(literal) when is_binary(literal), do: literal + defp include_expr(literal), do: quote_name(literal) + defp engine_expr(_storage_engine), do: [""] defp options_expr(nil), do: [] diff --git a/lib/ecto/migration.ex b/lib/ecto/migration.ex index 8073bd1e2..e26d8f495 100644 --- a/lib/ecto/migration.ex +++ b/lib/ecto/migration.ex @@ -393,11 +393,21 @@ defmodule Ecto.Migration do comment: nil, options: nil + @type column :: atom | String.t() | {index_dir(), atom | String.t()} + + @type index_dir :: + :asc + | :asc_nulls_first + | :asc_nulls_last + | :desc + | :desc_nulls_first + | :desc_nulls_last + @type t :: %__MODULE__{ table: String.t(), prefix: String.t() | nil, name: String.t() | atom, - columns: [atom | String.t()], + columns: [column()], unique: boolean, concurrently: boolean, using: atom | String.t(), @@ -891,6 +901,24 @@ defmodule Ecto.Migration do create index("products", [:sku, :category_id], unique: true) create index("products", [:sku], unique: true, where: "category_id IS NULL") + ## Sorting direction + + You can specify the sorting direction of the index by using a keyword list: + + create index("products", [desc: sku]) + + The following keywords are supported: + + * `:asc` + * `:asc_nulls_last` + * `:asc_nulls_first` + * `:desc` + * `:desc_nulls_last` + * `:desc_nulls_first` + + The `*_nulls_first` and `*_nulls_last` variants are not supported by all + databases. + ## Examples # With no name provided, the name of the below index defaults to @@ -957,18 +985,19 @@ defmodule Ecto.Migration do defp default_index_name(index) do [index.table, index.columns, "index"] |> List.flatten() - |> Enum.map_join( - "_", - fn item -> - item - |> to_string() - |> String.replace(~r"[^\w]", "_") - |> String.replace_trailing("_", "") - end - ) + |> Enum.map_join("_", &column_name/1) |> String.to_atom() end + defp column_name({_dir, column}), do: column_name(column) + + defp column_name(column) do + column + |> to_string() + |> String.replace(~r"[^\w]", "_") + |> String.replace_trailing("_", "") + end + @doc """ Executes arbitrary SQL, anonymous function or a keyword command. diff --git a/test/ecto/adapters/myxql_test.exs b/test/ecto/adapters/myxql_test.exs index b88464549..1989daa34 100644 --- a/test/ecto/adapters/myxql_test.exs +++ b/test/ecto/adapters/myxql_test.exs @@ -2147,6 +2147,25 @@ defmodule Ecto.Adapters.MyXQLTest do ] end + test "create index with direction" do + create = + {:create, index(:posts, [:category_id, desc: :permalink])} + + assert execute_ddl(create) == + [ + ~s|CREATE INDEX `posts_category_id_permalink_index` ON `posts` (`category_id`, `permalink` DESC)| + ] + end + + test "create index with invalid direction" do + create = + {:create, index(:posts, [:category_id, asc_nulls_first: :permalink])} + + assert_raise ArgumentError, "asc_nulls_first is not supported in indexes in MySQL", fn -> + execute_ddl(create) + end + end + test "create unique index" do create = {:create, index(:posts, [:permalink], unique: true)} diff --git a/test/ecto/adapters/postgres_test.exs b/test/ecto/adapters/postgres_test.exs index 9bb21a610..90dbc4950 100644 --- a/test/ecto/adapters/postgres_test.exs +++ b/test/ecto/adapters/postgres_test.exs @@ -2700,6 +2700,16 @@ defmodule Ecto.Adapters.PostgresTest do ] end + test "create index with direction" do + create = + {:create, index(:posts, [:category_id, desc_nulls_last: :permalink])} + + assert execute_ddl(create) == + [ + ~s|CREATE INDEX "posts_category_id_permalink_index" ON "posts" ("category_id", "permalink" DESC NULLS LAST)| + ] + end + test "create unique index" do create = {:create, index(:posts, [:permalink], unique: true)} diff --git a/test/ecto/adapters/tds_test.exs b/test/ecto/adapters/tds_test.exs index c7f402bce..61a8d3e7f 100644 --- a/test/ecto/adapters/tds_test.exs +++ b/test/ecto/adapters/tds_test.exs @@ -1856,6 +1856,27 @@ defmodule Ecto.Adapters.TdsTest do [~s|CREATE INDEX [posts$main] ON [posts] ([permalink]) WITH(ONLINE=ON);|] end + test "create index with direction" do + create = + {:create, index(:posts, [:category_id, desc: :permalink])} + + assert execute_ddl(create) == + [ + ~s|CREATE INDEX [posts_category_id_permalink_index] ON [posts] ([category_id], [permalink] DESC);| + ] + end + + test "create index with invalid direction" do + create = + {:create, index(:posts, [:category_id, asc_nulls_first: :permalink])} + + assert_raise ArgumentError, + "asc_nulls_first is not supported in indexes in Tds adapter", + fn -> + execute_ddl(create) + end + end + test "create unique index" do create = {:create, index(:posts, [:permalink], unique: true)} diff --git a/test/ecto/migration_test.exs b/test/ecto/migration_test.exs index fb251ee2b..392048c55 100644 --- a/test/ecto/migration_test.exs +++ b/test/ecto/migration_test.exs @@ -592,6 +592,12 @@ defmodule Ecto.MigrationTest do assert {:create, %Index{}} = last_command() end + test "creates an index with desc_nulls_last" do + create index(:posts, desc_nulls_last: :title) + flush() + assert {:create, %Index{}} = last_command() + end + test "creates a check constraint" do create constraint(:posts, :price, check: "price > 0") flush()