Django

Code

Ticket #6161 (closed: fixed)

Opened 1 year ago

Last modified 9 months ago

Oracle support for queryset-refactor.

Reported by: jbronn Assigned to: mtredinnick
Milestone: Component: Database layer (models, ORM)
Version: other branch Keywords: qs-rf oracle
Cc: jbronn@gmail.com, ikelly Triage Stage: Accepted
Has patch: 1 Needs documentation: 0
Needs tests: 0 Patch needs improvement: 0

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 geometry adaptor class of GeoDjango.

Attachments

oracle_qsrf_patch.diff (15.3 kB) - added by jbronn on 12/08/07 17:28:24.
Patch enabling Oracle functionality on the queryset-refactor branch.
oracle_qsrf_patch_v2.diff (14.3 kB) - added by jbronn on 12/08/07 19:21:23.
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 on 03/17/08 16:02:16.
New version of patch that reverts the changes added in [6905]
oracle_qsrf_patch_v4.diff (14.9 kB) - added by jbronn on 03/19/08 00:07:24.
Fixes infinite loop caused by use of default empty_fetchmany_value.

Change History

12/08/07 17:28:24 changed by jbronn

  • attachment oracle_qsrf_patch.diff added.

Patch enabling Oracle functionality on the queryset-refactor branch.

12/08/07 18:05:17 changed by mtredinnick

  • needs_better_patch changed.
  • stage changed from Unreviewed to Accepted.
  • needs_tests changed.
  • needs_docs changed.

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.

12/08/07 19:21:23 changed by jbronn

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

03/16/08 17:35:07 changed by ikelly

  • cc changed from jbronn@gmail.com to jbronn@gmail.com, ikelly.

03/17/08 16:02:16 changed by ikelly

  • attachment oracle_qsrf_patch_v3.diff added.

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

03/17/08 16:06:48 changed 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.

03/17/08 23:52:12 changed 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.

03/18/08 01:17:33 changed 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.

03/19/08 00:07:24 changed by jbronn

  • attachment oracle_qsrf_patch_v4.diff added.

Fixes infinite loop caused by use of default empty_fetchmany_value.

03/19/08 00:09:14 changed by jbronn

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

03/19/08 10:22:26 changed 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?

03/19/08 10:46:20 changed by mtredinnick

  • status changed from new to closed.
  • resolution set to fixed.

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

03/19/08 10:49:22 changed 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).

03/19/08 17:20:03 changed by jbronn

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

03/19/08 18:13:44 changed by jbronn

Confirmed that the GIS tests pass.

Add/Change #6161 (Oracle support for queryset-refactor.)




Change Properties
Action