Merge pull request #24161 from vespa-engine/balder/short-circuit-boolean

Balder/short circuit boolean

6 files changed, 36 insertions, 35 deletions

diff --git a/searchlib/src/main/java/com/yahoo/searchlib/rankingexpression/RankingExpression.java b/searchlib/src/main/java/com/yahoo/searchlib/rankingexpression/RankingExpression.java
index 6d6cbe13b5b..c9f818544e3 100755
--- a/searchlib/src/main/java/com/yahoo/searchlib/rankingexpression/RankingExpression.java
+++ b/searchlib/src/main/java/com/yahoo/searchlib/rankingexpression/RankingExpression.java
@@ -20,7 +20,6 @@ import java.io.Serializable;
 import java.io.StringReader;
 import java.util.Deque;
 import java.util.LinkedList;
-import java.util.List;
 import java.util.Map;
 
 /**
diff --git a/searchlib/src/main/java/com/yahoo/searchlib/rankingexpression/rule/ArithmeticNode.java b/searchlib/src/main/java/com/yahoo/searchlib/rankingexpression/rule/ArithmeticNode.java
index 580f42e67cb..c3e39197316 100755
--- a/searchlib/src/main/java/com/yahoo/searchlib/rankingexpression/rule/ArithmeticNode.java
+++ b/searchlib/src/main/java/com/yahoo/searchlib/rankingexpression/rule/ArithmeticNode.java
@@ -47,7 +47,9 @@ public final class ArithmeticNode extends CompositeNode {
             string.append("(");
 
         Iterator<ExpressionNode> child = children.iterator();
-        child.next().toString(string, context, path, this).append(" ");
+        child.next().toString(string, context, path, this);
+        if (child.hasNext())
+            string.append(" ");
         for (Iterator<ArithmeticOperator> op = operators.iterator(); op.hasNext() && child.hasNext();) {
             string.append(op.next().toString()).append(" ");
             child.next().toString(string, context, path, this);
@@ -65,16 +67,15 @@ public final class ArithmeticNode extends CompositeNode {
      * (even though by virtue of being a node it will be calculated before the parent).
      */
     private boolean nonDefaultPrecedence(CompositeNode parent) {
-        if ( parent==null) return false;
-        if ( ! (parent instanceof ArithmeticNode)) return false;
+        if ( parent == null) return false;
+        if ( ! (parent instanceof ArithmeticNode arithmeticParent)) return false;
 
-        ArithmeticNode arithParent = (ArithmeticNode) parent;
         // The line below can only be correct in both only have one operator.
         // Getting this correct is impossible without more work.
-        // So for now now we only handle the simple case correctly, and use a safe approach by adding
+        // So for now we only handle the simple case correctly, and use a safe approach by adding
         // extra parenthesis just in case....
-        return arithParent.operators.get(0).hasPrecedenceOver(this.operators.get(0))
-                || ((arithParent.operators.size() > 1) || (operators.size() > 1));
+        return arithmeticParent.operators.get(0).hasPrecedenceOver(this.operators.get(0))
+                || ((arithmeticParent.operators.size() > 1) || (operators.size() > 1));
     }
 
     @Override
@@ -94,11 +95,11 @@ public final class ArithmeticNode extends CompositeNode {
 
         // Apply in precedence order:
         Deque<ValueItem> stack = new ArrayDeque<>();
-        stack.push(new ValueItem(ArithmeticOperator.OR, child.next().evaluate(context)));
+        stack.push(new ValueItem(null, child.next().evaluate(context)));
         for (Iterator<ArithmeticOperator> it = operators.iterator(); it.hasNext() && child.hasNext();) {
             ArithmeticOperator op = it.next();
             if ( ! stack.isEmpty()) {
-                while (stack.peek().op.hasPrecedenceOver(op)) {
+                while (stack.size() > 1 && ! op.hasPrecedenceOver(stack.peek().op)) {
                     popStack(stack);
                 }
             }
@@ -127,9 +128,7 @@ public final class ArithmeticNode extends CompositeNode {
     public int hashCode() { return Objects.hash(children, operators); }
 
     public static ArithmeticNode resolve(ExpressionNode left, ArithmeticOperator op, ExpressionNode right) {
-        if ( ! (left instanceof ArithmeticNode)) return new ArithmeticNode(left, op, right);
-
-        ArithmeticNode leftArithmetic = (ArithmeticNode)left;
+        if ( ! (left instanceof ArithmeticNode leftArithmetic)) return new ArithmeticNode(left, op, right);
 
         List<ExpressionNode> newChildren = new ArrayList<>(leftArithmetic.children());
         newChildren.add(right);
@@ -149,6 +148,12 @@ public final class ArithmeticNode extends CompositeNode {
             this.op = op;
             this.value = value;
         }
+
+        @Override
+        public String toString() {
+            return value.toString();
+        }
+
     }
 
 }
diff --git a/searchlib/src/main/java/com/yahoo/searchlib/rankingexpression/rule/ArithmeticOperator.java b/searchlib/src/main/java/com/yahoo/searchlib/rankingexpression/rule/ArithmeticOperator.java
index ebeb7f8ac01..959045a63a0 100644
--- a/searchlib/src/main/java/com/yahoo/searchlib/rankingexpression/rule/ArithmeticOperator.java
+++ b/searchlib/src/main/java/com/yahoo/searchlib/rankingexpression/rule/ArithmeticOperator.java
@@ -3,8 +3,6 @@ package com.yahoo.searchlib.rankingexpression.rule;
 
 import com.yahoo.searchlib.rankingexpression.evaluation.Value;
 
-import java.util.ArrayList;
-import java.util.Collections;
 import java.util.List;
 
 /**
@@ -40,7 +38,7 @@ public enum ArithmeticOperator {
     }};
 
     /** A list of all the operators in this in order of decreasing precedence */
-    public static final List<ArithmeticOperator> operatorsByPrecedence = operatorsByPrecedence();
+    public static final List<ArithmeticOperator> operatorsByPrecedence = List.of(POWER, MODULO, DIVIDE, MULTIPLY, MINUS, PLUS, AND, OR);
 
     private final int precedence;
     private final String image;
@@ -62,17 +60,4 @@ public enum ArithmeticOperator {
         return image;
     }
 
-    private static List<ArithmeticOperator> operatorsByPrecedence() {
-        List<ArithmeticOperator> operators = new ArrayList<>();
-        operators.add(POWER);
-        operators.add(MODULO);
-        operators.add(DIVIDE);
-        operators.add(MULTIPLY);
-        operators.add(MINUS);
-        operators.add(PLUS);
-        operators.add(AND);
-        operators.add(OR);
-        return Collections.unmodifiableList(operators);
-    }
-
 }
diff --git a/searchlib/src/main/java/com/yahoo/searchlib/rankingexpression/transform/ExpressionTransformer.java b/searchlib/src/main/java/com/yahoo/searchlib/rankingexpression/transform/ExpressionTransformer.java
index 4aee3268111..e23c6ec5dd5 100644
--- a/searchlib/src/main/java/com/yahoo/searchlib/rankingexpression/transform/ExpressionTransformer.java
+++ b/searchlib/src/main/java/com/yahoo/searchlib/rankingexpression/transform/ExpressionTransformer.java
@@ -20,7 +20,11 @@ public abstract class ExpressionTransformer<CONTEXT extends TransformContext> {
         return new RankingExpression(expression.getName(), transform(expression.getRoot(), context));
     }
 
-    /** Transforms an expression node and returns the transformed node */
+    /**
+     * Transforms an expression node and returns the transformed node.
+     * This ic called with the root node of an expression to transform by clients of transformers.
+     * Transforming nested expression nodes are left to each transformer.
+     */
     public abstract ExpressionNode transform(ExpressionNode node, CONTEXT context);
 
     /**
diff --git a/searchlib/src/main/java/com/yahoo/searchlib/rankingexpression/transform/Simplifier.java b/searchlib/src/main/java/com/yahoo/searchlib/rankingexpression/transform/Simplifier.java
index c64f0eaa211..90861e64164 100644
--- a/searchlib/src/main/java/com/yahoo/searchlib/rankingexpression/transform/Simplifier.java
+++ b/searchlib/src/main/java/com/yahoo/searchlib/rankingexpression/transform/Simplifier.java
@@ -110,9 +110,8 @@ public class Simplifier extends ExpressionTransformer<TransformContext> {
     }
 
     private ExpressionNode transformNegativeNode(NegativeNode node) {
-        if ( ! (node.getValue() instanceof ConstantNode) ) return node;
+        if ( ! (node.getValue() instanceof ConstantNode constant) ) return node;
 
-        ConstantNode constant = (ConstantNode) node.getValue();
         if ( ! (constant.getValue() instanceof DoubleCompatibleValue)) return node;
         return new ConstantNode(constant.getValue().negate() );
     }
@@ -141,8 +140,7 @@ public class Simplifier extends ExpressionTransformer<TransformContext> {
     }
 
     private boolean isZero(ExpressionNode node) {
-        if ( ! (node instanceof ConstantNode)) return false;
-        ConstantNode constant = (ConstantNode)node;
+        if ( ! (node instanceof ConstantNode constant)) return false;
         if ( ! constant.getValue().hasDouble()) return false;
         return constant.getValue().asDouble() == 0.0;
     }
diff --git a/searchlib/src/test/java/com/yahoo/searchlib/rankingexpression/evaluation/EvaluationTestCase.java b/searchlib/src/test/java/com/yahoo/searchlib/rankingexpression/evaluation/EvaluationTestCase.java
index 19e32c23234..ad50a423eb9 100644
--- a/searchlib/src/test/java/com/yahoo/searchlib/rankingexpression/evaluation/EvaluationTestCase.java
+++ b/searchlib/src/test/java/com/yahoo/searchlib/rankingexpression/evaluation/EvaluationTestCase.java
@@ -56,6 +56,15 @@ public class EvaluationTestCase {
     }
 
     @Test
+    public void testEvaluationOrder() {
+        EvaluationTester tester = new EvaluationTester();
+        tester.assertEvaluates(-4, "1 + -2 + -3");
+        tester.assertEvaluates(2, "1 - (2 - 3)");
+        tester.assertEvaluates(-4, "(1 - 2) - 3");
+        tester.assertEvaluates(-4, "1 - 2 - 3");
+    }
+
+    @Test
     public void testEvaluation() {
         EvaluationTester tester = new EvaluationTester();
         tester.assertEvaluates(0.5, "0.5");
@@ -78,6 +87,7 @@ public class EvaluationTestCase {
         tester.assertEvaluates(3, "1 + 10 % 6 / 2");
         tester.assertEvaluates(10.0, "3 ^ 2 + 1");
         tester.assertEvaluates(18.0, "2 * 3 ^ 2");
+        tester.assertEvaluates(-4, "1 - 2 - 3"); // Means 1 + -2 + -3
 
         // Conditionals
         tester.assertEvaluates(2 * (3 * 4 + 3) * (4 * 5 - 4 * 200) / 10, "2*(3*4+3)*(4*5-4*200)/10");
@@ -106,7 +116,7 @@ public class EvaluationTestCase {
 
         // Conditionals with branch probabilities
         RankingExpression e = tester.assertEvaluates(3.5, "if(1.0-1.0, 2.5, 3.5, 0.3)");
-        assertEquals(0.3d, (double)((IfNode) e.getRoot()).getTrueProbability(), tolerance);
+        assertEquals(0.3d, ((IfNode) e.getRoot()).getTrueProbability(), tolerance);
 
         // Conditionals as expressions
         tester.assertEvaluates(new BooleanValue(true), "2<3");