Bug on sqlite __year comparison

[Manuel Saelices <msaelices@…>]

This code:

  >>> from myapp.models import FooModel
  >>> FooModel.objects.filter(pub_date__year=2007)

produces this SQL:

SELECT "myapp_foomodel"."id", ... FROM "myapp_foomodel" WHERE ("myapp_foomodel"."pub_date" BETWEEN '2006-01-01 00:00:00' AND '2006-12-31 23:59:59.999999');

but I think sqlite doesn't support this notation (at least in sqlite 3.3.5). It prefers something like that:

SELECT "myapp_foomodel"."id", ... FROM "myapp_foomodel" WHERE ("myapp_foomodel"."pub_date" BETWEEN '2006-01-01' AND '2006-12-31');

I've attached a patch that fixes this bug.

[Manuel Saelices <msaelices@…>]

A patch that fixes sqlite year lookup

[jarek.zgoda@…]

Shouldn't the format modifier after converting to int (line 512) to be %d, not %s on line 515?

[Manuel Saelices <msaelices@…>]

Replying to jarek.zgoda@gmail.com:

Shouldn't the format modifier after converting to int (line 512) to be %d, not %s on line 515?

Maybe, but I cut code from base classes, and I think it was for a reason

[Jarek Zgoda <jarek.zgoda@…>]

For no apparent reason.
Python does str() on the int object in such case so this doesn't end with ValueError.

[Fredrik Lundh <fredrik@…>]

Pulling this back; not clear if we can repeat this without the patch on current trunk, really.

[Fredrik Lundh <fredrik@…>]

msaelices, it looks as if your suggested patch breaks the standard test suite under sqlite 3.4.X, in the current trunk. I cannot seem to repeat that with sqlite 3.3.X (I've tested both 3.3.4 and 3.3.7), but I cannot seem to repeat your bug either :-(

If you still have access to 3.3.5, can you check what the standard "basic" test, if used with and without your patch:

./runtests.py --settings ... basic

[anonymous]

Ok I'll try with sqlite 3.3.5

[Manuel Saelices]

I run all tests with sqlite 3.3.5 with this command:

  ./runtests.py

The result is patch doesn't break any tests. There are seven tests failed but no from django.db.

[Malcolm Tredinnick]

Firstly, there should be no tests failing. Otherwise you might be having other problems.

Secondly, do any extra tests (particularly the "basic" one -- just run that on its own as Fredrik indicated) fail when you do not have the patch in place?

The problem we are trying to work through is that without this patch, we can't get the existing tests to fail on the SQLite version Fredrik and I have tried, so it isn't clear that this is fixing a real problem. It may turn out to be tied to some particular bug in 3.3.5, in which case that's going to be hard cheese for users of 3.3.5. Or it might be something else, but right now, we don't have a clear demonstration of something that fails before the patch is applied and then starts working afterwards.

[anonymous]

Replying to mtredinnick:

Firstly, there should be no tests failing. Otherwise you might be having other problems.

Ok. I consider it's a Django problem, because many failed tests refers (I think) to have differents locales that people that usually runs all tests (things like Expected: July, Got: Julio. Other fails test refer to new file session backend, and I've considered as usual in a sprint like this, with many changes in a little time.

It's another question, but I attached this tests report​ on this ticket.

Secondly, do any extra tests (particularly the "basic" one -- just run that on its own as Fredrik indicated) fail when you do not have the patch in place?

Here I know that you said to locate patch lines in another situation Field.get_db_prep_lookup. But, I thought previous comments of failing tests was made with my patch and possibly location doesn't matter. Sorry If I was wrong.

The problem we are trying to work through is that without this patch, we can't get the existing tests to fail on the SQLite version Fredrik and I have tried, so it isn't clear that this is fixing a real problem. It may turn out to be tied to some particular bug in 3.3.5, in which case that's going to be hard cheese for users of 3.3.5. Or it might be something else, but right now, we don't have a clear demonstration of something that fails before the patch is applied and then starts working afterwards.

[Manuel Saelices]

Sorry, I havent logged before posting my comment

Replying to anonymous:

Replying to mtredinnick:

Firstly, there should be no tests failing. Otherwise you might be having other problems.

Ok. I consider it's a Django problem, because many failed tests refers (I think) to have differents locales that people that usually runs all tests (things like Expected: July, Got: Julio. Other fails test refer to new file session backend, and I've considered as usual in a sprint like this, with many changes in a little time.

It's another question, but I attached this tests report​ on this ticket.

Secondly, do any extra tests (particularly the "basic" one -- just run that on its own as Fredrik indicated) fail when you do not have the patch in place?

Here I know that you said to locate patch lines in another situation Field.get_db_prep_lookup. But, I thought previous comments of failing tests was made with my patch and possibly location doesn't matter. Sorry If I was wrong.

The problem we are trying to work through is that without this patch, we can't get the existing tests to fail on the SQLite version Fredrik and I have tried, so it isn't clear that this is fixing a real problem. It may turn out to be tied to some particular bug in 3.3.5, in which case that's going to be hard cheese for users of 3.3.5. Or it might be something else, but right now, we don't have a clear demonstration of something that fails before the patch is applied and then starts working afterwards.

[raminf]

I started working on this during the sprint and was trying to be more conservative to avoid problems with other backends. Thought I'd go ahead and toss it in as an alternative.

[Doug Van Horn]

Replying to mtredinnick:

Firstly, there should be no tests failing. Otherwise you might be having other problems.

Secondly, do any extra tests (particularly the "basic" one -- just run that on its own as Fredrik indicated) fail when you do not have the patch in place?

The problem we are trying to work through is that without this patch, we can't get the existing tests to fail on the SQLite version Fredrik and I have tried, so it isn't clear that this is fixing a real problem. It may turn out to be tied to some particular bug in 3.3.5, in which case that's going to be hard cheese for users of 3.3.5. Or it might be something else, but right now, we don't have a clear demonstration of something that fails before the patch is applied and then starts working afterwards.

I posted a duplicate of this problem and my own patches at #6141. The issue here is that when you store a date in SQLite, it's being stored as a string:

sqlite> create table foo( d date null );
sqlite> insert into foo(d) values( '2008-01-01' );
sqlite> select d, typeof(d) from foo;
2008-01-01|text

See the ​SQLite documentation and ​my post to the SQLite forum.

Since dates are stored as text in SQLite and '2008-01-01' is lexigraphicly less than '2008-01-01 00:00:00', the record in question falls outside the bounds of the query.

No tests are failing because they're not testing the problem, which lies with 'date' columns in SQLite, not datetime columns. I'm attaching a patch call test.diff that will expose the problem when run against SQLite. It patches tests/modeltests/basic/models.py, adding a class and a test at the bottom of the file.

Hope this helps clarify why this is a problem on SQLite.

[Doug Van Horn]

Adds a test to identify the problem.

[Malcolm Tredinnick]

(In [7150]) Fixed #3689, #5223 -- Fixed "1st of January" comparisons for SQLite without breaking the other backends.

Based on a patch from raminf and tests from Nebojsa Djordjevic.