datetime_add uses incompatible format

[rhcarvalho]

I'm looking at a project (live_beats modified to use SQLite3) with a query like this:

    Ecto.Multi.new()
    |> Ecto.Multi.delete_all(
      :delete_expired_songs,
      from(s in Song,
        where: s.inserted_at < from_now(^(-count), ^to_string(interval)),
        where: s.server_ip == ^server_ip,
        where:
          s.user_id in subquery(
            from(u in Accounts.User, where: u.username not in ^admin_usernames, select: u.id)
          ),
        select: %{user_id: s.user_id, mp3_filepath: s.mp3_filepath}
      )
    )
    |> Ecto.Multi.merge(&update_users_songs_count(&1))
    |> Repo.transaction()

The query was not returning the expected results because the format used to store a datetime in inserted_at is different than the one produced by from_now. The former uses either iso8601 (%Y-%m-%d %H:%M:%f, the default) or text_datetime (%Y-%m-%d %H:%M:%S), while the latter produces %Y-%m-%d %H:%M:%f000Z.

It took me some time to understand where those are coming from and I traced it back to:

https://github.com/elixir-sqlite/ecto_sqlite3/blob/eca01c10b0761b0c89b5f0db7655446d5f644d97/lib/ecto/adapters/sqlite3/connection.ex#L1324-L1334

https://github.com/elixir-sqlite/ecto_sqlite3/blob/eca01c10b0761b0c89b5f0db7655446d5f644d97/lib/ecto/adapters/sqlite3/codec.ex#L110-L136

