Simplify and fix scoring issues - scoring does not matter when an expression is not true

Author: carlosdelest

x-pack/plugin/esql/compute/src/main/java/org/elasticsearch/compute/operator/EvalOperator.java

@@ -63,14 +63,9 @@ public void close() {
     public interface ExpressionEvaluator extends Releasable {
 
         /**
-         * Scoring value when an expression does not match (return false)
+         * Scoring value when an expression does not match (return false) or does not contribute to the score
          */
-        double NO_MATCH_SCORE = -1.0;
-
-        /**
-         * Default score for expressions that match (return true) but are not full text functions
-         */
-        double MATCH_SCORE = 0.0;
+        double NO_MATCH_SCORE = 0.0;
 
         /** A Factory for creating ExpressionEvaluators. */
         interface Factory {
@@ -96,10 +91,10 @@ default boolean eagerEvalSafeInLazy() {
 
         /**
          * Retrieves the score for the expression.
-         * Only expressions that can be scored, or that can combine scores, should override this method
+         * Only expressions that can be scored, or that can combine scores, should override this method.
          */
         default DoubleBlock score(Page page, BlockFactory blockFactory) {
-            return blockFactory.newConstantDoubleBlockWith(MATCH_SCORE, page.getPositionCount());
+            return blockFactory.newConstantDoubleBlockWith(NO_MATCH_SCORE, page.getPositionCount());
         }
     }
 

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/evaluator/EvalMapper.java

@@ -14,7 +14,6 @@
 import org.elasticsearch.compute.data.BooleanBlock;
 import org.elasticsearch.compute.data.BooleanVector;
 import org.elasticsearch.compute.data.DoubleBlock;
-import org.elasticsearch.compute.data.DoubleVector;
 import org.elasticsearch.compute.data.ElementType;
 import org.elasticsearch.compute.data.Page;
 import org.elasticsearch.compute.data.Vector;
@@ -27,10 +26,8 @@
 import org.elasticsearch.xpack.esql.core.expression.Expression;
 import org.elasticsearch.xpack.esql.core.expression.FoldContext;
 import org.elasticsearch.xpack.esql.core.expression.Literal;
-import org.elasticsearch.xpack.esql.core.tree.Source;
 import org.elasticsearch.xpack.esql.evaluator.mapper.EvaluatorMapper;
 import org.elasticsearch.xpack.esql.evaluator.mapper.ExpressionMapper;
-import org.elasticsearch.xpack.esql.evaluator.predicate.operator.logical.NotEvaluator;
 import org.elasticsearch.xpack.esql.expression.predicate.logical.BinaryLogic;
 import org.elasticsearch.xpack.esql.expression.predicate.logical.Not;
 import org.elasticsearch.xpack.esql.expression.predicate.nulls.IsNotNull;
@@ -215,43 +212,12 @@ public ExpressionEvaluator.Factory map(
             List<ShardContext> shardContexts,
             boolean usesScoring
         ) {
-            record NotScoreEvaluator(Source source, NotEvaluator notEval, ExpressionEvaluator innerEval, DriverContext driverContext)
-                implements
-                    ExpressionEvaluator {
-                @Override
-                public Block eval(Page page) {
-                    return notEval.eval(page);
-                }
-
-                @Override
-                // NotEvaluator is a final, generated class - create this record to override score method
-                public DoubleBlock score(Page page, BlockFactory blockFactory) {
-                    try (DoubleBlock scoreBlock = innerEval.score(page, blockFactory)) {
-                        DoubleVector scoreVector = scoreBlock.asVector();
-                        DoubleVector.Builder result = blockFactory.newDoubleVectorFixedBuilder(page.getPositionCount());
-                        // TODO We could optimize for constant vectors
-                        for (int i = 0; i < scoreVector.getPositionCount(); i++) {
-                            result.appendDouble(scoreVector.getDouble(i) == NO_MATCH_SCORE ? MATCH_SCORE : NO_MATCH_SCORE);
-                        }
-                        return result.build().asBlock();
-                    }
-                }
-
-                @Override
-                public void close() {
-                    Releasables.closeExpectNoException(notEval, innerEval);
-                }
-            }
-
-            return driverContext -> {
-                var expEval = toEvaluator(foldCtx, not.field(), layout, shardContexts, usesScoring);
-                ExpressionEvaluator innerEval = expEval.get(driverContext);
-                NotEvaluator notEvaluator = new NotEvaluator(not.source(), innerEval, driverContext);
-                if (usesScoring) {
-                    return new NotScoreEvaluator(not.source(), notEvaluator, innerEval, driverContext);
-                }
-                return notEvaluator;
-            };
+            var expEval = toEvaluator(foldCtx, not.field(), layout);
+            return dvrCtx -> new org.elasticsearch.xpack.esql.evaluator.predicate.operator.logical.NotEvaluator(
+                not.source(),
+                expEval.get(dvrCtx),
+                dvrCtx
+            );
         }
     }
 
@@ -400,22 +366,6 @@ public Block eval(Page page) {
                 }
             }
 
-            @Override
-            public DoubleBlock score(Page page, BlockFactory blockFactory) {
-                // TODO We could skip re-evaluating the field if we store the result of eval()
-                try (Block fieldBlock = field.eval(page)) {
-                    if (fieldBlock.asVector() != null) {
-                        return driverContext.blockFactory().newConstantDoubleBlockWith(NO_MATCH_SCORE, page.getPositionCount());
-                    }
-                    try (var builder = driverContext.blockFactory().newDoubleVectorFixedBuilder(page.getPositionCount())) {
-                        for (int p = 0; p < page.getPositionCount(); p++) {
-                            builder.appendDouble(p, fieldBlock.isNull(p) ? MATCH_SCORE : NO_MATCH_SCORE);
-                        }
-                        return builder.build().asBlock();
-                    }
-                }
-            }
-
             @Override
             public void close() {
                 Releasables.closeExpectNoException(field);
@@ -471,22 +421,6 @@ public Block eval(Page page) {
                 }
             }
 
-            @Override
-            public DoubleBlock score(Page page, BlockFactory blockFactory) {
-                // TODO We could skip re-evaluating the field if we store the result of eval()
-                try (Block fieldBlock = field.eval(page)) {
-                    if (fieldBlock.asVector() != null) {
-                        return driverContext.blockFactory().newConstantDoubleBlockWith(MATCH_SCORE, page.getPositionCount());
-                    }
-                    try (var builder = driverContext.blockFactory().newDoubleVectorFixedBuilder(page.getPositionCount())) {
-                        for (int p = 0; p < page.getPositionCount(); p++) {
-                            builder.appendDouble(p, fieldBlock.isNull(p) ? NO_MATCH_SCORE : MATCH_SCORE);
-                        }
-                        return builder.build().asBlock();
-                    }
-                }
-            }
-
             @Override
             public void close() {
                 Releasables.closeExpectNoException(field);

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/predicate/logical/BinaryLogicOperation.java

@@ -11,8 +11,6 @@
 
 import java.util.function.BiFunction;
 
-import static org.elasticsearch.compute.operator.EvalOperator.ExpressionEvaluator.NO_MATCH_SCORE;
-
 public enum BinaryLogicOperation implements PredicateBiFunction<Boolean, Boolean, Boolean> {
 
     AND((l, r) -> {
@@ -23,11 +21,6 @@ public enum BinaryLogicOperation implements PredicateBiFunction<Boolean, Boolean
             return null;
         }
         return Boolean.logicalAnd(l.booleanValue(), r.booleanValue());
-    }, (leftScore, rightScore) -> {
-        if (NO_MATCH_SCORE == leftScore || NO_MATCH_SCORE == rightScore) {
-            return NO_MATCH_SCORE;
-        }
-        return leftScore + rightScore;
     }, "AND"),
     OR((l, r) -> {
         if (Boolean.TRUE.equals(l) || Boolean.TRUE.equals(r)) {
@@ -37,20 +30,13 @@ public enum BinaryLogicOperation implements PredicateBiFunction<Boolean, Boolean
             return null;
         }
         return Boolean.logicalOr(l.booleanValue(), r.booleanValue());
-    }, (leftScore, rightScore) -> {
-        if (NO_MATCH_SCORE == leftScore || NO_MATCH_SCORE == rightScore) {
-            return Math.max(leftScore, rightScore);
-        }
-        return leftScore + rightScore;
     }, "OR");
 
     private final BiFunction<Boolean, Boolean, Boolean> process;
-    private final BiFunction<Double, Double, Double> scoreFunction;
     private final String symbol;
 
-    BinaryLogicOperation(BiFunction<Boolean, Boolean, Boolean> process, BiFunction<Double, Double, Double> scoreFunction, String symbol) {
+    BinaryLogicOperation(BiFunction<Boolean, Boolean, Boolean> process, String symbol) {
         this.process = process;
-        this.scoreFunction = scoreFunction;
         this.symbol = symbol;
     }
 
@@ -65,7 +51,7 @@ public Boolean apply(Boolean left, Boolean right) {
     }
 
     public Double score(Double leftScore, Double rightScore) {
-        return scoreFunction.apply(leftScore, rightScore);
+        return leftScore + rightScore;
     }
 
     @Override