Fix neq and null condition query behavior

Fixes JanusGraph#2205

Signed-off-by: Boxuan Li <[email protected]>

janusgraph-backend-testutils/src/main/java/org/janusgraph/graphdb/JanusGraphIndexTest.java

@@ -188,6 +188,123 @@ public static void assertGraphOfTheGods(JanusGraph graphOfTheGods) {
         graphOfTheGods.tx().commit();
     }
 
+    @Test
+    public void testNeqQuery() {
+        makeKey("p2", String.class);
+        PropertyKey p3 = makeKey("p3", String.class);
+        PropertyKey p4 = makeKey("p4", String.class);
+        mgmt.buildIndex("composite", Vertex.class).addKey(p3).buildCompositeIndex();
+        mgmt.buildIndex("mixed", Vertex.class).addKey(p4, Mapping.STRING.asParameter()).buildMixedIndex(INDEX);
+        finishSchema();
+
+        tx.addVertex();
+        tx.commit();
+        newTx();
+
+        // property not registered in schema
+        assertFalse(tx.traversal().V().has("p1", P.neq("v")).hasNext());
+        assertFalse(tx.traversal().V().has("p1", P.neq(null)).hasNext());
+        // property registered in schema
+        assertFalse(tx.traversal().V().has("p2", P.neq("v")).hasNext());
+        assertFalse(tx.traversal().V().has("p2", P.neq(null)).hasNext());
+        // property registered in schema and has composite index
+        assertFalse(tx.traversal().V().has("p3", P.neq("v")).hasNext());
+        assertFalse(tx.traversal().V().has("p3", P.neq(null)).hasNext());
+        // property registered in schema and has mixed index
+        assertFalse(tx.traversal().V().has("p4", P.neq("v")).hasNext());
+        assertFalse(tx.traversal().V().has("p4", P.neq(null)).hasNext());
+    }
+
+    /**
+     * The behaviour of has(p, null) deviates from TinkerGraph. Since JanusGraph does not support null values,
+     * has(p, null) indicates hasNot(p). In other words, an absent property implicitly implies null value for that property.
+     */
+    @Test
+    public void testHasNullQuery() {
+        makeKey("p2", String.class);
+        PropertyKey p3 = makeKey("p3", String.class);
+        PropertyKey p4 = makeKey("p4", String.class);
+        mgmt.buildIndex("composite", Vertex.class).addKey(p3).buildCompositeIndex();
+        mgmt.buildIndex("mixed", Vertex.class).addKey(p4, Mapping.STRING.asParameter()).buildMixedIndex(INDEX);
+        finishSchema();
+
+        tx.addVertex();
+        tx.commit();
+        newTx();
+
+        // property not registered in schema
+        assertTrue(tx.traversal().V().has("p1", (Object) null).hasNext());
+        // property registered in schema
+        assertTrue(tx.traversal().V().has("p2", (Object) null).hasNext());
+        // property registered in schema and has composite index
+        assertTrue(tx.traversal().V().has("p3", (Object) null).hasNext());
+        // property registered in schema and has mixed index
+        assertTrue(tx.traversal().V().has("p4", (Object) null).hasNext());
+    }
+
+    /**
+     * The behaviour of hasNot(p) is straight-forward: hasNot(p) means it does not have such property p.
+     * Note that hasNot(p, value) (which is a JanusGraph API rather than gremlin API) is a bit tricky and it is equivalent
+     * to has(p, neq(value)). Therefore, hasNot(p, null) means has(p, neq(null)) which is equivalent to has(p).
+     */
+    @Test
+    public void testHasNot() {
+        makeKey("p2", String.class);
+        PropertyKey p3 = makeKey("p3", String.class);
+        PropertyKey p4 = makeKey("p4", String.class);
+        mgmt.buildIndex("composite", Vertex.class).addKey(p3).buildCompositeIndex();
+        mgmt.buildIndex("mixed", Vertex.class).addKey(p4, Mapping.STRING.asParameter()).buildMixedIndex(INDEX);
+        finishSchema();
+
+        tx.addVertex();
+        tx.commit();
+        newTx();
+
+        // property not registered in schema
+        assertTrue(tx.traversal().V().hasNot("p1").hasNext());
+        assertTrue(tx.query().hasNot("p1").vertices().iterator().hasNext());
+        assertFalse(tx.query().hasNot("p1", null).vertices().iterator().hasNext());
+        assertFalse(tx.query().hasNot("p1", "value").vertices().iterator().hasNext());
+        // property registered in schema
+        assertTrue(tx.traversal().V().hasNot("p2").hasNext());
+        assertTrue(tx.query().hasNot("p2").vertices().iterator().hasNext());
+        assertFalse(tx.query().hasNot("p2", null).vertices().iterator().hasNext());
+        assertFalse(tx.query().hasNot("p2", "value").vertices().iterator().hasNext());
+        // property registered in schema and has composite index
+        assertTrue(tx.traversal().V().hasNot("p3").hasNext());
+        assertTrue(tx.query().hasNot("p3").vertices().iterator().hasNext());
+        assertFalse(tx.query().hasNot("p3", null).vertices().iterator().hasNext());
+        assertFalse(tx.query().hasNot("p3", "value").vertices().iterator().hasNext());
+        // property registered in schema and has mixed index
+        assertTrue(tx.traversal().V().hasNot("p4").hasNext());
+        assertTrue(tx.query().hasNot("p4").vertices().iterator().hasNext());
+        assertFalse(tx.query().hasNot("p4", null).vertices().iterator().hasNext());
+        assertFalse(tx.query().hasNot("p4", "value").vertices().iterator().hasNext());
+    }
+
+    @Test
+    public void testNotHas() {
+        makeKey("p2", String.class);
+        PropertyKey p3 = makeKey("p3", String.class);
+        PropertyKey p4 = makeKey("p4", String.class);
+        mgmt.buildIndex("composite", Vertex.class).addKey(p3).buildCompositeIndex();
+        mgmt.buildIndex("mixed", Vertex.class).addKey(p4, Mapping.STRING.asParameter()).buildMixedIndex(INDEX);
+        finishSchema();
+
+        tx.addVertex();
+        tx.commit();
+        newTx();
+
+        // property not registered in schema
+        assertTrue(tx.traversal().V().not(__.has("p1")).hasNext());
+        // property registered in schema
+        assertTrue(tx.traversal().V().not(__.has("p2")).hasNext());
+        // property registered in schema and has composite index
+        assertTrue(tx.traversal().V().not(__.has("p3")).hasNext());
+        // property registered in schema and has mixed index
+        assertTrue(tx.traversal().V().not(__.has("p4")).hasNext());
+    }
+
     /**
      * Ensure clearing storage actually removes underlying graph and index databases.
      * @throws Exception

janusgraph-backend-testutils/src/main/java/org/janusgraph/graphdb/JanusGraphTest.java

@@ -5792,18 +5792,6 @@ private static void assertElementOrderForWithoutIndex(StandardJanusGraph graph)
 
     //................................................
 
-
-    @Test
-    public void testHasNot() {
-        JanusGraphVertex v1, v2;
-        v1 = graph.addVertex();
-
-        v2 = graph.query().hasNot("abcd").vertices().iterator().next();
-        assertEquals(v1, v2);
-        v2 = graph.query().hasNot("abcd", true).vertices().iterator().next();
-        assertEquals(v1, v2);
-    }
-
     @Test
     public void testVertexCentricIndexWithNull() {
         EdgeLabel bought = makeLabel("bought");

janusgraph-core/src/main/java/org/janusgraph/graphdb/query/QueryUtil.java

@@ -160,8 +160,7 @@ public static <E extends JanusGraphElement> And<E> constraints2QNF(StandardJanus
             final RelationType type = getType(tx, atom.getKey());
 
             if (type == null) {
-                if (atom.getPredicate() == Cmp.EQUAL && atom.getValue() == null ||
-                        (atom.getPredicate() == Cmp.NOT_EQUAL && atom.getValue() != null))
+                if (atom.getPredicate() == Cmp.EQUAL && atom.getValue() == null)
                     continue; //Ignore condition, its trivially satisfied
 
                 return null;

janusgraph-driver/src/main/java/org/janusgraph/core/attribute/Cmp.java

@@ -73,10 +73,13 @@ public boolean isValidCondition(Object condition) {
 
         @Override
         public boolean test(Object value, Object condition) {
-            if (condition==null) {
-                return value!=null;
+            if (value == null) {
+                // To align with TinkerPop behaviour, if an element does not have property p, then has(p, neq(anything))
+                // should always evaluate to false. Note that JanusGraph does not support null value, so the "value == null"
+                // here implies the element does not have such property.
+                return false;
             } else {
-                return !condition.equals(value);
+                return !value.equals(condition);
             }
         }