-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Not step doesn't look in Index #1868
Comments
Jakub, why did you change the title from "Not step doesn't look in Index" to "Not step doesn't look in Index, after an Or step". Is there a way to get it to hit the index using a not() step without the or() step before it? |
@albertlockett2 you're right, I've updated the title. I don't think I tried the following when I originally opened the issue:
|
Thanks Jakub. I was digging through the code on the weekend a bit. It looks like there's support for it in the ElasticSearchIndex class, in the getFilter method:
But that "not" condition doesn't get injected into the IndexQuery object. I didn't know which class should be responsible for that. Maybe JanusGraphStepStrategy should be responsible for setting it on JanusGraphStep, and the GraphCentricQueryBuilder should handle it? Can anyone who's "in the know" confirm that? |
|
@li-boxuan Do you care to explain how they are different from your perspective? |
I admit using the above example, JanusGraph shows deviated behavior:
which is a bug as reported in #2205 However, when using index, JanusGraph follows the correct semantic:
|
Thanks for the explanation @li-boxuan, it clears up some of the confusion, between these 2 similar filters. |
What about steps that don't have an equivalent not predicate like Text.textRegex? If I'm using Elasticsearch for indexing solution, I would be able to add a not clause to a bool query
|
Have you tried the complement operator? E.g. If the above does not work well for your use case, you could open up a new feature request issue. Probably we could support syntax like |
@li-boxuan yeah that can work too. My thinking was that from an application developer's perspective I guess so does |
@albertlockett2 It’s not about which way is easier to implement. Similar to what I described at #1868 (comment), not(has(“prop”, Text.textRegex(...))) means either a vertex does not have prop OR its prop value does not satisfy given regex. In other words, it’s the complement of the has clause, not just complement of the regex part. |
@li-boxuan Ok fair enough but for the record |
@albertlockett2 If a vertex/edge does not have “prop” property, then it will not be indexed. If we use Regex query in ES, it returns vertices with “prop” and whose values match regex. If we use exists query in ES, it returns vertices with “prop”. If your goal is to find vertices with “prop” but not matches regex, I think you can write query in this way: .has(“prop”).not(has(“prop”, Text.textRegex(...)) However, the above query still does not use index, but it can be a nice feature for JanusGraph to support. For the not(has(“prop”, Text.textRegex(...)) query, I don’t see any chance to avoid a full scan. This query is not what you want anyways, since it has a different semantics meaning. |
@albertlockett2 FYI there is a PR to add this support: #2559 |
In JanusGraph
.not(__.has("pname", "value"))
after an or step, is not processed correctly by the query planner.This issue was first described here: #163 (comment).
For example:
The query bellow does not use an index, since"query.force-index" is set to "true" and thus, results in an exception:
The query bellow works just fine:
Those queries are semantically the same. But the execution is not the same.
The first version tries to do a full-graph scan, which is disabled in our setup.
The second query using
P.neq
is able to use the index.The
Not
step does not use the index, if it occurs just after anOr
step.We have a mixed index with all the properties used in the query.
0.3.2
Stack Trace
The text was updated successfully, but these errors were encountered: