diff --git a/docs/changelog/124424.yaml b/docs/changelog/124424.yaml new file mode 100644 index 0000000000000..b514803e22f62 --- /dev/null +++ b/docs/changelog/124424.yaml @@ -0,0 +1,5 @@ +pr: 124424 +summary: Lazy collection copying during node transform +area: ES|QL +type: bug +issues: [] diff --git a/x-pack/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/expression/NameId.java b/x-pack/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/expression/NameId.java index d2d01857a1f73..2ee600eb21e0b 100644 --- a/x-pack/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/expression/NameId.java +++ b/x-pack/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/expression/NameId.java @@ -12,7 +12,6 @@ import org.elasticsearch.xpack.esql.core.util.PlanStreamInput; import java.io.IOException; -import java.util.Objects; import java.util.concurrent.atomic.AtomicLong; /** @@ -34,7 +33,7 @@ public NameId() { @Override public int hashCode() { - return Objects.hash(id); + return Long.hashCode(id); } @Override diff --git a/x-pack/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/tree/Node.java b/x-pack/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/tree/Node.java index feeba39756373..667720536d884 100644 --- a/x-pack/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/tree/Node.java +++ b/x-pack/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/tree/Node.java @@ -110,7 +110,7 @@ public void forEachPropertyUp(Class typeToken, Consumer rule) protected void forEachProperty(Class typeToken, Consumer rule) { for (Object prop : info().properties()) { // skip children (only properties are interesting) - if (prop != children && children.contains(prop) == false && typeToken.isInstance(prop)) { + if (prop != children && typeToken.isInstance(prop) && children.contains(prop) == false) { rule.accept((E) prop); } } @@ -203,20 +203,21 @@ public T transformUp(Class typeToken, Function protected > T transformChildren(Function traversalOperation) { boolean childrenChanged = false; - // stream() could be used but the code is just as complicated without any advantages - // further more, it would include bring in all the associated stream/collector object creation even though in - // most cases the immediate tree would be quite small (0,1,2 elements) - List transformedChildren = new ArrayList<>(children().size()); + // Avoid creating a new array of children if no change is needed. + // And when it happens, look at using replacement to minimize the amount of method invocations. + List transformedChildren = null; - for (T child : children) { + for (int i = 0, s = children.size(); i < s; i++) { + T child = children.get(i); T next = traversalOperation.apply(child); - if (child.equals(next)) { - // use the initial value - next = child; - } else { - childrenChanged = true; + if (child.equals(next) == false) { + // lazy copy + replacement in place + if (childrenChanged == false) { + childrenChanged = true; + transformedChildren = new ArrayList<>(children); + } + transformedChildren.set(i, next); } - transformedChildren.add(next); } return (childrenChanged ? replaceChildrenSameSize(transformedChildren) : (T) this); diff --git a/x-pack/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/tree/NodeInfo.java b/x-pack/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/tree/NodeInfo.java index e8ce23bc20fd3..28e4e739085d4 100644 --- a/x-pack/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/tree/NodeInfo.java +++ b/x-pack/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/tree/NodeInfo.java @@ -52,7 +52,7 @@ final T transform(Function rule, Class typeToken) List children = node.children(); Function realRule = p -> { - if (p != children && false == children.contains(p) && (p == null || typeToken.isInstance(p))) { + if (p != children && (p == null || typeToken.isInstance(p)) && false == children.contains(p)) { return rule.apply(typeToken.cast(p)); } return p; diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/QueryPlan.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/QueryPlan.java index 02373cc62e81f..d70177508d847 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/QueryPlan.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/QueryPlan.java @@ -131,8 +131,8 @@ public PlanType transformExpressionsUp(Class typeToken @SuppressWarnings("unchecked") private static Object doTransformExpression(Object arg, Function traversal) { - if (arg instanceof Expression) { - return traversal.apply((Expression) arg); + if (arg instanceof Expression exp) { + return traversal.apply(exp); } // WARNING: if the collection is typed, an incompatible function will be applied to it @@ -141,17 +141,19 @@ private static Object doTransformExpression(Object arg, Function c) { - List transformed = new ArrayList<>(c.size()); + List transformed = null; boolean hasChanged = false; + int i = 0; for (Object e : c) { Object next = doTransformExpression(e, traversal); - if (e.equals(next)) { - // use the initial value - next = e; - } else { - hasChanged = true; + if (e.equals(next) == false) { + if (hasChanged == false) { + hasChanged = true; + transformed = new ArrayList<>(c); + } + transformed.set(i, next); } - transformed.add(next); + i++; } return hasChanged ? transformed : arg; @@ -186,8 +188,8 @@ public void forEachExpressionUp(Class typeToken, Consu @SuppressWarnings("unchecked") private static void doForEachExpression(Object arg, Consumer traversal) { - if (arg instanceof Expression) { - traversal.accept((Expression) arg); + if (arg instanceof Expression exp) { + traversal.accept(exp); } else if (arg instanceof Collection c) { for (Object o : c) { doForEachExpression(o, traversal); diff --git a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/NameId.java b/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/NameId.java index 20e8214ddef34..6fdb835064a28 100644 --- a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/NameId.java +++ b/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/NameId.java @@ -6,7 +6,6 @@ */ package org.elasticsearch.xpack.ql.expression; -import java.util.Objects; import java.util.concurrent.atomic.AtomicLong; /** @@ -28,7 +27,7 @@ public NameId() { @Override public int hashCode() { - return Objects.hash(id); + return Long.hashCode(id); } @Override diff --git a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/plan/QueryPlan.java b/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/plan/QueryPlan.java index fa38e87612d5d..231a8e00349b2 100644 --- a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/plan/QueryPlan.java +++ b/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/plan/QueryPlan.java @@ -109,8 +109,8 @@ public PlanType transformExpressionsUp(Class typeToken @SuppressWarnings("unchecked") private static Object doTransformExpression(Object arg, Function traversal) { - if (arg instanceof Expression) { - return traversal.apply((Expression) arg); + if (arg instanceof Expression exp) { + return traversal.apply(exp); } // WARNING: if the collection is typed, an incompatible function will be applied to it @@ -119,17 +119,19 @@ private static Object doTransformExpression(Object arg, Function c) { - List transformed = new ArrayList<>(c.size()); + List transformed = null; boolean hasChanged = false; + int i = 0; for (Object e : c) { Object next = doTransformExpression(e, traversal); - if (e.equals(next)) { - // use the initial value - next = e; - } else { - hasChanged = true; + if (e.equals(next) == false) { + if (hasChanged == false) { + hasChanged = true; + transformed = new ArrayList<>(c); + } + transformed.set(i, next); } - transformed.add(next); + i++; } return hasChanged ? transformed : arg; @@ -164,8 +166,8 @@ public void forEachExpressionUp(Class typeToken, Consu @SuppressWarnings("unchecked") private static void doForEachExpression(Object arg, Consumer traversal) { - if (arg instanceof Expression) { - traversal.accept((Expression) arg); + if (arg instanceof Expression exp) { + traversal.accept(exp); } else if (arg instanceof Collection c) { for (Object o : c) { doForEachExpression(o, traversal); diff --git a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/tree/Node.java b/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/tree/Node.java index cb0233429f323..b51b303eed14b 100644 --- a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/tree/Node.java +++ b/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/tree/Node.java @@ -109,7 +109,7 @@ public void forEachPropertyUp(Class typeToken, Consumer rule) protected void forEachProperty(Class typeToken, Consumer rule) { for (Object prop : info().properties()) { // skip children (only properties are interesting) - if (prop != children && children.contains(prop) == false && typeToken.isInstance(prop)) { + if (prop != children && typeToken.isInstance(prop) && children.contains(prop) == false) { rule.accept((E) prop); } } @@ -202,20 +202,21 @@ public T transformUp(Class typeToken, Function protected > T transformChildren(Function traversalOperation) { boolean childrenChanged = false; - // stream() could be used but the code is just as complicated without any advantages - // further more, it would include bring in all the associated stream/collector object creation even though in - // most cases the immediate tree would be quite small (0,1,2 elements) - List transformedChildren = new ArrayList<>(children().size()); + // Avoid creating a new array of children if no change is needed. + // And when it happens, look at using replacement to minimize the amount of method invocations. + List transformedChildren = null; - for (T child : children) { + for (int i = 0, s = children.size(); i < s; i++) { + T child = children.get(i); T next = traversalOperation.apply(child); - if (child.equals(next)) { - // use the initial value - next = child; - } else { - childrenChanged = true; + if (child.equals(next) == false) { + // lazy copy + replacement in place + if (childrenChanged == false) { + childrenChanged = true; + transformedChildren = new ArrayList<>(children); + } + transformedChildren.set(i, next); } - transformedChildren.add(next); } return (childrenChanged ? replaceChildrenSameSize(transformedChildren) : (T) this); diff --git a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/tree/NodeInfo.java b/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/tree/NodeInfo.java index a9782554eadbd..b355981fe957f 100644 --- a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/tree/NodeInfo.java +++ b/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/tree/NodeInfo.java @@ -52,7 +52,7 @@ final T transform(Function rule, Class typeToken) List children = node.children(); Function realRule = p -> { - if (p != children && false == children.contains(p) && (p == null || typeToken.isInstance(p))) { + if (p != children && (p == null || typeToken.isInstance(p)) && false == children.contains(p)) { return rule.apply(typeToken.cast(p)); } return p;