DateTime passed into query producing incorrect results

[tomtaylor]

I've got a table that looks like this:

CREATE TABLE IF NOT EXISTS "things" ("id" INTEGER PRIMARY KEY, "thing_id" INTEGER NOT NULL, "user_id" INTEGER NOT NULL, "timestamp" TEXT_DATETIME NOT NULL);

With data that looks like:

1|1|2|2016-03-09 23:26:46
2|2|3|2016-03-11 18:56:01
3|3|2|2016-03-08 15:24:38
4|4|2|2016-03-08 14:23:26
5|5|2|2016-03-08 10:02:39
6|6|2|2016-03-07 20:18:08
7|7|2|2016-03-02 15:30:22
8|8|2|2016-03-01 13:59:31
9|9|2|2016-02-25 19:02:16
10|10|2|2016-02-24 17:49:14
...

And a query that's very similar to this:

Thing
|> select([f], {min(f.timestamp), f.thing_id})
|> where([f], f.user_id in ^user_ids)
|> where([f], fragment("(?, ?) < (?, ?)", f.timestamp, f.thing_id, ^since_timestamp, ^since_thing_id))
|> group_by([f], f.thing_id)
|> order_by([f], desc: min(f.timestamp), desc: f.thing_id)
|> limit(^count)

I've been tearing my hair out trying to work out why it works fine in my SQLite console, but through Ecto it doesn't seem to produce the right results - the dates of the result are incorrect. I was passing a DateTime in as since_timestamp, but I've discovered the query only works correctly if I pass in the same thing in as an ISO8601 String. So "2022-05-29 06:34:29" instead of ~U[2022-05-29 06:34:29Z].

I guessing this is something to do with how a DateTime is cast into a string into Ecto, which is different to the string format in the database. It's really difficult to debug, because I can't see exactly what query is being produced.

Let me know if I can provide any more information or a test case.

[ruslandoga]

One possible workaround is to use datetime(?) on the passed timestamp in the fragment.

|> where([t], fragment("(?, ?) < (datetime(?), ?)", t.timestamp, t.thing_id, ^since_timestamp, ^since_thing_id))

[warmwaffles]

@tomtaylor also can you throw IO.inspect(Ecto.Adapters.SQL.to_sql(:all, Repo, queryable)) into your query chain and post back the generated query?

[ruslandoga]

@warmwaffles here's the simplest query that returns incorrect result on that data.

github.com/ruslandoga/issue74

iex> since_timestamp = ~U[2016-02-25 19:02:15Z]
iex> import Ecto.Query
iex> Thing |> where([t], t.timestamp < ^since_timestamp) |> Repo.all()

# [debug] QUERY OK source="things" db=0.5ms queue=0.1ms idle=1714.0ms
# SELECT t0."id", t0."thing_id", t0."user_id", t0."timestamp" FROM things AS t0 WHERE (t0."timestamp" < ?) ["2016-02-25T19:02:15"]

[
  %Thing{
    __meta__: #Ecto.Schema.Metadata<:loaded, "things">,
    id: 9,
    thing_id: 9,
    timestamp: ~U[2016-02-25 19:02:16Z], # shouldn't have been returned
    user_id: 2
  },
  %Thing{
    __meta__: #Ecto.Schema.Metadata<:loaded, "things">,
    id: 10,
    thing_id: 10,
    timestamp: ~U[2016-02-24 17:49:14Z],
    user_id: 2
  }
]

# it comes down to this for SQLite, unless extra info is provided
iex> "2016-02-25 19:02:16" < "2016-02-25T19:02:15Z"
true

