Fix so that we coerce when necessary in compiled expression

In a not so distant future we should be able to rely on planning to have
properly coerced for us, but until then we need to handle it explicitly
in compiled expressions

community/cypher/cypher-planner-3.5/src/main/scala/org/neo4j/cypher/internal/compiler/v3_5/helpers/PredicateHelper.scala

@@ -57,7 +57,7 @@ object PredicateHelper {
   //i) we do late ast rewrite after semantic analysis, so all semantic table will be missing some expression
   //ii) For WHERE a.prop semantic analysis will say that a.prop has boolean type since it belongs to a WHERE.
   //    That makes it not usable here since we would need to coerce in that case.
-  private def isPredicate(expression: Expression) = {
+  def isPredicate(expression: Expression) = {
     expression match {
       case o: OperatorExpression => o.signatures.forall(_.outputType == symbols.CTBoolean)
       case f: FunctionInvocation => BOOLEAN_FUNCTIONS.contains(f.function)

community/cypher/runtime-util/src/main/java/org/neo4j/cypher/operations/CypherBoolean.java

@@ -58,150 +58,133 @@ private CypherBoolean()
 
     public static Value xor( AnyValue lhs, AnyValue rhs )
     {
-        boolean seenNull = false;
-        if ( lhs == NO_VALUE || rhs == NO_VALUE )
-        {
-            return NO_VALUE;
-        }
-
         return (lhs == Values.TRUE) ^ (rhs == Values.TRUE) ? Values.TRUE : Values.FALSE;
     }
 
     public static Value not( AnyValue in )
     {
-        if ( in == NO_VALUE )
-        {
-            return NO_VALUE;
-        }
-
         return in != Values.TRUE ? Values.TRUE : Values.FALSE;
     }
 
     public static Value equals( AnyValue lhs, AnyValue rhs )
     {
-        Boolean equals = lhs.ternaryEquals( rhs );
-        if ( equals == null )
+        Boolean compare = lhs.ternaryEquals( rhs );
+        if ( compare == null )
         {
             return NO_VALUE;
         }
-        else
-        {
-            return equals ? Values.TRUE : Values.FALSE;
-        }
+        return compare ? Values.TRUE : Values.FALSE;
     }
 
     public static Value notEquals( AnyValue lhs, AnyValue rhs )
     {
-        Boolean equals = lhs.ternaryEquals( rhs );
-        if ( equals == null )
+        Boolean compare = lhs.ternaryEquals( rhs );
+        if ( compare == null )
         {
             return NO_VALUE;
         }
-        else
-        {
-            return equals ? Values.FALSE : Values.TRUE;
-        }
+        return compare ? Values.FALSE : Values.TRUE;
     }
 
-    public static BooleanValue coerceToBoolean( AnyValue value )
+    public static Value coerceToBoolean( AnyValue value )
     {
-        return value.map( BOOLEAN_MAPPER ) ? Values.TRUE : Values.FALSE;
+        return value.map( BOOLEAN_MAPPER );
     }
 
-    private static final class BooleanMapper implements ValueMapper<Boolean>
+    private static final class BooleanMapper implements ValueMapper<Value>
     {
         @Override
-        public Boolean mapPath( PathValue value )
+        public Value mapPath( PathValue value )
         {
-            return value.size() > 0;
+            return value.size() > 0 ? Values.TRUE : Values.FALSE;
         }
 
         @Override
-        public Boolean mapNode( VirtualNodeValue value )
+        public Value mapNode( VirtualNodeValue value )
         {
             throw new CypherTypeException( "Don't know how to treat that as a boolean: " + value, null );
         }
 
         @Override
-        public Boolean mapRelationship( VirtualRelationshipValue value )
+        public Value mapRelationship( VirtualRelationshipValue value )
         {
             throw new CypherTypeException( "Don't know how to treat that as a boolean: " + value, null );
         }
 
         @Override
-        public Boolean mapMap( MapValue value )
+        public Value mapMap( MapValue value )
         {
             throw new CypherTypeException( "Don't know how to treat that as a boolean: " + value, null );
         }
 
         @Override
-        public Boolean mapNoValue()
+        public Value mapNoValue()
         {
-            return false;
+            return NO_VALUE;
         }
 
         @Override
-        public Boolean mapSequence( SequenceValue value )
+        public Value mapSequence( SequenceValue value )
         {
-            return value.length() > 0;
+            return value.length() > 0 ? Values.TRUE : Values.FALSE;
         }
 
         @Override
-        public Boolean mapText( TextValue value )
+        public Value mapText( TextValue value )
         {
             throw new CypherTypeException( "Don't know how to treat that as a boolean: " + value, null );
         }
 
         @Override
-        public Boolean mapBoolean( BooleanValue value )
+        public Value mapBoolean( BooleanValue value )
         {
-            return value.booleanValue();
+            return value;
         }
 
         @Override
-        public Boolean mapNumber( NumberValue value )
+        public Value mapNumber( NumberValue value )
         {
             throw new CypherTypeException( "Don't know how to treat that as a boolean: " + value, null );
         }
 
         @Override
-        public Boolean mapDateTime( DateTimeValue value )
+        public Value mapDateTime( DateTimeValue value )
         {
             throw new CypherTypeException( "Don't know how to treat that as a boolean: " + value, null );
         }
 
         @Override
-        public Boolean mapLocalDateTime( LocalDateTimeValue value )
+        public Value mapLocalDateTime( LocalDateTimeValue value )
         {
             throw new CypherTypeException( "Don't know how to treat that as a boolean: " + value, null );
         }
 
         @Override
-        public Boolean mapDate( DateValue value )
+        public Value mapDate( DateValue value )
         {
             throw new CypherTypeException( "Don't know how to treat that as a boolean: " + value, null );
         }
 
         @Override
-        public Boolean mapTime( TimeValue value )
+        public Value mapTime( TimeValue value )
         {
             throw new CypherTypeException( "Don't know how to treat that as a boolean: " + value, null );
         }
 
         @Override
-        public Boolean mapLocalTime( LocalTimeValue value )
+        public Value mapLocalTime( LocalTimeValue value )
         {
             throw new CypherTypeException( "Don't know how to treat that as a boolean: " + value, null );
         }
 
         @Override
-        public Boolean mapDuration( DurationValue value )
+        public Value mapDuration( DurationValue value )
         {
             throw new CypherTypeException( "Don't know how to treat that as a boolean: " + value, null );
         }
 
         @Override
-        public Boolean mapPoint( PointValue value )
+        public Value mapPoint( PointValue value )
         {
             throw new CypherTypeException( "Don't know how to treat that as a boolean: " + value, null );
         }

enterprise/cypher/compiled-expressions/src/main/java/org/neo4j/cypher/internal/runtime/compiled/expressions/CompiledHelpers.java

@@ -22,7 +22,8 @@
  */
 package org.neo4j.cypher.internal.runtime.compiled.expressions;
 
-import org.neo4j.cypher.CypherTypeException;
+import org.opencypher.v9_0.util.CypherTypeException;
+
 import org.neo4j.values.AnyValue;
 import org.neo4j.values.storable.BooleanValue;
 import org.neo4j.values.storable.Value;

enterprise/cypher/compiled-expressions/src/main/scala/org/neo4j/cypher/internal/runtime/compiled/expressions/IntermediateCodeGeneration.scala

@@ -24,6 +24,7 @@ package org.neo4j.cypher.internal.runtime.compiled.expressions
 
 import org.neo4j.cypher.internal.compatibility.v3_5.runtime.SlotConfiguration
 import org.neo4j.cypher.internal.compatibility.v3_5.runtime.ast._
+import org.neo4j.cypher.internal.compiler.v3_5.helpers.PredicateHelper.isPredicate
 import org.neo4j.cypher.internal.runtime.DbAccess
 import org.neo4j.cypher.internal.runtime.compiled.expressions.IntermediateRepresentation.method
 import org.neo4j.cypher.internal.runtime.interpreted.ExecutionContext
@@ -178,66 +179,88 @@ class IntermediateCodeGeneration(slots: SlotConfiguration) {
     case Or(lhs, rhs) =>
       for {l <- compile(lhs)
            r <- compile(rhs)
-      } yield generateOrs(List(l, r))
+      } yield {
+        val left = if (isPredicate(lhs)) l else coerceToPredicate(l)
+        val right = if (isPredicate(rhs)) r else coerceToPredicate(r)
+        generateOrs(List(left, right))
+      }
 
     case Ors(expressions) =>
-      val compiled = expressions.foldLeft[Option[List[IntermediateExpression]]](Some(List.empty)) { (acc, current) =>
+      val compiled = expressions.foldLeft[Option[List[(IntermediateExpression, Boolean)]]](Some(List.empty)) { (acc, current) =>
         for {l <- acc
-             e <- compile(current)} yield l :+ e
+             e <- compile(current)} yield l :+ (e -> isPredicate(current))
       }
 
       for (e <- compiled) yield e match {
         case Nil => IntermediateExpression(truthValue, nullable = false) //this will not really happen because of rewriters etc
-        case a :: Nil  => a
-        case list => generateOrs(list)
+        case (a, isPredicate) :: Nil  => if (isPredicate) a else coerceToPredicate(a)
+        case list =>
+          val coerced = list.map {
+            case (p, true) => p
+            case (p, false) => coerceToPredicate(p)
+          }
+          generateOrs(coerced)
       }
 
     case Xor(lhs, rhs) =>
       for {l <- compile(lhs)
            r <- compile(rhs)
-      } yield IntermediateExpression(invokeStatic(method[CypherBoolean, Value, AnyValue, AnyValue]("xor"), l.ir, r.ir),
-                                     l.nullable | l.nullable)
+      } yield {
+        val left = if (isPredicate(lhs)) l else coerceToPredicate(l)
+        val right = if (isPredicate(rhs)) r else coerceToPredicate(r)
+        IntermediateExpression(
+          noValueCheck(left, right)(invokeStatic(method[CypherBoolean, Value, AnyValue, AnyValue]("xor"), left.ir, right.ir)),
+                               left.nullable | right.nullable)
+      }
 
     case And(lhs, rhs) =>
       for {l <- compile(lhs)
            r <- compile(rhs)
-      } yield generateAnds(List(l, r))
+      } yield {
+        val left = if (isPredicate(lhs)) l else coerceToPredicate(l)
+        val right = if (isPredicate(rhs)) r else coerceToPredicate(r)
+        generateAnds(List(left, right))
+      }
 
     case Ands(expressions) =>
-      val compiled = expressions.foldLeft[Option[List[IntermediateExpression]]](Some(List.empty)) { (acc, current) =>
-        for {l <- acc
-             e <- compile(current)} yield l :+ e
+      val compiled = expressions.foldLeft[Option[List[(IntermediateExpression, Boolean)]]](Some(List.empty)) { (acc, current) =>
+          for {l <- acc
+               e <- compile(current)} yield l :+ (e -> isPredicate(current))
         }
 
       for (e <- compiled) yield e match {
         case Nil => IntermediateExpression(truthValue, nullable = false) //this will not really happen because of rewriters etc
-        case a :: Nil  => a
-        case list => generateAnds(list)
+        case (a, isPredicate) :: Nil  => if (isPredicate) a else coerceToPredicate(a)
+        case list =>
+          val coerced = list.map {
+            case (p, true) => p
+            case (p, false) => coerceToPredicate(p)
+          }
+          generateAnds(coerced)
       }
 
     case Not(arg) =>
-      compile(arg).map(a =>
+      compile(arg).map(a => {
+        val in = if (isPredicate(arg)) a else coerceToPredicate(a)
         IntermediateExpression(
-          invokeStatic(method[CypherBoolean, Value, AnyValue]("not"), a.ir), a.nullable))
+          noValueCheck(in)(invokeStatic(method[CypherBoolean, Value, AnyValue]("not"), in.ir)), in.nullable)
+      })
 
     case Equals(lhs, rhs) =>
       for {l <- compile(lhs)
            r <- compile(rhs)
-      } yield IntermediateExpression(
-                invokeStatic(method[CypherBoolean, Value, AnyValue, AnyValue]("equals"), l.ir, r.ir), l.nullable | r.nullable)
+      } yield IntermediateExpression(invokeStatic(method[CypherBoolean, Value, AnyValue, AnyValue]("equals"), l.ir, r.ir),
+                l.nullable | r.nullable)
 
 
     case NotEquals(lhs, rhs) =>
       for {l <- compile(lhs)
            r <- compile(rhs)
-      } yield IntermediateExpression(
-        invokeStatic(method[CypherBoolean, Value, AnyValue, AnyValue]("notEquals"), l.ir, r.ir), l.nullable | r.nullable)
+      } yield IntermediateExpression(invokeStatic(method[CypherBoolean, Value, AnyValue, AnyValue]("notEquals"), l.ir, r.ir),
+        l.nullable | r.nullable)
+
+    case CoerceToPredicate(inner) => compile(inner).map(coerceToPredicate)
 
-    case CoerceToPredicate(inner) =>
-      compile(inner).map(e =>
-                           IntermediateExpression(
-                             invokeStatic(method[CypherBoolean, BooleanValue, AnyValue]("coerceToBoolean"), e.ir),
-                             nullable = false))
     //data access
     case Parameter(name, _) => //TODO parameters that are autogenerated from literals should have nullable = false
       Some(IntermediateExpression(invoke(load("params"), method[MapValue, AnyValue, String]("get"),
@@ -375,6 +398,10 @@ class IntermediateCodeGeneration(slots: SlotConfiguration) {
     nextName
   }
 
+  private def coerceToPredicate(e: IntermediateExpression) = IntermediateExpression(
+    invokeStatic(method[CypherBoolean, Value, AnyValue]("coerceToBoolean"), e.ir),
+    nullable = e.nullable)
+
   /**
     * Ok AND and ANDS are complicated.  At the core we try to find a single `FALSE` if we find one there is no need to look
     * at more predicates. If it doesn't find a `FALSE` it will either return `NULL` if any of the predicates has evaluated