Code

Opened 7 years ago

Closed 6 years ago

Last modified 6 years ago

#6161 closed (fixed)

Oracle support for queryset-refactor.

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

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 7 years ago.
Patch enabling Oracle functionality on the queryset-refactor branch.
oracle_qsrf_patch_v2.diff (14.3 KB) - added by jbronn 7 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 ikelly 6 years ago.
New version of patch that reverts the changes added in [6905]
oracle_qsrf_patch_v4.diff (14.9 KB) - added by jbronn 6 years ago.
Fixes infinite loop caused by use of default empty_fetchmany_value.

Download all attachments as: .zip

Change History (15)

Changed 7 years ago by jbronn

Patch enabling Oracle functionality on the queryset-refactor branch.

comment:1 Changed 7 years ago by mtredinnick

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

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.

Changed 7 years ago by jbronn

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

comment:2 Changed 6 years ago by ikelly

  • Cc ikelly added

Changed 6 years ago by ikelly

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

comment:3 Changed 6 years ago by ikelly

[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 Changed 6 years ago by mtredinnick

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

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.

Changed 6 years ago by jbronn

Fixes infinite loop caused by use of default empty_fetchmany_value.

comment:6 Changed 6 years ago by jbronn

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

comment:7 Changed 6 years ago by mtredinnick

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

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

(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 Changed 6 years ago by mtredinnick

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

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

comment:11 Changed 6 years ago by jbronn

Confirmed that the GIS tests pass.

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.