So I wonder if changing the implementation of expr({:datetime_add, ... to match the configured format would be an acceptable change?

[warmwaffles]

IIRC the %f000Z section is the utc_datetime_usec specification. 😬

[rhcarvalho]

Hey thanks for the feedback. I'm not familiar with that spec (and just learning Ecto), but considering that SQLite3 doesn't have a native storage class for timestamps, it's on user-land code to represent dates in a consistent way.

Is there a different way to write where: s.inserted_at < from_now(^(-count), ^to_string(interval)), such that we're sure that the text format in inserted_at is the same as what is returned from from_now?

I'll try and look at how it is done for the Postgres' Adapter, but guessing there the problem doesn't exist because of native data types.

[warmwaffles]

@rhcarvalho you just need to calculate it in the application layer

since = DateTime.add(DateTime.utc_now(), -count, :seconds)

# ...

where: s.inserted_at < ^since

[rhcarvalho]

Thanks @warmwaffles, that works. Closing this as probably working as intended! Thanks again!

[warmwaffles]

I don't think there is a reason we can't support it. https://sqlite.org/lang_datefunc.html

[iwarshak]

running into this as well. the ago and from_now ecto functions are returning incorrect data

[warmwaffles]

Okay. I will see what I can figure out. @iwarshak can you share the query you are using? And potentially the schema?

[iwarshak]

something like:

from(w in WaterLevel,
      select: [w.inserted_at, w.level, w.confidence],
      order_by: [desc: w.inserted_at],
      where: w.inserted_at >= published_at > ago(3, "month")
    )

my schema was generated from the generators (i.e. using timestamps()and looks like:

CREATE TABLE "water_levels" ("id" INTEGER PRIMARY KEY AUTOINCREMENT, "level" DECIMAL, "sensor" TEXT, "inserted_at" TEXT NOT NULL, "updated_at" TEXT NOT NULL, "error" TEXT, "confidence" DECIMAL);

[warmwaffles]

Okay. I'll plug that in later today and dig into the issue more. The built in date functions in sqlite should get us what we need here.

[warmwaffles]

@iwarshak I can't recreate this. The local schema in the test suite has this

CREATE TABLE IF NOT EXISTS "products" ("id" INTEGER PRIMARY KEY AUTOINCREMENT, "account_id" INTEGER CONSTRAINT "products_account_id_fkey" REFERENCES "accounts"("id"), "name" TEXT, "description" TEXT, "external_id" TEXT, "bid" TEXT, "tags" TEXT, "approved_at" TEXT, "ordered_at" TEXT, "price" DECIMAL, "inserted_at" TEXT NOT NULL, "updated_at" TEXT NOT NULL);

I have a test where I put this in place:

  test "using built in ecto functions" do
    Application.put_env(:ecto_sqlite3, :datetime_type, :text_datetime)

    account = insert_account(%{name: "Test"})

    insert_product(%{
      account_id: account.id,
      name: "Foo",
      inserted_at: days_ago(1)
    })

    insert_product(%{
      account_id: account.id,
      name: "Bar",
      inserted_at: days_ago(2)
    })

    insert_product(%{
      account_id: account.id,
      name: "Qux",
      inserted_at: days_ago(5)
    })

    assert [
             %{name: "Foo"},
             %{name: "Bar"}
           ] =
             Product
             |> select([p], p)
             |> where([p], p.inserted_at >= from_now(-3, "day"))
             |> order_by([p], desc: p.inserted_at)
             |> TestRepo.all()
  end

  defp insert_account(attrs) do
    %Account{}
    |> Account.changeset(attrs)
    |> TestRepo.insert!()
  end

  defp insert_product(attrs) do
    %Product{}
    |> Product.changeset(attrs)
    |> TestRepo.insert!()
  end

  defp days_ago(days) do
    now = DateTime.utc_now()
    DateTime.add(now, -days * 24 * 60 * 60, :second)
  end

The "Qux" product looks like this in the debug

insert_product(%{account_id: account.id, name: "Qux", inserted_at: days_ago(5)}) #=> %EctoSQLite3.Schemas.Product{
  __meta__: #Ecto.Schema.Metadata<:loaded, "products">,
  id: 3,
  name: "Qux",
  description: nil,
  external_id: <<210, 239, 78, 89, 113, 95, 79, 99, 145, 148, 188, 236, 13, 214,
    121, 14>>,
  bid: nil,
  tags: [],
  approved_at: nil,
  ordered_at: nil,
  price: nil,
  account_id: 1,
  account: #Ecto.Association.NotLoaded<association :account is not loaded>,
  inserted_at: ~N[2024-05-10 01:20:04],
  updated_at: ~N[2024-05-15 01:20:04]
}

You can check out the work done here. https://github.com/elixir-sqlite/ecto_sqlite3/tree/try-issue-116

If there is a way you can try to recreate the issue reliably in a test, that would be a tremendous help.

[krwenholz]

Hey @warmwaffles. Thanks for your great work. I have also stumbled on this, but I come bearing a test!

The trick is the T that's included in :iso8601 formatting between the %Y-%m-%d and %H:%M:%S components.

Rewriting your test to focus on the time comparison in seconds, rather than days, demonstrates the issue:

  test "using built in ecto functions" do
    account = insert_account(%{name: "Test"})

    insert_product(%{
      account_id: account.id,
      name: "Foo",
      inserted_at: seconds_ago(1)
    })

    insert_product(%{
      account_id: account.id,
      name: "Bar",
      inserted_at: seconds_ago(3)
    })

    q = 
      Product
      |> select([p], p)
      |> where([p], p.inserted_at >= ago(2, "second"))
      |> order_by([p], desc: p.inserted_at)

    expanded_q =
      Ecto.Adapters.SQL.to_sql(:all, TestRepo, q)
      |> dbg()

    TestRepo.query(elem(expanded_q, 0), elem(expanded_q, 1))
    |> dbg()

    assert [
             %{name: "Foo"},
           ] =
             q
             |> TestRepo.all()
  end

  defp insert_account(attrs) do
    %Account{}
    |> Account.changeset(attrs)
    |> TestRepo.insert!()
  end

  defp insert_product(attrs) do
    %Product{}
    |> Product.changeset(attrs)
    |> TestRepo.insert!()
  end

  defp seconds_ago(seconds) do
    now = DateTime.utc_now()
    DateTime.add(now, -seconds, :second)
  end

Those dbg statements help make it extra clear:

[test/ecto/integration/timestamps_test.exs:207: Ecto.Integration.TimestampsTest."test using built in ecto functions"/1]
Ecto.Adapters.SQL.to_sql(:all, TestRepo, q) #=> {"SELECT p0.\"id\", p0.\"name\", p0.\"description\", p0.\"external_id\", p0.\"bid\", p0.\"tags\", p0.\"approved_at\", p0.\"ordered_at\", p0.\"price\", p0.\"account_id\", p0.\"inserted_at\", p0.\"updated_at\" FROM \"products\" AS p0 WHERE (p0.\"inserted_at\" >= CAST (strftime('%Y-%m-%d %H:%M:%f000Z',?,CAST(-2 AS REAL) || ' second') AS TEXT)) ORDER BY p0.\"inserted_at\" DESC",
[~U[2024-09-04 21:10:49.323351Z]]}

[test/ecto/integration/timestamps_test.exs:210: Ecto.Integration.TimestampsTest."test using built in ecto functions"/1]
TestRepo.query(elem(expanded_q, 0), elem(expanded_q, 1)) #=> {:ok,
%Exqlite.Result{
 command: :execute,
 columns: ["id", "name", "description", "external_id", "bid", "tags",
  "approved_at", "ordered_at", "price", "account_id", "inserted_at",
  "updated_at"],
 rows: [
   [
     1,
     "Foo",
     nil,
     <<176, 205, 18, 155, 220, 11, 69, 162, 178, 242, 198, 35, 11, 42, 162,
       181>>,
     nil,
     "[]",
     nil,
     nil,
     nil,
     1,
     "2024-09-04T21:10:48",
     "2024-09-04T21:10:49"
   ],
   [
     2,
     "Bar",
     nil,
     <<52, 193, 197, 202, 27, 50, 72, 199, 148, 41, 26, 182, 88, 84, 208,
       227>>,
     nil,
     "[]",
     nil,
     nil,
     nil,
     1,
     "2024-09-04T21:10:46",
     "2024-09-04T21:10:49"
   ]
 ],
 num_rows: 2
}}

I've whipped up a potential solution I can submit if it looks ~correct (new to Elixir, would love feedback):

https://github.com/krwenholz/ecto_sqlite3/commit/d45d0b26708c943a979b74b4197e42389c5cc8cf

[warmwaffles]

Yea that solution looks super close to being the solution to use. I believe we'd want to use compile_env in this case

@datetime_type Application.compile_env(:ecto_sqlite3, :datetime_type, :iso8601)

defp expr({:datetime_add, _, [datetime, count, interval]}, sources, query) do
  fmt = Ecto.Adapters.SQLite3.Codec.datetime_format(@datetime_type)

If you want to open a PR that'd be fine. Otherwise I can grab the solution you have and play with it some more. Thank you for reproducing the error.

[warmwaffles]

Fixed under v0.17.2 thanks a ton @krwenholz, I had to alter your solution a bit, it was failing integration tests expecting that microsecond to be returned.

[krwenholz]

Oh cool. Thanks, Matt! Had to step out yesterday, but next time I'll just open the PR 😅

I didn't realize the options were compile time (still wrapping my head around that distinction). The other changes you made make sense. Thanks for getting it merged so quickly!

[warmwaffles]

Options don't need to be compile time, but I don't think users want to alter the adapter at runtime. The trade off is that every time the the datetime_add is invoked, it'll incur a lookup penalty vs simply being inlined by the compiler.

[krwenholz]

Oh duh. That makes sense. Thanks!