Opened 4 years ago

Closed 2 years ago

Last modified 2 years ago

#17339 closed New feature (fixed)

[nonrel] Factor out has_result into database backend

Reported by: jonash Owned by: nobody
Component: Database layer (models, ORM) Version: master
Severity: Normal Keywords: nonrel
Cc: Triage Stage: Ready for checkin
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

(This patch is part of the changes made to the inofficial fork "Django-nonrel" which adds basic support for non-relational databases.)

Refactors some code of sql.query.Query.has_result into the database backend. This is required so that database backends can use specialized more performant "query has any results" checks.

Attachments (1)

factor-out-has_results.patch (1.4 KB) - added by jonash 4 years ago.

Download all attachments as: .zip

Change History (13)

Changed 4 years ago by jonash

comment:1 Changed 4 years ago by carljm

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted
  • Type changed from Uncategorized to New feature

Giving backends more control here makes sense to me.

comment:2 Changed 4 years ago by jonash

Does this need tests?

comment:3 Changed 4 years ago by carljm

Pretty sure the test suite already exercises this code quite heavily, so I think in this case of a pure refactoring there aren't more tests needed, as long as the existing ones pass. And obviously docs aren't relevant at this level either.

In terms of the patch specifics, what's the justification for why some query modification bits are moved into the compiler's has_results and not others?

comment:4 Changed 4 years ago by jonash

I have no idea. Maybe I can get the original author, Waldemar Kornewald, to join these threads...

comment:5 Changed 3 years ago by wkornewald

The reason why I only moved two lines to the compiler is that all the other operations are DB independent optimizations. The selected fields and query ordering etc need to be cleared on NoSQL, too. Only the extra mask is a SQL specific optimization and other DBs might use some other trick. Anyway,it would be easy to change the patch to also allow for overriding the other operations, but we should keep the current separation between extra mask and general optimizations because all nonrel backends only override the extra mask code and they shouldn't have to reimplement the general optimizations unnecessarily.

comment:6 Changed 2 years ago by timo

Duplicate of #14857 which was closed as "won't fix" by Russ, but I'm not sure if his reasoning still holds since query-refactor was never merged and NoSQL support seems to have dropped off the radar as far as I know:

"NoSQL support isn't something that needs to be addressed piecemeal. This isn't the only place that raw SQL leaks out of the compiler; these leaks need to be addressed as a whole, not piecemeal. As I've told you *many* times Waldemar, NoSQL support is on my radar for 1.4, when we address merging Alex's query-refactor SoC branch."

I've updated the patch to apply cleanly and ensure that all tests pass. Any objections to merging this?

https://github.com/django/django/pull/1346

comment:7 Changed 2 years ago by russellm

The NoSQL branch efforts are obviously dead, but the pull request here makes perfect sense to me. It's a small piece of the puzzle, but it doesn't hurt, so no objections on my part to merging.

comment:8 Changed 2 years ago by claudep

  • Triage Stage changed from Accepted to Ready for checkin

comment:9 Changed 2 years ago by Tim Graham <timograham@…>

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

In 95eb68c98fe2dc95277d4e26a0316d856dc868dd:

Fixed #17339 -- Factor out has_result into database backend.

Thanks jonash.

comment:10 Changed 2 years ago by anonymous

  • Easy pickings set
  • Has patch unset
  • Resolution changed from fixed to needsinfo

comment:11 Changed 2 years ago by anonymous

  • Easy pickings unset

comment:12 Changed 2 years ago by timo

  • Resolution changed from needsinfo to fixed
Note: See TracTickets for help on using tickets.
Back to Top