Repository: elixir-ecto/ecto_sql
Pull request #657 (2025-03-24 20:49:54)

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.

Munksgaard: Some of the tests seemed a bit flaky when I tried to run them locally, but the new ones I wrote works for me. It doesn't look like indexes are tested in our integration tests, so I haven't added anything there. The generated postgres queries look correct, but I'd greatly appreciate it if someone with more experience in MySQL and MSSQL could tell me if the queries for those databases are also correct.
Munksgaard: Oh, and I should probably say that, whether or not to actually include this in ecto_sql is still up for discussion. I think it is useful, but maybe others disagree. This PR was mostly to figure out how intrusive it would be in the implementation.
josevalim: I am happy with this addition. @greg-rychlewski, feel free to merge it if you are happy too.

Patch:
diff --git a/lib/ecto/adapters/myxql/connection.ex b/lib/ecto/adapters/myxql/connection.ex
index 61e2f40..43aaf2b 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 13cc10e..9a7c478 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 deb6a89..852e1ef 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 8073bd1..e26d8f4 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.
 


Test patch:
diff --git a/test/ecto/adapters/myxql_test.exs b/test/ecto/adapters/myxql_test.exs
index b884645..1989daa 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 9bb21a6..90dbc49 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 c7f402b..61a8d3e 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 fb251ee..392048c 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()