Fix catalog plan for not query

[mamico]

The not queries produce the same key as the normal queries in the catalog plan, this is a problem because the not queries could generally be slower.

Before this PR, the queries {'UID': '1', path: '/a'} and {'UID': {'not': '1'}, path: '/a'} share the same plan, with the key ('UID', 'path'), after they will have two distinct plan with the keys ('UID', 'path') and (('UID', 'not'), 'path').

[jensens]

Complex topic, but a different plan for not queries is definitely missing here. That said, not queries are not always slow, but as far as I understand this is for the test only, to push not to the end.

[ale-rt]

Disclaimer: I am not competent at all to judge on this PR, so do not expect a review from me :).

The only thing I can tell is that I am not a super fan of mixing key types (in this case string and tuples).
It might backfire for whatever reason.

Maybe changing the key from ("UID", "not") to something like "!UID", "UID:not", "UID|not" or whatever might be better.
I would not pick the $NAME_not option because I am pretty sure in some portal catalog in this world there is an index whose name ends with _not ;)

Feel free to discard my comment.

[mamico]

The only thing I can tell is that I am not a super fan of mixing key types (in this case string and tuples). It might backfire for whatever reason.

Good point ... but I've followed the same implementation (tuple with name and query) already used for valueindexes see https://github.com/zopefoundation/Products.ZCatalog/blob/master/src/Products/ZCatalog/plan.py#L246

[davisagli]

@mamico If I'm reading https://github.com/zopefoundation/Products.ZCatalog/blob/master/src/Products/PluginIndexes/unindex.py#L433 correctly, I think it is possible to query a single index with both a "normal" (non-inverted) and "not" query at the same time; e.g. searchResults(indexname={"query": "x", "not": "y"}).

The assumption made by this line of code is that an index is either performing a normal query or a not query, but not both, and I don't think that's a valid assumption in this edge case.

[davisagli]

Maybe that's fine though. In that edge case we'll end up with ("indexname", "not") as the key -- so it'll be grouped together with queries that are only doing a not query, but at least it'll be grouped together with another less common case for the purpose of catalog planning rather than being grouped with other queries that don't intersect with the not results.

[mamico]

Maybe, a catchall key for all operators could be something like

        operatorkeys = [
            name for name in key
            if isinstance(query.get(name), dict)
        ]
        if operatorkeys:
            key = [name for name in key if name not in operatorkeys]
            key.extend([(name, tuple(sorted(query[name].keys()) for name in operatorkeys])

[davisagli]

@mamico Interesting idea.

But since indexes are pluggable and each type of index can have arbitrary operators, I'm a little hesitant about making it this generic, since there are probably situations where the operator has a significant impact on the runtime of the query and other situations where it does not. Better to only handle not at the moment, I think, so that we're only changing behavior of a specific case that we understand.

It also occurs to me that maybe determining the key for the queryplan should be delegated to the index implementation -- except that it's not necessarily trivial to find the index(es?) that corresponds to a particular name in the query.

[davisagli]

Another case that is not handled here is if there is a query using the not operator on an index that is being treated as a value index.

[mamico]

Another case that is not handled here is if there is a query using the not operator on an index that is being treated as a value index.

Nope, This is handled. Because those are string (with not and value included), not dict, after this https://github.com/zopefoundation/Products.ZCatalog/blob/master/src/Products/ZCatalog/plan.py#L246

[davisagli]

Ah, yes, I see what you mean.

[davisagli]

Okay, so now that I've thought this through: I think there is more refinement that can be done on the code, but I also think it will not cause problems as currently implemented, and it helps with a specific case. So I'm okay with merging as is.