Code

Opened 7 years ago

Closed 6 years ago

#5223 closed (fixed)

Bug on sqlite __year comparison

Reported by: Manuel Saelices <msaelices@…> Owned by: nobody
Component: Database layer (models, ORM) Version: master
Severity: Keywords: sqlite orm sq-rf sprintsept14
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: no
Easy pickings: UI/UX:

Description

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.

Attachments (4)

sqlite_year_lookup_fix.diff (752 bytes) - added by Manuel Saelices <msaelices@…> 7 years ago.
A patch that fixes sqlite year lookup
tests_report_07_09_16.log (203.9 KB) - added by msaelices 7 years ago.
sqlite_year.diff (1.3 KB) - added by raminf 7 years ago.
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.
test.diff (1.1 KB) - added by Doug Van Horn 6 years ago.
Adds a test to identify the problem.

Download all attachments as: .zip

Change History (20)

Changed 7 years ago by Manuel Saelices <msaelices@…>

A patch that fixes sqlite year lookup

comment:1 Changed 7 years ago by mtredinnick

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Summary changed from [patch] Bug on sqlite __year comparison to Bug on sqlite __year comparison

comment:2 Changed 7 years ago by SmileyChris

  • Needs tests set
  • Triage Stage changed from Unreviewed to Accepted

comment:3 follow-up: Changed 7 years ago by jarek.zgoda@…

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

comment:4 in reply to: ↑ 3 Changed 7 years ago by 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

comment:5 Changed 7 years ago by 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.

comment:6 Changed 7 years ago by mtredinnick

  • Keywords orm, sq-rf added; orm removed

comment:7 Changed 7 years ago by Fredrik Lundh <fredrik@…>

  • Keywords orm sprintsept14 added; orm, removed
  • Triage Stage changed from Accepted to Ready for checkin

comment:8 Changed 7 years ago by Fredrik Lundh <fredrik@…>

  • Triage Stage changed from Ready for checkin to Accepted

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

comment:9 Changed 7 years ago by 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

comment:10 Changed 7 years ago by anonymous

Ok I'll try with sqlite 3.3.5

comment:11 Changed 7 years ago by msaelices

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.

comment:12 follow-ups: Changed 7 years ago by 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.

comment:13 in reply to: ↑ 12 ; follow-up: Changed 7 years ago by 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.

comment:14 in reply to: ↑ 13 Changed 7 years ago by msaelices

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.

Changed 7 years ago by msaelices

Changed 7 years ago by 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.

comment:15 in reply to: ↑ 12 Changed 6 years ago by 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.

Changed 6 years ago by Doug Van Horn

Adds a test to identify the problem.

comment:16 Changed 6 years ago by mtredinnick

  • Resolution set to fixed
  • Status changed from new to closed

(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.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.