Skip to content
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

neq behaviour differs from TinkerGraph #2205

Closed
porunov opened this issue Sep 11, 2020 · 4 comments
Closed

neq behaviour differs from TinkerGraph #2205

porunov opened this issue Sep 11, 2020 · 4 comments
Assignees

Comments

@porunov
Copy link
Member

porunov commented Sep 11, 2020

TinkerPop added a test which now fails with JanusGraph.
In our case we assume that if there is no property with the specified name in has step with neq predicate than the result is true but TinkerGraph assumes that if there is no such property the result should always be false.

The related discussion with @spmallette: apache/tinkerpop@76fc5c2#r42228711

As we need to alight with TinkerPop, I believe we need to change the behaviour on neq step to preserve compatibility.

For confirmed bugs, please report:

gremlin> graph=JanusGraphFactory.open("inmemory")
==>standardjanusgraph[inmemory:[127.0.0.1]]
gremlin> graph.addVertex()
==>v[4344]
gremlin> graph.traversal().V().has("p", neq("v"))
18:51:57 WARN  org.janusgraph.graphdb.transaction.StandardJanusGraphTx  - Query requires iterating over all vertices [()]. For better performance, use indexes
==>v[4344]
gremlin>
gremlin> graph=TinkerGraph.open()
==>tinkergraph[vertices:0 edges:0]
gremlin> graph.addVertex()
==>v[0]
gremlin> graph.traversal().V().has("p", neq("v"))
gremlin> 
@rngcntr
Copy link
Contributor

rngcntr commented Sep 14, 2020

Responsible for that part of the logic should be this piece of code:

if (type == null) {
if (atom.getPredicate() == Cmp.EQUAL && atom.getValue() == null ||
(atom.getPredicate() == Cmp.NOT_EQUAL && atom.getValue() != null))
continue; //Ignore condition, its trivially satisfied
return null;
}

Just as a hint for everyone who wants to start investigating.

@rngcntr
Copy link
Contributor

rngcntr commented Sep 14, 2020

The definition of has('someProp', null) should probably also be changed in order to stay consistent with TinkerPop. Example:

gremlin> graph=JanusGraphFactory.open("inmemory")
==>standardjanusgraph[inmemory:[127.0.0.1]]
gremlin> graph.addVertex()
==>v[4344]
gremlin> graph.traversal().V().has("p", null)
18:51:57 WARN  org.janusgraph.graphdb.transaction.StandardJanusGraphTx  - Query requires iterating over all vertices [()]. For better performance, use indexes
==>v[4344]
gremlin>
gremlin> graph=TinkerGraph.open()
==>tinkergraph[vertices:0 edges:0]
gremlin> graph.addVertex()
==>v[0]
gremlin> graph.traversal().V().has("p", null)
gremlin> 

@spmallette
Copy link
Contributor

Note that null will have meaning for Gremlin in 3.5.0 - https://tinkerpop.apache.org/docs/3.5.0-SNAPSHOT/upgrade/#_null_semantics

@li-boxuan
Copy link
Member

In fact, JanusGraph's neq behavior when using index contradicts to its own behavior when using full scan, as shown in #1868 (comment)

li-boxuan added a commit to li-boxuan/janusgraph that referenced this issue Dec 6, 2020
@li-boxuan li-boxuan self-assigned this Dec 6, 2020
li-boxuan added a commit to li-boxuan/janusgraph that referenced this issue Dec 6, 2020
li-boxuan added a commit to li-boxuan/janusgraph that referenced this issue Dec 7, 2020
@porunov porunov added this to the Release v0.6.0 milestone Dec 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants