Use ArithmeticNode.resolve instead of creating a new one explicit.

2 files changed, 68 insertions, 11 deletions

diff --git a/config-model/src/main/java/com/yahoo/schema/expressiontransforms/BooleanExpressionTransformer.java b/config-model/src/main/java/com/yahoo/schema/expressiontransforms/BooleanExpressionTransformer.java
index 0d3f289b824..8fa4b469590 100644
--- a/config-model/src/main/java/com/yahoo/schema/expressiontransforms/BooleanExpressionTransformer.java
+++ b/config-model/src/main/java/com/yahoo/schema/expressiontransforms/BooleanExpressionTransformer.java
@@ -15,6 +15,7 @@ import java.util.ArrayDeque;
 import java.util.Deque;
 import java.util.Iterator;
 import java.util.List;
+import java.util.ArrayList;
 
 /**
  * Transforms
@@ -69,11 +70,39 @@ public class BooleanExpressionTransformer extends ExpressionTransformer<Transfor
             combination = andByIfNode(lhs.child, rhs.child);
         else if (rhs.op == ArithmeticOperator.OR)
             combination = orByIfNode(lhs.child, rhs.child);
-        else
-            combination = new ArithmeticNode(List.of(lhs.child, rhs.child), List.of(rhs.op));
+        else {
+            combination = resolve(lhs, rhs);
+            lhs.artificial = true;
+        }
         lhs.child = combination;
     }
 
+    private static ArithmeticNode resolve(ChildNode left, ChildNode right) {
+        if ( ! (left.child instanceof ArithmeticNode) && ! (right.child instanceof ArithmeticNode))
+            return new ArithmeticNode(left.child, right.op, right.child);
+
+        // Collapse inserted ArithmeticNodes
+        List<ArithmeticOperator> joinedOps = new ArrayList<>();
+        joinOps(left, joinedOps);
+        joinedOps.add(right.op);
+        joinOps(right, joinedOps);
+        List<ExpressionNode> joinedChildren = new ArrayList<>();
+        joinChildren(left, joinedChildren);
+        joinChildren(right, joinedChildren);
+        return new ArithmeticNode(joinedChildren, joinedOps);
+    }
+
+    private static void joinOps(ChildNode node, List<ArithmeticOperator> joinedOps) {
+        if (node.artificial && node.child instanceof ArithmeticNode arithmeticNode)
+            joinedOps.addAll(arithmeticNode.operators());
+    }
+    private static void joinChildren(ChildNode node, List<ExpressionNode> joinedChildren) {
+        if (node.artificial && node.child instanceof ArithmeticNode arithmeticNode)
+            joinedChildren.addAll(arithmeticNode.children());
+        else
+            joinedChildren.add(node.child);
+    }
+
 
     private IfNode andByIfNode(ExpressionNode a, ExpressionNode b) {
         return new IfNode(a, b, new ConstantNode(new BooleanValue(false)));
@@ -88,6 +117,7 @@ public class BooleanExpressionTransformer extends ExpressionTransformer<Transfor
 
         final ArithmeticOperator op;
         ExpressionNode child;
+        boolean artificial;
 
         public ChildNode(ArithmeticOperator op, ExpressionNode child) {
             this.op = op;
diff --git a/config-model/src/test/java/com/yahoo/schema/expressiontransforms/BooleanExpressionTransformerTestCase.java b/config-model/src/test/java/com/yahoo/schema/expressiontransforms/BooleanExpressionTransformerTestCase.java
index 71d0657c701..01e4d4ce232 100644
--- a/config-model/src/test/java/com/yahoo/schema/expressiontransforms/BooleanExpressionTransformerTestCase.java
+++ b/config-model/src/test/java/com/yahoo/schema/expressiontransforms/BooleanExpressionTransformerTestCase.java
@@ -4,12 +4,16 @@ package com.yahoo.schema.expressiontransforms;
 import com.yahoo.searchlib.rankingexpression.RankingExpression;
 import com.yahoo.searchlib.rankingexpression.evaluation.MapContext;
 import com.yahoo.searchlib.rankingexpression.evaluation.MapTypeContext;
+import com.yahoo.searchlib.rankingexpression.rule.ArithmeticNode;
 import com.yahoo.searchlib.rankingexpression.transform.TransformContext;
 import org.junit.jupiter.api.Test;
-import static org.junit.jupiter.api.Assertions.assertEquals;
 
 import java.util.Map;
 
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+
 /**
  * @author bratseth
  */
@@ -33,6 +37,37 @@ public class BooleanExpressionTransformerTestCase {
         assertTransformed("if(1 - 1, true, 1 - 1)", "1 - 1 || 1 - 1");
     }
 
+    @Test
+    public void testNotSkewingNonBoolean() throws Exception {
+        assertTransformed("a + b + c * d + e + f", "a + b + c * d + e + f");
+        var expr = new BooleanExpressionTransformer()
+                .transform(new RankingExpression("a + b + c * d + e + f"),
+                        new TransformContext(Map.of(), new MapTypeContext()));
+        assertTrue(expr.getRoot() instanceof ArithmeticNode);
+        ArithmeticNode root = (ArithmeticNode) expr.getRoot();
+        assertEquals(5, root.operators().size());
+        assertEquals(6, root.children().size());
+    }
+
+    @Test
+    public void testTransformPreservesPrecedence() throws Exception {
+        assertUnTransformed("a");
+        assertUnTransformed("a + b");
+        assertUnTransformed("a + b + c");
+        assertUnTransformed("a * b");
+        assertUnTransformed("a + b * c + d");
+        assertUnTransformed("a + b + c * d + e + f");
+        assertUnTransformed("a * b + c + d + e * f");
+        assertUnTransformed("(a * b) + c + d + e * f");
+        assertUnTransformed("(a * b + c) + d + e * f");
+        assertUnTransformed("a * (b + c) + d + e * f");
+        assertUnTransformed("(a * b) + (c + (d + e)) * f");
+    }
+
+    private void assertUnTransformed(String input) throws Exception {
+        assertTransformed(input, input);
+    }
+
     private void assertTransformed(String expected, String input) throws Exception {
         var transformedExpression = new BooleanExpressionTransformer()
                                             .transform(new RankingExpression(input),
@@ -40,18 +75,10 @@ public class BooleanExpressionTransformerTestCase {
 
         assertEquals(new RankingExpression(expected), transformedExpression, "Transformed as expected");
 
-        MapContext context = contextWithSingleLetterVariables();
         var inputExpression = new RankingExpression(input);
         assertEquals(inputExpression.evaluate(new MapContext()).asBoolean(),
                      transformedExpression.evaluate(new MapContext()).asBoolean(),
                      "Transform and original input are equivalent");
     }
 
-    private MapContext contextWithSingleLetterVariables() {
-        var context = new MapContext();
-        for (int i = 0; i < 26; i++)
-            context.put(Character.toString(i + 97), Math.floorMod(i, 2));
-        return context;
-    }
-
 }