Skip to content

Pre-size collections #126382

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Apr 8, 2025
Merged

Pre-size collections #126382

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
34d1db9
Pre-size collections
idegtiarenko Apr 7, 2025
b799d1c
Merge branch 'main' into presize_collections
idegtiarenko Apr 7, 2025
3a747fc
remove unused map
idegtiarenko Apr 7, 2025
8bec883
Merge branch 'main' into presize_collections
idegtiarenko Apr 8, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions ...rc/main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/CombineProjections.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ private static List<NamedExpression> combineProjections(List<? extends NamedExpr
// collect named expressions declaration in the lower list
AttributeMap.Builder<NamedExpression> namedExpressionsBuilder = AttributeMap.builder();
// while also collecting the alias map for resolving the source (f1 = 1, f2 = f1, etc..)
AttributeMap.Builder<Expression> aliasesBuilder = AttributeMap.builder();
AttributeMap.Builder<Expression> aliasesBuilder = AttributeMap.builder(lower.size());
for (NamedExpression ne : lower) {
// record the alias
aliasesBuilder.put(ne.toAttribute(), Alias.unwrap(ne));
Expand All @@ -128,7 +128,7 @@ private static List<NamedExpression> combineProjections(List<? extends NamedExpr
namedExpressionsBuilder.put(ne.toAttribute(), as.replaceChild(aliasesBuilder.build().resolve(child, child)));
}
}
List<NamedExpression> replaced = new ArrayList<>();
List<NamedExpression> replaced = new ArrayList<>(upper.size());
var namedExpressions = namedExpressionsBuilder.build();

// replace any matching attribute with a lower alias (if there's a match)
Expand Down
12 changes: 8 additions & 4 deletions x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/planner/Layout.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,14 +7,14 @@

package org.elasticsearch.xpack.esql.planner;

import org.elasticsearch.common.util.Maps;
import org.elasticsearch.xpack.esql.core.expression.NameId;
import org.elasticsearch.xpack.esql.core.expression.NamedExpression;
import org.elasticsearch.xpack.esql.core.type.DataType;

import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Set;
Expand Down Expand Up @@ -65,7 +65,7 @@ class Builder {
private final List<ChannelSet> channels;

public Builder() {
channels = new ArrayList<>();
this(new ArrayList<>());
}

Builder(List<ChannelSet> channels) {
Expand All @@ -76,7 +76,7 @@ public Builder() {
* Appends a new channel to the layout. The channel is mapped to one or more attribute ids.
*/
public Builder append(ChannelSet set) {
if (set.nameIds.size() < 1) {
if (set.nameIds.isEmpty()) {
throw new IllegalArgumentException("Channel must be mapped to at least one id.");
}
channels.add(set);
Expand Down Expand Up @@ -104,7 +104,11 @@ public Builder append(Collection<? extends NamedExpression> attributes) {
* Build a new {@link Layout}.
*/
public Layout build() {
Map<NameId, ChannelAndType> layout = new HashMap<>();
int size = 0;
for (ChannelSet channel : channels) {
size += channel.nameIds.size();
}
Map<NameId, ChannelAndType> layout = Maps.newHashMapWithExpectedSize(size);
int numberOfChannels = 0;
for (ChannelSet set : channels) {
int channel = numberOfChannels++;
Expand Down
14 changes: 4 additions & 10 deletions ...plugin/esql/src/main/java/org/elasticsearch/xpack/esql/planner/LocalExecutionPlanner.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
import org.elasticsearch.common.lucene.BytesRefs;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.common.util.BigArrays;
import org.elasticsearch.common.util.Maps;
import org.elasticsearch.compute.Describable;
import org.elasticsearch.compute.aggregation.AggregatorMode;
import org.elasticsearch.compute.data.Block;
Expand Down Expand Up @@ -734,16 +735,9 @@ private PhysicalOperation planProject(ProjectExec project, LocalExecutionPlanner
List<Integer> projectionList = new ArrayList<>(projections.size());

Layout.Builder layout = new Layout.Builder();
Map<Integer, Layout.ChannelSet> inputChannelToOutputIds = new HashMap<>();
for (int index = 0, size = projections.size(); index < size; index++) {
NamedExpression ne = projections.get(index);

NameId inputId = null;
if (ne instanceof Alias a) {
inputId = ((NamedExpression) a.child()).id();
} else {
inputId = ne.id();
}
Map<Integer, Layout.ChannelSet> inputChannelToOutputIds = Maps.newHashMapWithExpectedSize(projections.size());
Copy link
Contributor

@ivancea ivancea Apr 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We aren't using this collection from what I see? We're just doing a .get(), but never adding things

Copy link
Contributor Author

@idegtiarenko idegtiarenko Apr 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, why do we even spend time and resources populating it then?

Copy link
Contributor Author

@idegtiarenko idegtiarenko Apr 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder, since that map is not used, does it mean we might duplicate actual data in the actual layout?

Copy link
Contributor

@alex-spies alex-spies Apr 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch!

I think this map is unused for forever now, and people found this redundant map multiple times. I was sure one of the times, we actually had merged the fix...

I'm not sure how the channel sets were intended to be used for project, but the current project operator does shallow copies of blocks, and this works well and is intended. It didn't always do this, we had to first make it work by making blocks refcounted. Maybe before that, the channel sets were supposed to help with deduplication.

for (NamedExpression ne : projections) {
NameId inputId = ne instanceof Alias a ? ((NamedExpression) a.child()).id() : ne.id();
Layout.ChannelAndType input = source.layout.get(inputId);
if (input == null) {
throw new IllegalStateException("can't find input for [" + ne + "]");
Expand Down