Make extra table constraints subqueried #562
Conversation
msdemlei
force-pushed
the
make-extra-table-constraints-subqueried
branch
from
d7a186e to
ca6584a
Compare
This is a lot more stable versus extra rows than the natural joins we've been doing so far.
msdemlei
force-pushed
the
make-extra-table-constraints-subqueried
branch
from
ca6584a to
3bc52a3
Compare
|
LGTM except a question: did you intentionally do three sub-queries for the Freetext constraint? Is this more efficient than a OR in a single sub-query? |
msdemlei
force-pushed
the
make-extra-table-constraints-subqueried
branch
from
3bc52a3 to
8a726d6
Compare
|
Ok, sorry about the whitespace and the pdb. This should now clear the CI except for the mysterious linkcheck message. If a sphinx deity can help me out there, that'd be much appreciated. But perhaps this can go even with this broken link? |
|
On Thu, Jun 27, 2024 at 12:35:27AM -0700, Manon Marchand wrote:
LGTM except a question: did you intentionally do three sub-queries
for the Freetext constraint? Is this more efficient than a OR in a
single sub-query?
This has been introduced in an older change, and indeed I should be
moving the non-UNION branch to subqueries as well (I promise I will
do so later). Anyway, the background is that the OR operator in many
situations precludes the use of an index, and UNION helps in these
cases. So, yes, that's by design.
|
| @@ -284,14 +318,15 @@ def _get_or_condition(self, service): | |||
| return super().get_search_condition(service) | |||
|
|
|||
|
|
|||
| class Author(Constraint): | |||
| class Author(SubqueriedConstraint): | |||
There was a problem hiding this comment.
sphinx is failing as in the API docs it tries to link to the baseclass, but the new class hasn't been added to the public API and therefore there is no API docs available for it.
I'll add a commit that resolves this issue, take it or leave it (I mean we can work around not exposing the class in the public API if necessary).
FWIW, Constraint is in the public API, so I don't think it's a big problem to add the new one, too.
|
On Thu, Jun 27, 2024 at 05:58:47PM -0700, Brigitta Sipőcz wrote:
I'll add a commit that resolves this issue, take it or leave it (I
mean we can work around not exposing the class in the public API if
necessary).
Ah, thanks.
FWIW, `Constraint` is in the public API, so I don't think it's a
big problem to add the new one, too.
Yeah, and I'm pretty sure that when I try to teach people how to
write constraints of their own, I'll nudge them towards
SubqueriedConstraint, as a lot less can go wrong with it.
|
|
If @bsipocz also approves I think this can be merged |
|
I haven't reviewed the content, but 100% trust your review, so go for the merge. The only thing to check for is if the PR is fixing a bug that is already in the release, then it can have the bugfix milestone (atm I'm not sure if we have one more of those or 1.6 next, so we don't care about changelog consistency) |
There has been a fairly evil thinko in the way we've been writing constraints requiring extra tables. What we did is something like
This is innocent enough in hand-written, one-shot queries. For us with our complicated way of pulling all capabilities in one go, it's bad, because if there's n columns with a matching UCD, then every interface will appear n times in our interfaces list. That was relatively harmless before we became more uptight about duplicated interfaces in #505 and environment. It is a disaster now, and so we want to change our style to
This is probably not going to be much of a difference in terms of what the databases actually execute; the query planner should take care of that.
Oh, by the way, this PR should be merged after #561 and contains that commit already.