Skip to content

Commit d360e00

Browse files
costincostin
authored
ESQL: Lazy collection copying during node transform (elastic#124424) (elastic#124528)
* ESQL: Lazy collection copying during node transform A set of optimization for tree traversal: 1. perform lazy copying during children transform 2. use long hashing to avoid object creation 3. perform type check first before collection checking Relates elastic#124395
1 parent 91a163f commit d360e00

File tree

9 files changed

+61
-52
lines changed

9 files changed

+61
-52
lines changed

‎docs/changelog/124424.yaml‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
pr: 124424
2+
summary: Lazy collection copying during node transform
3+
area: ES|QL
4+
type: bug
5+
issues: []

‎x-pack/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/expression/NameId.java‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,6 @@
1212
import org.elasticsearch.xpack.esql.core.util.PlanStreamInput;
1313

1414
import java.io.IOException;
15-
import java.util.Objects;
1615
import java.util.concurrent.atomic.AtomicLong;
1716

1817
/**
@@ -34,7 +33,7 @@ public NameId() {
3433

3534
@Override
3635
public int hashCode() {
37-
return Objects.hash(id);
36+
return Long.hashCode(id);
3837
}
3938

4039
@Override

‎x-pack/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/tree/Node.java‎

Lines changed: 13 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -110,7 +110,7 @@ public <E> void forEachPropertyUp(Class<E> typeToken, Consumer<? super E> rule)
110110
protected <E> void forEachProperty(Class<E> typeToken, Consumer<? super E> rule) {
111111
for (Object prop : info().properties()) {
112112
// skip children (only properties are interesting)
113-
if (prop != children && children.contains(prop) == false && typeToken.isInstance(prop)) {
113+
if (prop != children && typeToken.isInstance(prop) && children.contains(prop) == false) {
114114
rule.accept((E) prop);
115115
}
116116
}
@@ -203,20 +203,21 @@ public <E extends T> T transformUp(Class<E> typeToken, Function<E, ? extends T>
203203
protected <R extends Function<? super T, ? extends T>> T transformChildren(Function<T, ? extends T> traversalOperation) {
204204
boolean childrenChanged = false;
205205

206-
// stream() could be used but the code is just as complicated without any advantages
207-
// further more, it would include bring in all the associated stream/collector object creation even though in
208-
// most cases the immediate tree would be quite small (0,1,2 elements)
209-
List<T> transformedChildren = new ArrayList<>(children().size());
206+
// Avoid creating a new array of children if no change is needed.
207+
// And when it happens, look at using replacement to minimize the amount of method invocations.
208+
List<T> transformedChildren = null;
210209

211-
for (T child : children) {
210+
for (int i = 0, s = children.size(); i < s; i++) {
211+
T child = children.get(i);
212212
T next = traversalOperation.apply(child);
213-
if (child.equals(next)) {
214-
// use the initial value
215-
next = child;
216-
} else {
217-
childrenChanged = true;
213+
if (child.equals(next) == false) {
214+
// lazy copy + replacement in place
215+
if (childrenChanged == false) {
216+
childrenChanged = true;
217+
transformedChildren = new ArrayList<>(children);
218+
}
219+
transformedChildren.set(i, next);
218220
}
219-
transformedChildren.add(next);
220221
}
221222

222223
return (childrenChanged ? replaceChildrenSameSize(transformedChildren) : (T) this);

‎x-pack/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/tree/NodeInfo.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,7 @@ final <E> T transform(Function<? super E, ? extends E> rule, Class<E> typeToken)
5252
List<?> children = node.children();
5353

5454
Function<Object, Object> realRule = p -> {
55-
if (p != children && false == children.contains(p) && (p == null || typeToken.isInstance(p))) {
55+
if (p != children && (p == null || typeToken.isInstance(p)) && false == children.contains(p)) {
5656
return rule.apply(typeToken.cast(p));
5757
}
5858
return p;

‎x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/QueryPlan.java‎

Lines changed: 13 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -131,8 +131,8 @@ public <E extends Expression> PlanType transformExpressionsUp(Class<E> typeToken
131131

132132
@SuppressWarnings("unchecked")
133133
private static Object doTransformExpression(Object arg, Function<Expression, ? extends Expression> traversal) {
134-
if (arg instanceof Expression) {
135-
return traversal.apply((Expression) arg);
134+
if (arg instanceof Expression exp) {
135+
return traversal.apply(exp);
136136
}
137137

138138
// 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<Expression, ? e
141141
// has no type info so it's difficult to have automatic checking without having base classes).
142142

143143
if (arg instanceof Collection<?> c) {
144-
List<Object> transformed = new ArrayList<>(c.size());
144+
List<Object> transformed = null;
145145
boolean hasChanged = false;
146+
int i = 0;
146147
for (Object e : c) {
147148
Object next = doTransformExpression(e, traversal);
148-
if (e.equals(next)) {
149-
// use the initial value
150-
next = e;
151-
} else {
152-
hasChanged = true;
149+
if (e.equals(next) == false) {
150+
if (hasChanged == false) {
151+
hasChanged = true;
152+
transformed = new ArrayList<>(c);
153+
}
154+
transformed.set(i, next);
153155
}
154-
transformed.add(next);
156+
i++;
155157
}
156158

157159
return hasChanged ? transformed : arg;
@@ -186,8 +188,8 @@ public <E extends Expression> void forEachExpressionUp(Class<E> typeToken, Consu
186188

187189
@SuppressWarnings("unchecked")
188190
private static void doForEachExpression(Object arg, Consumer<Expression> traversal) {
189-
if (arg instanceof Expression) {
190-
traversal.accept((Expression) arg);
191+
if (arg instanceof Expression exp) {
192+
traversal.accept(exp);
191193
} else if (arg instanceof Collection<?> c) {
192194
for (Object o : c) {
193195
doForEachExpression(o, traversal);

‎x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/NameId.java‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,6 @@
66
*/
77
package org.elasticsearch.xpack.ql.expression;
88

9-
import java.util.Objects;
109
import java.util.concurrent.atomic.AtomicLong;
1110

1211
/**
@@ -28,7 +27,7 @@ public NameId() {
2827

2928
@Override
3029
public int hashCode() {
31-
return Objects.hash(id);
30+
return Long.hashCode(id);
3231
}
3332

3433
@Override

‎x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/plan/QueryPlan.java‎

Lines changed: 13 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -109,8 +109,8 @@ public <E extends Expression> PlanType transformExpressionsUp(Class<E> typeToken
109109

110110
@SuppressWarnings("unchecked")
111111
private static Object doTransformExpression(Object arg, Function<Expression, ? extends Expression> traversal) {
112-
if (arg instanceof Expression) {
113-
return traversal.apply((Expression) arg);
112+
if (arg instanceof Expression exp) {
113+
return traversal.apply(exp);
114114
}
115115

116116
// 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<Expression, ? e
119119
// has no type info so it's difficult to have automatic checking without having base classes).
120120

121121
if (arg instanceof Collection<?> c) {
122-
List<Object> transformed = new ArrayList<>(c.size());
122+
List<Object> transformed = null;
123123
boolean hasChanged = false;
124+
int i = 0;
124125
for (Object e : c) {
125126
Object next = doTransformExpression(e, traversal);
126-
if (e.equals(next)) {
127-
// use the initial value
128-
next = e;
129-
} else {
130-
hasChanged = true;
127+
if (e.equals(next) == false) {
128+
if (hasChanged == false) {
129+
hasChanged = true;
130+
transformed = new ArrayList<>(c);
131+
}
132+
transformed.set(i, next);
131133
}
132-
transformed.add(next);
134+
i++;
133135
}
134136

135137
return hasChanged ? transformed : arg;
@@ -164,8 +166,8 @@ public <E extends Expression> void forEachExpressionUp(Class<E> typeToken, Consu
164166

165167
@SuppressWarnings("unchecked")
166168
private static void doForEachExpression(Object arg, Consumer<Expression> traversal) {
167-
if (arg instanceof Expression) {
168-
traversal.accept((Expression) arg);
169+
if (arg instanceof Expression exp) {
170+
traversal.accept(exp);
169171
} else if (arg instanceof Collection<?> c) {
170172
for (Object o : c) {
171173
doForEachExpression(o, traversal);

‎x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/tree/Node.java‎

Lines changed: 13 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -109,7 +109,7 @@ public <E> void forEachPropertyUp(Class<E> typeToken, Consumer<? super E> rule)
109109
protected <E> void forEachProperty(Class<E> typeToken, Consumer<? super E> rule) {
110110
for (Object prop : info().properties()) {
111111
// skip children (only properties are interesting)
112-
if (prop != children && children.contains(prop) == false && typeToken.isInstance(prop)) {
112+
if (prop != children && typeToken.isInstance(prop) && children.contains(prop) == false) {
113113
rule.accept((E) prop);
114114
}
115115
}
@@ -202,20 +202,21 @@ public <E extends T> T transformUp(Class<E> typeToken, Function<E, ? extends T>
202202
protected <R extends Function<? super T, ? extends T>> T transformChildren(Function<T, ? extends T> traversalOperation) {
203203
boolean childrenChanged = false;
204204

205-
// stream() could be used but the code is just as complicated without any advantages
206-
// further more, it would include bring in all the associated stream/collector object creation even though in
207-
// most cases the immediate tree would be quite small (0,1,2 elements)
208-
List<T> transformedChildren = new ArrayList<>(children().size());
205+
// Avoid creating a new array of children if no change is needed.
206+
// And when it happens, look at using replacement to minimize the amount of method invocations.
207+
List<T> transformedChildren = null;
209208

210-
for (T child : children) {
209+
for (int i = 0, s = children.size(); i < s; i++) {
210+
T child = children.get(i);
211211
T next = traversalOperation.apply(child);
212-
if (child.equals(next)) {
213-
// use the initial value
214-
next = child;
215-
} else {
216-
childrenChanged = true;
212+
if (child.equals(next) == false) {
213+
// lazy copy + replacement in place
214+
if (childrenChanged == false) {
215+
childrenChanged = true;
216+
transformedChildren = new ArrayList<>(children);
217+
}
218+
transformedChildren.set(i, next);
217219
}
218-
transformedChildren.add(next);
219220
}
220221

221222
return (childrenChanged ? replaceChildrenSameSize(transformedChildren) : (T) this);

‎x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/tree/NodeInfo.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,7 @@ final <E> T transform(Function<? super E, ? extends E> rule, Class<E> typeToken)
5252
List<?> children = node.children();
5353

5454
Function<Object, Object> realRule = p -> {
55-
if (p != children && false == children.contains(p) && (p == null || typeToken.isInstance(p))) {
55+
if (p != children && (p == null || typeToken.isInstance(p)) && false == children.contains(p)) {
5656
return rule.apply(typeToken.cast(p));
5757
}
5858
return p;

0 commit comments

Comments
 (0)