Opened 16 years ago

Closed 16 years ago

Last modified 16 years ago

#6161 closed (fixed)

Oracle support for queryset-refactor.

Reported by: jbronn Owned by: Malcolm Tredinnick
Component: Database layer (models, ORM) Version: other branch
Severity: Keywords: qs-rf oracle
Cc: jbronn@…, Erin Kelly Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

I was playing around w/qs-rf branch and successfully merged it w/GeoDjango (there are a few minor tweaks I had make to Query, but otherwise it went well -- I will try and give these diffs to Malcolm later). Because GeoDjango supports Oracle, I decided to try it out, but noticed that the OracleQuerySet had not been modified to be compatible w/queryset-refactor changes.

Attached is the patch I used to enable Oracle functionality under the queryset-refactor branch. Essentially, I added an OracleQuery subclass that has resolve_columns from the original OracleQuerySet and and overrides the as_sql method to deal with the peculiarities of limit/offset SQL in Oracle. The new OracleQuerySet subclass just overrides __init__ to use OracleQuery. There is probably a more elegant way of using qs-rf facilities (e.g., using get_from_clause the way count() does), but it appears to work well enough to pass all GeoDjango tests (not confirmed w/Django tests).

Another change (that has been in GeoDjango for a while) is the modification of the _format_params routine in FormatStylePlaceholderCursor. Because GeoDjango performs spatial queries w/CLOBs (to store WKT geometries), the setinputsizes routine needs to be called. With the patch, _format_params looks to see if the parameter has an oracle_type method, and if it does it will call setinputsizes with the result of oracle_type. For an example of how this is used see the Oracle [browser:django/branches/gis/django/contrib/gis/db/backend/oracle/adaptor.py geometry adaptor class] of GeoDjango.

Attachments (4)

oracle_qsrf_patch.diff (15.3 KB ) - added by jbronn 16 years ago.
Patch enabling Oracle functionality on the queryset-refactor branch.
oracle_qsrf_patch_v2.diff (14.3 KB ) - added by jbronn 16 years ago.
A bit of an improvement; uses qs-rf facilities to be more concise and reduce amount of code.
oracle_qsrf_patch_v3.diff (15.5 KB ) - added by Erin Kelly 16 years ago.
New version of patch that reverts the changes added in [6905]
oracle_qsrf_patch_v4.diff (14.9 KB ) - added by jbronn 16 years ago.
Fixes infinite loop caused by use of default empty_fetchmany_value.

Download all attachments as: .zip

Change History (15)

by jbronn, 16 years ago

Attachment: oracle_qsrf_patch.diff added

Patch enabling Oracle functionality on the queryset-refactor branch.

comment:1 by Malcolm Tredinnick, 16 years ago

Triage Stage: UnreviewedAccepted

Thanks for doing the initial work here, Justin. Looks pretty good. I'll rejig the limit/offset stuff to use a nested query (like COUNT()) and possibly shuffle some things around so that there's almost nothing in the Oracle-specific QuerySet class (everything should be possible in !Query, I suspect), but otherwise this looks fine.

Will be a few days before I commit this, since I'm spending this weekend getting that last "common" bugs nailed and will then port the rest of the code across. Oracle is kind of last, but this will help.

by jbronn, 16 years ago

Attachment: oracle_qsrf_patch_v2.diff added

A bit of an improvement; uses qs-rf facilities to be more concise and reduce amount of code.

comment:2 by Erin Kelly, 16 years ago

Cc: Erin Kelly added

by Erin Kelly, 16 years ago

Attachment: oracle_qsrf_patch_v3.diff added

New version of patch that reverts the changes added in [6905]

comment:3 by Erin Kelly, 16 years ago

[6905] fixed an NCLOB bug by adding calls to setinputsizes. qs-rf already appears to have fixed this in a better way, so I've taken that back out in the patch. I'm having problems testing this, though; I'm running runtests.py basic, and it's just been sitting and spinning for 15 minutes of CPU time.

comment:4 by Malcolm Tredinnick, 16 years ago

Thanks for look at this, Ian.

"Sitting and spinning" sadly isn't something I can debug, however. I'm kind of awake during your mornings at the moment (and on IRC and IM), so grab me if you get a chance and we can bounce some ideas around. I don't have a functional Oracle install at the moment (vmware is disagreeing with my kernel again), so if you do want to pick this up and see what happens, I'm here to help.

One thing I just thought of: the save() path has changed to do less direct SQL and instead go through the insert() method on QuerySet. That's a little slower than the direct approach, but there's still optimisation possibilities there. It's only a few percent slower, though, not 15 minutes slower. However, although it shouldn't be changing the functionality, it's possible I'm doing something silly that doesn't port well to Oracle. So might want to print out all the SQL that is generated by, say, QuerySet.exeute_sql() to see if we're really sending queries to the database -- that's the last point of call before we talk to the db ,so it's a good chokepoint.

comment:5 by Malcolm Tredinnick, 16 years ago

Based on a chat with jbronn on IRC, it sounds like the endless spinning is the same problem as #6807 (for MySQL). Serves me right for only testing on SQLite and PostgreSQL. :-(

[7283] adds a empty_fetchmany_value to the BaseDatabaseFeatures class. Overriding that to be () might solve the infinite loop problem.

by jbronn, 16 years ago

Attachment: oracle_qsrf_patch_v4.diff added

Fixes infinite loop caused by use of default empty_fetchmany_value.

comment:6 by jbronn, 16 years ago

Setting the empty_fetchmany_value fixed it for me, and GIS tests pass.

comment:7 by Malcolm Tredinnick, 16 years ago

I was so close to applying this and then I read the patch. :-(

Justin, what have you done to my lookup_cast() and regex_lookup_*() methods??! They seemed to be playing a fairly important role, since I moved that functionality out of django/db/models/sql/where.py so that there weren't backend-specific bits in there.

Is this just a blunder or am I missing some reason that they're not needed? Or are we not testing that area at all and so it escaped?

comment:8 by Malcolm Tredinnick, 16 years ago

Resolution: fixed
Status: newclosed

(In [7321]) queryset-refactor: Initial pass at fixing the Oracle support. Thanks, Justin Bronn. Fixed #6161

This is untested (by me) and is a slight modification on Justin's original
patch, so feedback and bug reports are welcome.

comment:9 by Malcolm Tredinnick, 16 years ago

I committed what I guess is the right mixture between Justin's version and the new additions. Let's see what happens (and then we can tweak the as_sql() method a bit, as Justin indicates in the summary).

comment:10 by jbronn, 16 years ago

My bad, I accidentally copied base.py from trunk, I'll test again soon.

comment:11 by jbronn, 16 years ago

Confirmed that the GIS tests pass.

Note: See TracTickets for help on using tickets.
Back to Top