Use 3 value logic to determine which functions to push

elasticsearchmachine · elasticsearchmachine · commit 35c4d0e0e6cd · 2025-10-08T18:59:12.000+02:00
diff --git a/x-pack/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/expression/Expression.java b/x-pack/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/expression/Expression.java
@@ -212,11 +212,27 @@ public String propertiesToString(boolean skipIfChild) {
         return super.propertiesToString(false);
     }
 
-    public boolean isPushable() {
-        return false;
+    public PushableOptions pushableOptions() {
+        return PushableOptions.NOT_SUPPORTED;
     }
 
     public String asScript() {
         throw new UnsupportedOperationException("asScript not implemented for " + getWriteableName());
     }
+
+    public enum PushableOptions {
+        SUPPORTED,
+        NOT_SUPPORTED,
+        PREFERRED;
+
+        public PushableOptions and(PushableOptions other) {
+            if (this == NOT_SUPPORTED || other == NOT_SUPPORTED) {
+                return NOT_SUPPORTED;
+            }
+            if (this == PREFERRED || other == PREFERRED) {
+                return PREFERRED;
+            }
+            return SUPPORTED;
+        }
+    }
 }
diff --git a/x-pack/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/expression/FieldAttribute.java b/x-pack/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/expression/FieldAttribute.java
@@ -260,8 +260,8 @@ public EsField field() {
     }
 
     @Override
-    public boolean isPushable() {
-        return true;
+    public PushableOptions pushableOptions() {
+        return PushableOptions.SUPPORTED;
     }
 
     @Override
diff --git a/x-pack/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/expression/Literal.java b/x-pack/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/expression/Literal.java
@@ -238,8 +238,9 @@ private static long wkbAsLong(DataType dataType, BytesRef wkb) {
     }
 
     @Override
-    public boolean isPushable() {
-        return true;
+    public PushableOptions pushableOptions() {
+        // Don't mess with nulls, they don't have the same meaning in Lucene
+        return value() == null ? PushableOptions.NOT_SUPPORTED : PushableOptions.SUPPORTED;
     }
 
     @Override
diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/vector/Magnitude.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/vector/Magnitude.java
@@ -133,8 +133,8 @@ public String toString() {
     }
 
     @Override
-    public boolean isPushable() {
-        return true;
+    public PushableOptions pushableOptions() {
+        return PushableOptions.PREFERRED;
     }
 
     private record ScalarEvaluator(
diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/vector/VectorSimilarityFunction.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/vector/VectorSimilarityFunction.java
@@ -90,10 +90,12 @@ public final EvalOperator.ExpressionEvaluator.Factory toEvaluator(EvaluatorMappe
     }
 
     @Override
-    public boolean isPushable() {
+    public PushableOptions pushableOptions() {
         // Can only push if one of the arguments is a literal, and the other a field name
         return left().foldable() && left().dataType() != NULL && right() instanceof FieldAttribute
-            || right().foldable() && right().dataType() != NULL && left() instanceof FieldAttribute;
+            || right().foldable() && right().dataType() != NULL && left() instanceof FieldAttribute
+                ? PushableOptions.PREFERRED
+                : PushableOptions.NOT_SUPPORTED;
     }
 
     @Override
diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/predicate/operator/arithmetic/EsqlArithmeticOperation.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/predicate/operator/arithmetic/EsqlArithmeticOperation.java
@@ -165,8 +165,8 @@ public static String formatIncompatibleTypesMessage(String symbol, DataType left
     }
 
     @Override
-    public boolean isPushable() {
-        return left().isPushable() && right().isPushable();
+    public PushableOptions pushableOptions() {
+        return left().pushableOptions().and(right().pushableOptions());
     }
 
     @Override
diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/physical/local/PushScriptsToSource.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/physical/local/PushScriptsToSource.java
@@ -13,6 +13,7 @@
 import org.elasticsearch.search.sort.SortBuilder;
 import org.elasticsearch.search.sort.SortOrder;
 import org.elasticsearch.xpack.esql.core.expression.Alias;
+import org.elasticsearch.xpack.esql.core.expression.Expression;
 import org.elasticsearch.xpack.esql.core.expression.FieldAttribute;
 import org.elasticsearch.xpack.esql.core.type.DataType;
 import org.elasticsearch.xpack.esql.core.type.EsField;
@@ -38,8 +39,7 @@ protected PhysicalPlan rule(EvalExec evalExec, LocalPhysicalOptimizerContext ctx
             List<Alias> nonPushedAliases = new ArrayList<>();
             EvalExec scriptEval = null;
             for (Alias alias : aliases) {
-                if (aliasPushed == false && alias.child().isPushable()) {
-
+                if (aliasPushed == false && alias.child().pushableOptions() == Expression.PushableOptions.PREFERRED) {
                     if (esQueryExec.sorts() != null
                         && esQueryExec.sorts()
                             .stream()
diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/physical/local/PushTopNToSource.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/physical/local/PushTopNToSource.java
@@ -10,7 +10,6 @@
 import org.elasticsearch.geometry.Geometry;
 import org.elasticsearch.geometry.Point;
 import org.elasticsearch.script.Script;
-import org.elasticsearch.search.sort.ScriptSortBuilder;
 import org.elasticsearch.xpack.esql.core.expression.Alias;
 import org.elasticsearch.xpack.esql.core.expression.Attribute;
 import org.elasticsearch.xpack.esql.core.expression.AttributeMap;
@@ -20,8 +19,6 @@
 import org.elasticsearch.xpack.esql.core.expression.MetadataAttribute;
 import org.elasticsearch.xpack.esql.core.expression.NameId;
 import org.elasticsearch.xpack.esql.core.expression.ReferenceAttribute;
-import org.elasticsearch.xpack.esql.core.type.DataType;
-import org.elasticsearch.xpack.esql.core.type.EsField;
 import org.elasticsearch.xpack.esql.expression.Foldables;
 import org.elasticsearch.xpack.esql.expression.Order;
 import org.elasticsearch.xpack.esql.expression.function.scalar.spatial.BinarySpatialFunction;
@@ -133,27 +130,18 @@ private static PushableGeoDistance from(FoldContext ctx, Attribute attr, Express
     record PushableCompoundExec(EvalExec evalExec, EsQueryExec queryExec, List<EsQueryExec.Sort> pushableSorts) implements Pushable {
         public PhysicalPlan rewrite(TopNExec topNExec) {
             List<Alias> evalExecAlias = evalExec.fields();
-            List<FieldAttribute> additionalQueryAttrs = new ArrayList<>();
-            boolean aliasChanged = false;
             for (EsQueryExec.Sort pushableSort : pushableSorts) {
                 if (pushableSort instanceof EsQueryExec.ScriptSort scriptSort) {
                     // Change eval alias to the script sort field
                     for(int i = 0; i < evalExecAlias.size(); i++) {
                         Alias alias = evalExecAlias.get(i);
                         if (alias.id().equals(scriptSort.alias().id())) {
                             evalExecAlias.set(i, alias.replaceChild(scriptSort.field()));
-                            additionalQueryAttrs.add(scriptSort.field());
-                            aliasChanged = true;
                             break;
                         }
                     }
                 }
             }
-            if (aliasChanged) {
-                EsQueryExec newQueryExec = queryExec.withSorts(pushableSorts).withLimit(topNExec.limit());
-//                newQueryExec.attrs().addAll(additionalQueryAttrs);
-                EvalExec newEvalExec = new EvalExec(evalExec.source(), evalExec.child(), evalExecAlias);
-            }
 
             // We need to keep the EVAL in place because the coordinator will have its own TopNExec so we need to keep the distance
             return evalExec.replaceChild(queryExec.withSorts(pushableSorts).withLimit(topNExec.limit()));
@@ -191,7 +179,7 @@ && canPushLimit(topNExec, plannerSettings)) {
                     distances.put(alias.id(), distance);
                 } else if (alias.child() instanceof Attribute attr) {
                     aliasReplacedByBuilder.put(alias.toAttribute(), attr.toAttribute());
-                } else if (alias.child().isPushable()) {
+                } else if (alias.child().pushableOptions() == Expression.PushableOptions.PREFERRED) {
                     pushableExpressions.put(alias.id(), alias);
                 }
             });
diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/PhysicalPlanOptimizerTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/PhysicalPlanOptimizerTests.java
@@ -38,6 +38,7 @@
 import org.elasticsearch.search.sort.FieldSortBuilder;
 import org.elasticsearch.search.sort.GeoDistanceSortBuilder;
 import org.elasticsearch.test.ESTestCase;
+import org.elasticsearch.test.junit.annotations.TestLogging;
 import org.elasticsearch.xpack.core.enrich.EnrichPolicy;
 import org.elasticsearch.xpack.esql.EsqlTestUtils;
 import org.elasticsearch.xpack.esql.EsqlTestUtils.TestConfigurableSearchStats;
@@ -216,7 +217,7 @@
 import static org.hamcrest.Matchers.nullValue;
 import static org.hamcrest.Matchers.startsWith;
 
-// @TestLogging(value = "org.elasticsearch.xpack.esql:TRACE", reason = "debug")
+ @TestLogging(value = "org.elasticsearch.xpack.esql:TRACE", reason = "debug")
 public class PhysicalPlanOptimizerTests extends ESTestCase {
 
     private static final String PARAM_FORMATTING = "%1$s";

Original file line number	Diff line number	Diff line change
`@@ -260,8 +260,8 @@ public EsField field() {`
`260`	`260`	`}`
`261`	`261`
`262`	`262`	`@Override`
`263`		`- public boolean isPushable() {`
`264`		`- return true;`
	`263`	`+ public PushableOptions pushableOptions() {`
	`264`	`+ return PushableOptions.SUPPORTED;`
`265`	`265`	`}`
`266`	`266`
`267`	`267`	`@Override`
Original file line number	Diff line number	Diff line change
`@@ -238,8 +238,9 @@ private static long wkbAsLong(DataType dataType, BytesRef wkb) {`
`238`	`238`	`}`
`239`	`239`
`240`	`240`	`@Override`
`241`		`- public boolean isPushable() {`
`242`		`- return true;`
	`241`	`+ public PushableOptions pushableOptions() {`
	`242`	`+ // Don't mess with nulls, they don't have the same meaning in Lucene`
	`243`	`+ return value() == null ? PushableOptions.NOT_SUPPORTED : PushableOptions.SUPPORTED;`
`243`	`244`	`}`
`244`	`245`
`245`	`246`	`@Override`
Original file line number	Diff line number	Diff line change
`@@ -133,8 +133,8 @@ public String toString() {`
`133`	`133`	`}`
`134`	`134`
`135`	`135`	`@Override`
`136`		`- public boolean isPushable() {`
`137`		`- return true;`
	`136`	`+ public PushableOptions pushableOptions() {`
	`137`	`+ return PushableOptions.PREFERRED;`
`138`	`138`	`}`
`139`	`139`
`140`	`140`	`private record ScalarEvaluator(`
Original file line number	Diff line number	Diff line change
`@@ -90,10 +90,12 @@ public final EvalOperator.ExpressionEvaluator.Factory toEvaluator(EvaluatorMappe`
`90`	`90`	`}`
`91`	`91`
`92`	`92`	`@Override`
`93`		`- public boolean isPushable() {`
	`93`	`+ public PushableOptions pushableOptions() {`
`94`	`94`	`// Can only push if one of the arguments is a literal, and the other a field name`
`95`	`95`	`return left().foldable() && left().dataType() != NULL && right() instanceof FieldAttribute`
`96`		`- \|\| right().foldable() && right().dataType() != NULL && left() instanceof FieldAttribute;`
	`96`	`+ \|\| right().foldable() && right().dataType() != NULL && left() instanceof FieldAttribute`
	`97`	`+ ? PushableOptions.PREFERRED`
	`98`	`+ : PushableOptions.NOT_SUPPORTED;`
`97`	`99`	`}`
`98`	`100`
`99`	`101`	`@Override`
Original file line number	Diff line number	Diff line change
`@@ -165,8 +165,8 @@ public static String formatIncompatibleTypesMessage(String symbol, DataType left`
`165`	`165`	`}`
`166`	`166`
`167`	`167`	`@Override`
`168`		`- public boolean isPushable() {`
`169`		`- return left().isPushable() && right().isPushable();`
	`168`	`+ public PushableOptions pushableOptions() {`
	`169`	`+ return left().pushableOptions().and(right().pushableOptions());`
`170`	`170`	`}`
`171`	`171`
`172`	`172`	`@Override`