-- SQLite version 3.38.5 2022-05-06 15:25:27
sqlite> SELECT * FROM things WHERE (timestamp < '2016-02-25T19:02:15');
┌────┬──────────┬─────────┬─────────────────────┐
│ id │ thing_id │ user_id │      timestamp      │
├────┼──────────┼─────────┼─────────────────────┤
│ 9  │ 9        │ 2       │ 2016-02-25 19:02:16 │
│ 10 │ 10       │ 2       │ 2016-02-24 17:49:14 │
└────┴──────────┴─────────┴─────────────────────┘
sqlite> SELECT * FROM things WHERE (timestamp < cast('2016-02-25T19:02:15' as text_datetime));
┌────┬──────────┬─────────┬─────────────────────┐
│ id │ thing_id │ user_id │      timestamp      │
├────┼──────────┼─────────┼─────────────────────┤
│ 9  │ 9        │ 2       │ 2016-02-25 19:02:16 │
│ 10 │ 10       │ 2       │ 2016-02-24 17:49:14 │
└────┴──────────┴─────────┴─────────────────────┘
sqlite> SELECT * FROM things WHERE (timestamp < datetime('2016-02-25T19:02:15'));
┌────┬──────────┬─────────┬─────────────────────┐
│ id │ thing_id │ user_id │      timestamp      │
├────┼──────────┼─────────┼─────────────────────┤
│ 10 │ 10       │ 2       │ 2016-02-24 17:49:14 │
└────┴──────────┴─────────┴─────────────────────┘
sqlite> SELECT * FROM things WHERE (timestamp < '2016-02-25 19:02:15');
┌────┬──────────┬─────────┬─────────────────────┐
│ id │ thing_id │ user_id │      timestamp      │
├────┼──────────┼─────────┼─────────────────────┤
│ 10 │ 10       │ 2       │ 2016-02-24 17:49:14 │
└────┴──────────┴─────────┴─────────────────────┘

[warmwaffles]

I'll take a look into this soon unless @ruslandoga you find a solution before I do.

[ruslandoga]

@warmwaffles I don't think there is any fix due to SQLite's lack of real date/time types. As far as I can see, the best thing Ecto can do is cast the input, but SQLite seems to ignore it.

sqlite> SELECT * FROM things WHERE (timestamp < cast('2016-02-25T19:02:15' as text_datetime));
┌────┬──────────┬─────────┬─────────────────────┐
│ id │ thing_id │ user_id │      timestamp      │
├────┼──────────┼─────────┼─────────────────────┤
│ 9  │ 9        │ 2       │ 2016-02-25 19:02:16 │
│ 10 │ 10       │ 2       │ 2016-02-24 17:49:14 │
└────┴──────────┴─────────┴─────────────────────┘

And replacing any timestamp with a function call like datetime(<timestamp>) seems like a hacky way to hide SQLite's shortcomings.

Related: https://sqlite.org/forum/forumpost/9e5b6175fc702f4a

[warmwaffles]

seems like a hacky way to hide SQLite's shortcomings.

Heh, I don't think it is so much a short coming but a deliberate decision to leave that to the application level.

It also doesn't help that we use text_datetime here instead of just datetime IIRC I chose to use text_datetime because the old esqlite + ecto2 adapter used it and I wanted to make sure others could transition easily.

[tomtaylor]

Thanks for everyone's work looking at this, and for providing a minimal test case.

I feel like the current behaviour is a little surprising. While SQLite has no native datetime format, the implication of the datetime function is that datetimes are best stored as YYYY-MM-DD HH:MM:SS, not YYYY-MM-DDTHH:MM:SSZ.

Is this something we could make configurable? Perhaps defaulting to YYYY-MM-DD HH:MM:SS with ISO8601 configurable as an option? I imagine most databases use an internally consistent format, so making this configurable at the library level might be reasonable for most users.

[warmwaffles]

Is this something we could make configurable?

A PR for the change would be welcome.

We simply defaulted to DateTime.to_iso8601/1 for serializing to the database because it was simplest and most of sqlite's datetime https://www.sqlite.org/lang_datefunc.html expects the datetime to be in a subset of ISO-8601.

[greg-rychlewski]

@tomtaylor you can give the changes in this PR a try: #84

[warmwaffles]

Lemme get a release cut

[warmwaffles]

The recent set of changes has been released under v0.8.0