Opened 5 years ago

Closed 3 years ago

Last modified 3 years ago

#17339 closed New feature (fixed)

[nonrel] Factor out has_result into database backend

Reported by: Jonas H. 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


(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 Jonas H. 5 years ago.

Download all attachments as: .zip

Change History (13)

Changed 5 years ago by Jonas H.

comment:1 Changed 5 years ago by Carl Meyer

Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset
Triage Stage: UnreviewedAccepted
Type: UncategorizedNew feature

Giving backends more control here makes sense to me.

comment:2 Changed 5 years ago by Jonas H.

Does this need tests?

comment:3 Changed 5 years ago by Carl Meyer

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 5 years ago by Jonas H.

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

comment:5 Changed 5 years ago by Waldemar Kornewald

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 3 years ago by Tim Graham

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?

comment:7 Changed 3 years ago by Russell Keith-Magee

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 3 years ago by Claude Paroz

Triage Stage: AcceptedReady for checkin

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

Resolution: fixed
Status: newclosed

In 95eb68c98fe2dc95277d4e26a0316d856dc868dd:

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

Thanks jonash.

comment:10 Changed 3 years ago by anonymous

Easy pickings: set
Has patch: unset
Resolution: fixedneedsinfo

comment:11 Changed 3 years ago by anonymous

Easy pickings: unset

comment:12 Changed 3 years ago by Tim Graham

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