From dbe03f9454e02cc1537585ddabfb8437792e905d Mon Sep 17 00:00:00 2001 From: Andras Palinkas Date: Wed, 4 Nov 2020 19:00:46 +0000 Subject: [PATCH 1/6] SQL: Removed all the `AttributeMap(Map)` usage --- .../xpack/ql/expression/AttributeMap.java | 27 ++------ .../ql/expression/AttributeMapTests.java | 65 ++++++------------- .../xpack/sql/optimizer/Optimizer.java | 10 +-- .../xpack/sql/plan/logical/Pivot.java | 24 ++++--- .../xpack/sql/planner/QueryFolder.java | 21 +++--- .../querydsl/container/QueryContainer.java | 13 ++-- .../search/SourceGeneratorTests.java | 11 +--- .../container/QueryContainerTests.java | 8 +-- 8 files changed, 62 insertions(+), 117 deletions(-) diff --git a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/AttributeMap.java b/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/AttributeMap.java index 3fc545d735d80..109208bd8027a 100644 --- a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/AttributeMap.java +++ b/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/AttributeMap.java @@ -15,7 +15,6 @@ import java.util.function.BiConsumer; import java.util.stream.Stream; -import static java.util.Collections.emptyMap; import static java.util.Collections.singletonMap; import static java.util.Collections.unmodifiableCollection; import static java.util.Collections.unmodifiableSet; @@ -141,8 +140,8 @@ public String toString() { } @SuppressWarnings("rawtypes") - public static final AttributeMap EMPTY = new AttributeMap<>(); - + private static final AttributeMap EMPTY = new AttributeMap<>(); + @SuppressWarnings("unchecked") public static final AttributeMap emptyAttributeMap() { return EMPTY; @@ -157,23 +156,6 @@ public AttributeMap() { delegate = new LinkedHashMap<>(); } - /** - * Please use the {@link AttributeMap#builder()} instead. - */ - @Deprecated - public AttributeMap(Map attr) { - if (attr.isEmpty()) { - delegate = emptyMap(); - } - else { - delegate = new LinkedHashMap<>(attr.size()); - - for (Entry entry : attr.entrySet()) { - delegate.put(new AttributeWrapper(entry.getKey()), entry.getValue()); - } - } - } - public AttributeMap(Attribute key, E value) { delegate = singletonMap(new AttributeWrapper(key), value); } @@ -393,7 +375,10 @@ public Builder putAll(AttributeMap m) { } public AttributeMap build() { - return new AttributeMap<>(map); + // copy, in case someone would do a .build, .put, .build sequence + AttributeMap m = new AttributeMap<>(); + m.addAll(map); + return m; } } } diff --git a/x-pack/plugin/ql/src/test/java/org/elasticsearch/xpack/ql/expression/AttributeMapTests.java b/x-pack/plugin/ql/src/test/java/org/elasticsearch/xpack/ql/expression/AttributeMapTests.java index 41ea7fd44d7ba..e947ca460d8b8 100644 --- a/x-pack/plugin/ql/src/test/java/org/elasticsearch/xpack/ql/expression/AttributeMapTests.java +++ b/x-pack/plugin/ql/src/test/java/org/elasticsearch/xpack/ql/expression/AttributeMapTests.java @@ -10,7 +10,6 @@ import org.elasticsearch.xpack.ql.type.DataTypes; import java.util.Collection; -import java.util.LinkedHashMap; import java.util.List; import java.util.Map; import java.util.Map.Entry; @@ -30,12 +29,12 @@ private static Attribute a(String name) { } private static AttributeMap threeMap() { - Map map = new LinkedHashMap<>(); - map.put(a("one"), "one"); - map.put(a("two"), "two"); - map.put(a("three"), "three"); + AttributeMap.Builder builder = AttributeMap.builder(); + builder.put(a("one"), "one"); + builder.put(a("two"), "two"); + builder.put(a("three"), "three"); - return new AttributeMap<>(map); + return builder.build(); } public void testAttributeMapWithSameAliasesCanResolveAttributes() { @@ -49,19 +48,6 @@ public void testAttributeMapWithSameAliasesCanResolveAttributes() { assertTrue(param1.toAttribute().equals(param2.toAttribute())); assertFalse(param1.toAttribute().semanticEquals(param2.toAttribute())); - Map collectRefs = new LinkedHashMap<>(); - for (Alias a : List.of(param1, param2)) { - collectRefs.put(a.toAttribute(), a.child()); - } - // we can look up the same item by both attributes - assertNotNull(collectRefs.get(param1.toAttribute())); - assertNotNull(collectRefs.get(param2.toAttribute())); - AttributeMap attributeMap = new AttributeMap<>(collectRefs); - - // validate that all Alias can be e - assertTrue(attributeMap.containsKey(param1.toAttribute())); - assertFalse(attributeMap.containsKey(param2.toAttribute())); // results in unknown attribute exception - AttributeMap.Builder mapBuilder = AttributeMap.builder(); for (Alias a : List.of(param1, param2)) { mapBuilder.put(a.toAttribute(), a.child()); @@ -69,7 +55,9 @@ public void testAttributeMapWithSameAliasesCanResolveAttributes() { AttributeMap newAttributeMap = mapBuilder.build(); assertTrue(newAttributeMap.containsKey(param1.toAttribute())); - assertTrue(newAttributeMap.containsKey(param2.toAttribute())); // no more unknown attribute exception + assertTrue(newAttributeMap.get(param1.toAttribute()) == param1.child()); + assertTrue(newAttributeMap.containsKey(param2.toAttribute())); + assertTrue(newAttributeMap.get(param2.toAttribute()) == param2.child()); } private Alias createIntParameterAlias(int index, int value) { @@ -85,13 +73,13 @@ public void testEmptyConstructor() { assertThat(m.isEmpty(), is(true)); } - public void testMapConstructor() { - Map map = new LinkedHashMap<>(); - map.put(a("one"), "one"); - map.put(a("two"), "two"); - map.put(a("three"), "three"); + public void testBuilder() { + AttributeMap.Builder builder = AttributeMap.builder(); + builder.put(a("one"), "one"); + builder.put(a("two"), "two"); + builder.put(a("three"), "three"); - AttributeMap m = new AttributeMap<>(map); + AttributeMap m = builder.build(); assertThat(m.size(), is(3)); assertThat(m.isEmpty(), is(false)); @@ -101,10 +89,10 @@ public void testMapConstructor() { assertThat(m.containsValue("one"), is(true)); assertThat(m.containsValue("on"), is(false)); assertThat(m.attributeNames(), contains("one", "two", "three")); - assertThat(m.values(), contains(map.values().toArray())); + assertThat(m.values(), contains("one", "two", "three")); // defensive copying - map.put(a("four"), "four"); + builder.put(a("four"), "four"); assertThat(m.size(), is(3)); assertThat(m.isEmpty(), is(false)); } @@ -164,12 +152,7 @@ public void testKeySet() { Attribute two = a("two"); Attribute three = a("three"); - Map map = new LinkedHashMap<>(); - map.put(one, "one"); - map.put(two, "two"); - map.put(three, "three"); - - Set keySet = new AttributeMap<>(map).keySet(); + Set keySet = threeMap().keySet(); assertThat(keySet, contains(one, two, three)); // toObject @@ -192,12 +175,7 @@ public void testEntrySet() { Attribute two = a("two"); Attribute three = a("three"); - Map map = new LinkedHashMap<>(); - map.put(one, "one"); - map.put(two, "two"); - map.put(three, "three"); - - Set> set = new AttributeMap<>(map).entrySet(); + Set> set = threeMap().entrySet(); assertThat(set, hasSize(3)); @@ -211,12 +189,9 @@ public void testEntrySet() { assertThat(values, contains("one", "two", "three")); } - public void testForEach() { + public void testCopy() { AttributeMap m = threeMap(); - - Map collect = new LinkedHashMap<>(); - m.forEach(collect::put); - AttributeMap copy = new AttributeMap<>(collect); + AttributeMap copy = AttributeMap.builder().putAll(m).build(); assertThat(m, is(copy)); } diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/optimizer/Optimizer.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/optimizer/Optimizer.java index 5744d6fbee43d..be4333d55fb3f 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/optimizer/Optimizer.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/optimizer/Optimizer.java @@ -302,7 +302,7 @@ protected LogicalPlan rule(Project project) { OrderBy ob = (OrderBy) project.child(); // resolve function references (that maybe hiding the target) - final Map collectRefs = new LinkedHashMap<>(); + AttributeMap.Builder collectRefs = AttributeMap.builder(); // collect Attribute sources // only Aliases are interesting since these are the only ones that hide expressions @@ -316,7 +316,7 @@ protected LogicalPlan rule(Project project) { } })); - AttributeMap functions = new AttributeMap<>(collectRefs); + AttributeMap functions = collectRefs.build(); // track the direct parents Map nestedOrders = new LinkedHashMap<>(); @@ -541,14 +541,14 @@ private List combineProjections(List //TODO: this need rewriting when moving functions of NamedExpression // collect aliases in the lower list - Map map = new LinkedHashMap<>(); + AttributeMap.Builder aliasesBuilder = AttributeMap.builder(); for (NamedExpression ne : lower) { if ((ne instanceof Attribute) == false) { - map.put(ne.toAttribute(), ne); + aliasesBuilder.put(ne.toAttribute(), ne); } } - AttributeMap aliases = new AttributeMap<>(map); + AttributeMap aliases = aliasesBuilder.build(); List replaced = new ArrayList<>(); // replace any matching attribute with a lower alias (if there's a match) diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plan/logical/Pivot.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plan/logical/Pivot.java index 55cec8d80a6fd..479a17e2dbdf0 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plan/logical/Pivot.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plan/logical/Pivot.java @@ -23,9 +23,7 @@ import org.elasticsearch.xpack.sql.SqlIllegalArgumentException; import java.util.ArrayList; -import java.util.LinkedHashMap; import java.util.List; -import java.util.Map; import java.util.Objects; import static java.util.Collections.singletonList; @@ -45,14 +43,14 @@ public class Pivot extends UnaryPlan { public Pivot(Source source, LogicalPlan child, Expression column, List values, List aggregates) { this(source, child, column, values, aggregates, null); } - + public Pivot(Source source, LogicalPlan child, Expression column, List values, List aggregates, List grouping) { super(source, child); this.column = column; this.values = values; this.aggregates = aggregates; - + // resolve the grouping set ASAP so it doesn't get re-resolved after analysis (since the aliasing information has been removed) if (grouping == null && expressionsResolved()) { AttributeSet columnSet = Expressions.references(singletonList(column)); @@ -90,11 +88,11 @@ public List values() { public List aggregates() { return aggregates; } - + public List groupings() { return grouping; } - + public AttributeSet groupingSet() { if (groupingSet == null) { throw new SqlIllegalArgumentException("Cannot determine grouping in unresolved PIVOT"); @@ -131,10 +129,10 @@ private AttributeSet valuesOutput() { } return valueOutput; } - + public AttributeMap valuesToLiterals() { AttributeSet outValues = valuesOutput(); - Map valuesMap = new LinkedHashMap<>(); + AttributeMap.Builder valuesMap = AttributeMap.builder(); int index = 0; // for each attribute, associate its value @@ -152,7 +150,7 @@ public AttributeMap valuesToLiterals() { } } - return new AttributeMap<>(valuesMap); + return valuesMap.build(); } @Override @@ -183,21 +181,21 @@ public boolean expressionsResolved() { public int hashCode() { return Objects.hash(column, values, aggregates, child()); } - + @Override public boolean equals(Object obj) { if (this == obj) { return true; } - + if (obj == null || getClass() != obj.getClass()) { return false; } - + Pivot other = (Pivot) obj; return Objects.equals(column, other.column) && Objects.equals(values, other.values) && Objects.equals(aggregates, other.aggregates) && Objects.equals(child(), other.child()); } -} \ No newline at end of file +} diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/planner/QueryFolder.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/planner/QueryFolder.java index 9165e1b5d035c..2851e29f2c509 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/planner/QueryFolder.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/planner/QueryFolder.java @@ -92,6 +92,7 @@ import java.util.Map; import java.util.Map.Entry; import java.util.concurrent.atomic.AtomicReference; +import java.util.stream.Collectors; import static org.elasticsearch.xpack.ql.util.CollectionUtils.combine; import static org.elasticsearch.xpack.sql.expression.function.grouping.Histogram.DAY_INTERVAL; @@ -421,20 +422,20 @@ static EsQueryExec fold(AggregateExec a, EsQueryExec exec) { // track aliases defined in the SELECT and used inside GROUP BY // SELECT x AS a ... GROUP BY a - Map aliasMap = new LinkedHashMap<>(); String id = null; - for (NamedExpression ne : a.aggregates()) { - if (ne instanceof Alias) { - aliasMap.put(ne.toAttribute(), ((Alias) ne).child()); - } - } - if (aliasMap.isEmpty() == false) { - Map newAliases = new LinkedHashMap<>(queryC.aliases()); - newAliases.putAll(aliasMap); - queryC = queryC.withAliases(new AttributeMap<>(newAliases)); + List aliases = a.aggregates().stream() + .filter(ne -> ne instanceof Alias) + .map(ne -> (Alias)ne) + .collect(Collectors.toList()); + + if (aliases.isEmpty() == false) { + AttributeMap.Builder aliasMapBuilder = AttributeMap.builder().putAll(queryC.aliases()); + aliases.forEach(alias -> aliasMapBuilder.put(alias.toAttribute(), alias.child())); + queryC = queryC.withAliases(aliasMapBuilder.build()); } + // build the group aggregation // NB: any reference in grouping is already "optimized" by its source so there's no need to look for aliases GroupingContext groupingContext = groupBy(a.groupings()); diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/container/QueryContainer.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/container/QueryContainer.java index a2761db533f93..526551a43e1d4 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/container/QueryContainer.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/container/QueryContainer.java @@ -171,7 +171,7 @@ public List> sortingColumns() { sortingColumns.add(new Tuple<>(Integer.valueOf(atIndex), comp)); } - + return sortingColumns; } @@ -331,7 +331,7 @@ private FieldExtraction topHitFieldRef(FieldAttribute fieldAttr) { FieldAttribute actualField = fieldAttr; FieldAttribute rootField = fieldAttr; StringBuilder fullFieldName = new StringBuilder(fieldAttr.field().getName()); - + // Only if the field is not an alias (in which case it will be taken out from docvalue_fields if it's isAggregatable()), // go up the tree of parents until a non-object (and non-nested) type of field is found and use that specific parent // as the field to extract data from, from _source. We do it like this because sub-fields are not in the _source, only @@ -366,8 +366,8 @@ private FieldExtraction topHitFieldRef(FieldAttribute fieldAttr) { private Tuple nestedHitFieldRef(FieldAttribute attr) { String name = aliasName(attr); Query q = rewriteToContainNestedField(query, attr.source(), - attr.nestedParent().name(), name, - SqlDataTypes.format(attr.field().getDataType()), + attr.nestedParent().name(), name, + SqlDataTypes.format(attr.field().getDataType()), SqlDataTypes.isFromDocValuesOnly(attr.field().getDataType())); SearchHitFieldRef nestedFieldRef = new SearchHitFieldRef(name, null, attr.field().getDataType(), attr.field().isAggregatable(), @@ -432,9 +432,8 @@ public FieldExtraction resolve(Attribute attribute) { // update proc (if needed) if (qContainer.scalarFunctions().size() != scalarFunctions.size()) { - Map procs = new LinkedHashMap<>(qContainer.scalarFunctions()); - procs.put(attr, proc); - qContainer = qContainer.withScalarProcessors(new AttributeMap<>(procs)); + qContainer = qContainer.withScalarProcessors( + AttributeMap.builder().putAll(qContainer.scalarFunctions).put(attr, proc).build()); } return new Tuple<>(qContainer, new ComputedRef(proc)); diff --git a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/execution/search/SourceGeneratorTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/execution/search/SourceGeneratorTests.java index 6bee4b338659b..ace6678ac0b32 100644 --- a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/execution/search/SourceGeneratorTests.java +++ b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/execution/search/SourceGeneratorTests.java @@ -14,9 +14,7 @@ import org.elasticsearch.search.sort.FieldSortBuilder; import org.elasticsearch.search.sort.SortOrder; import org.elasticsearch.test.ESTestCase; -import org.elasticsearch.xpack.ql.expression.Attribute; import org.elasticsearch.xpack.ql.expression.AttributeMap; -import org.elasticsearch.xpack.ql.expression.Expression; import org.elasticsearch.xpack.ql.expression.FieldAttribute; import org.elasticsearch.xpack.ql.expression.ReferenceAttribute; import org.elasticsearch.xpack.ql.querydsl.container.AttributeSort; @@ -32,9 +30,6 @@ import org.elasticsearch.xpack.sql.querydsl.container.QueryContainer; import org.elasticsearch.xpack.sql.querydsl.container.ScoreSort; -import java.util.LinkedHashMap; -import java.util.Map; - import static java.util.Collections.singletonList; import static org.elasticsearch.index.query.QueryBuilders.boolQuery; import static org.elasticsearch.index.query.QueryBuilders.matchQuery; @@ -89,9 +84,7 @@ public void testSortNoneSpecified() { public void testSelectScoreForcesTrackingScore() { Score score = new Score(Source.EMPTY); ReferenceAttribute attr = new ReferenceAttribute(score.source(), "score", score.dataType()); - Map alias = new LinkedHashMap<>(); - alias.put(attr, score); - QueryContainer container = new QueryContainer().withAliases(new AttributeMap<>(alias)).addColumn(attr); + QueryContainer container = new QueryContainer().withAliases(new AttributeMap<>(attr, score)).addColumn(attr); SearchSourceBuilder sourceBuilder = SourceGenerator.sourceBuilder(container, null, randomIntBetween(1, 10)); assertTrue(sourceBuilder.trackScores()); } @@ -105,7 +98,7 @@ public void testSortScoreSpecified() { public void testSortFieldSpecified() { FieldSortBuilder sortField = fieldSort("test").unmappedType("keyword"); - + QueryContainer container = new QueryContainer() .addSort("id", new AttributeSort(new FieldAttribute(Source.EMPTY, "test", new KeywordEsField("test")), Direction.ASC, Missing.LAST)); diff --git a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/querydsl/container/QueryContainerTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/querydsl/container/QueryContainerTests.java index 8886807ad5840..8b7cc542d93b9 100644 --- a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/querydsl/container/QueryContainerTests.java +++ b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/querydsl/container/QueryContainerTests.java @@ -9,7 +9,6 @@ import org.elasticsearch.xpack.ql.expression.Alias; import org.elasticsearch.xpack.ql.expression.Attribute; import org.elasticsearch.xpack.ql.expression.AttributeMap; -import org.elasticsearch.xpack.ql.expression.Expression; import org.elasticsearch.xpack.ql.expression.FieldAttribute; import org.elasticsearch.xpack.ql.querydsl.query.BoolQuery; import org.elasticsearch.xpack.ql.querydsl.query.MatchAll; @@ -24,8 +23,6 @@ import java.util.AbstractMap.SimpleImmutableEntry; import java.util.Arrays; import java.util.BitSet; -import java.util.LinkedHashMap; -import java.util.Map; import static java.util.Collections.emptyMap; import static java.util.Collections.singletonMap; @@ -86,11 +83,8 @@ public void testColumnMaskShouldDuplicateSameAttributes() { Attribute fourth = new FieldAttribute(Source.EMPTY, "fourth", esField); Alias firstAliased = new Alias(Source.EMPTY, "firstAliased", first); - Map aliasesMap = new LinkedHashMap<>(); - aliasesMap.put(firstAliased.toAttribute(), first); - QueryContainer queryContainer = new QueryContainer() - .withAliases(new AttributeMap<>(aliasesMap)) + .withAliases(new AttributeMap<>(firstAliased.toAttribute(), first)) .addColumn(third) .addColumn(first) .addColumn(fourth) From ff70ca7f9968596e9241045a6b4b3bdf764f4369 Mon Sep 17 00:00:00 2001 From: Andras Palinkas Date: Thu, 5 Nov 2020 17:37:11 +0000 Subject: [PATCH 2/6] Fix the AttributeSet type inference --- .../org/elasticsearch/xpack/ql/expression/AttributeSet.java | 6 ++---- .../org/elasticsearch/xpack/ql/expression/Expressions.java | 3 +-- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/AttributeSet.java b/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/AttributeSet.java index 585d3d5da10c9..1553579ae351c 100644 --- a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/AttributeSet.java +++ b/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/AttributeSet.java @@ -13,11 +13,9 @@ import java.util.function.Predicate; import java.util.stream.Stream; -import static java.util.Collections.emptyMap; - public class AttributeSet implements Set { - private static final AttributeMap EMPTY_DELEGATE = new AttributeMap<>(emptyMap()); + private static final AttributeMap EMPTY_DELEGATE = AttributeMap.emptyAttributeMap(); public static final AttributeSet EMPTY = new AttributeSet(EMPTY_DELEGATE); @@ -185,4 +183,4 @@ public int hashCode() { public String toString() { return delegate.keySet().toString(); } -} \ No newline at end of file +} diff --git a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/Expressions.java b/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/Expressions.java index 3f716d760654b..c5ce1cea5f54b 100644 --- a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/Expressions.java +++ b/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/Expressions.java @@ -21,7 +21,6 @@ import java.util.function.Predicate; import static java.util.Collections.emptyList; -import static java.util.Collections.emptyMap; public final class Expressions { @@ -65,7 +64,7 @@ public static List asAttributes(List named public static AttributeMap asAttributeMap(List named) { if (named.isEmpty()) { - return new AttributeMap<>(emptyMap()); + return AttributeMap.emptyAttributeMap(); } AttributeMap map = new AttributeMap<>(); From a46db5822b51034f7b34f2ad8023b80f4749a8dc Mon Sep 17 00:00:00 2001 From: Andras Palinkas Date: Thu, 5 Nov 2020 20:28:22 +0000 Subject: [PATCH 3/6] PR suggestion and whitespace reverts --- .../xpack/ql/expression/AttributeMap.java | 10 +++++++++- .../xpack/ql/expression/AttributeSet.java | 2 +- .../xpack/sql/plan/logical/Pivot.java | 18 +++++++++--------- .../xpack/sql/planner/QueryFolder.java | 17 +++++++++-------- .../sql/querydsl/container/QueryContainer.java | 8 ++++---- .../execution/search/SourceGeneratorTests.java | 2 +- 6 files changed, 33 insertions(+), 24 deletions(-) diff --git a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/AttributeMap.java b/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/AttributeMap.java index 109208bd8027a..ab58740f0c400 100644 --- a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/AttributeMap.java +++ b/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/AttributeMap.java @@ -141,7 +141,7 @@ public String toString() { @SuppressWarnings("rawtypes") private static final AttributeMap EMPTY = new AttributeMap<>(); - + @SuppressWarnings("unchecked") public static final AttributeMap emptyAttributeMap() { return EMPTY; @@ -359,6 +359,10 @@ public static Builder builder() { return new Builder<>(); } + public static Builder builder(AttributeMap map) { + return new Builder().putAll(map); + } + public static class Builder { private final AttributeMap map = new AttributeMap<>(); @@ -374,6 +378,10 @@ public Builder putAll(AttributeMap m) { return this; } + public boolean isEmpty() { + return map.isEmpty(); + } + public AttributeMap build() { // copy, in case someone would do a .build, .put, .build sequence AttributeMap m = new AttributeMap<>(); diff --git a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/AttributeSet.java b/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/AttributeSet.java index 1553579ae351c..3c46f42e80450 100644 --- a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/AttributeSet.java +++ b/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/AttributeSet.java @@ -183,4 +183,4 @@ public int hashCode() { public String toString() { return delegate.keySet().toString(); } -} +} \ No newline at end of file diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plan/logical/Pivot.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plan/logical/Pivot.java index 479a17e2dbdf0..fb6cb91d03e3e 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plan/logical/Pivot.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plan/logical/Pivot.java @@ -43,14 +43,14 @@ public class Pivot extends UnaryPlan { public Pivot(Source source, LogicalPlan child, Expression column, List values, List aggregates) { this(source, child, column, values, aggregates, null); } - + public Pivot(Source source, LogicalPlan child, Expression column, List values, List aggregates, List grouping) { super(source, child); this.column = column; this.values = values; this.aggregates = aggregates; - + // resolve the grouping set ASAP so it doesn't get re-resolved after analysis (since the aliasing information has been removed) if (grouping == null && expressionsResolved()) { AttributeSet columnSet = Expressions.references(singletonList(column)); @@ -88,11 +88,11 @@ public List values() { public List aggregates() { return aggregates; } - + public List groupings() { return grouping; } - + public AttributeSet groupingSet() { if (groupingSet == null) { throw new SqlIllegalArgumentException("Cannot determine grouping in unresolved PIVOT"); @@ -129,7 +129,7 @@ private AttributeSet valuesOutput() { } return valueOutput; } - + public AttributeMap valuesToLiterals() { AttributeSet outValues = valuesOutput(); AttributeMap.Builder valuesMap = AttributeMap.builder(); @@ -181,21 +181,21 @@ public boolean expressionsResolved() { public int hashCode() { return Objects.hash(column, values, aggregates, child()); } - + @Override public boolean equals(Object obj) { if (this == obj) { return true; } - + if (obj == null || getClass() != obj.getClass()) { return false; } - + Pivot other = (Pivot) obj; return Objects.equals(column, other.column) && Objects.equals(values, other.values) && Objects.equals(aggregates, other.aggregates) && Objects.equals(child(), other.child()); } -} +} \ No newline at end of file diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/planner/QueryFolder.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/planner/QueryFolder.java index 2851e29f2c509..196307c93ddc6 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/planner/QueryFolder.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/planner/QueryFolder.java @@ -92,7 +92,6 @@ import java.util.Map; import java.util.Map.Entry; import java.util.concurrent.atomic.AtomicReference; -import java.util.stream.Collectors; import static org.elasticsearch.xpack.ql.util.CollectionUtils.combine; import static org.elasticsearch.xpack.sql.expression.function.grouping.Histogram.DAY_INTERVAL; @@ -424,15 +423,17 @@ static EsQueryExec fold(AggregateExec a, EsQueryExec exec) { // SELECT x AS a ... GROUP BY a String id = null; - List aliases = a.aggregates().stream() - .filter(ne -> ne instanceof Alias) - .map(ne -> (Alias)ne) - .collect(Collectors.toList()); + AttributeMap.Builder aliases = AttributeMap.builder(); + for (NamedExpression ne : a.aggregates()) { + if (ne instanceof Alias) { + Alias alias = (Alias) ne; + aliases.put(alias.toAttribute(), alias.child()); + } + } if (aliases.isEmpty() == false) { - AttributeMap.Builder aliasMapBuilder = AttributeMap.builder().putAll(queryC.aliases()); - aliases.forEach(alias -> aliasMapBuilder.put(alias.toAttribute(), alias.child())); - queryC = queryC.withAliases(aliasMapBuilder.build()); + aliases.putAll(queryC.aliases()); + queryC = queryC.withAliases(aliases.build()); } diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/container/QueryContainer.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/container/QueryContainer.java index 526551a43e1d4..4684b4dfbb90d 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/container/QueryContainer.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/container/QueryContainer.java @@ -171,7 +171,7 @@ public List> sortingColumns() { sortingColumns.add(new Tuple<>(Integer.valueOf(atIndex), comp)); } - + return sortingColumns; } @@ -331,7 +331,7 @@ private FieldExtraction topHitFieldRef(FieldAttribute fieldAttr) { FieldAttribute actualField = fieldAttr; FieldAttribute rootField = fieldAttr; StringBuilder fullFieldName = new StringBuilder(fieldAttr.field().getName()); - + // Only if the field is not an alias (in which case it will be taken out from docvalue_fields if it's isAggregatable()), // go up the tree of parents until a non-object (and non-nested) type of field is found and use that specific parent // as the field to extract data from, from _source. We do it like this because sub-fields are not in the _source, only @@ -366,8 +366,8 @@ private FieldExtraction topHitFieldRef(FieldAttribute fieldAttr) { private Tuple nestedHitFieldRef(FieldAttribute attr) { String name = aliasName(attr); Query q = rewriteToContainNestedField(query, attr.source(), - attr.nestedParent().name(), name, - SqlDataTypes.format(attr.field().getDataType()), + attr.nestedParent().name(), name, + SqlDataTypes.format(attr.field().getDataType()), SqlDataTypes.isFromDocValuesOnly(attr.field().getDataType())); SearchHitFieldRef nestedFieldRef = new SearchHitFieldRef(name, null, attr.field().getDataType(), attr.field().isAggregatable(), diff --git a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/execution/search/SourceGeneratorTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/execution/search/SourceGeneratorTests.java index ace6678ac0b32..3ef98e6fff235 100644 --- a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/execution/search/SourceGeneratorTests.java +++ b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/execution/search/SourceGeneratorTests.java @@ -98,7 +98,7 @@ public void testSortScoreSpecified() { public void testSortFieldSpecified() { FieldSortBuilder sortField = fieldSort("test").unmappedType("keyword"); - + QueryContainer container = new QueryContainer() .addSort("id", new AttributeSort(new FieldAttribute(Source.EMPTY, "test", new KeywordEsField("test")), Direction.ASC, Missing.LAST)); From 62f01c1b491d8d70f7bf5d671e8ce1b1ffa922f1 Mon Sep 17 00:00:00 2001 From: Andras Palinkas Date: Thu, 5 Nov 2020 23:26:28 +0000 Subject: [PATCH 4/6] Missed PR suggestion --- .../xpack/sql/querydsl/container/QueryContainer.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/container/QueryContainer.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/container/QueryContainer.java index 4684b4dfbb90d..64e4245b557ce 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/container/QueryContainer.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/container/QueryContainer.java @@ -433,7 +433,7 @@ public FieldExtraction resolve(Attribute attribute) { // update proc (if needed) if (qContainer.scalarFunctions().size() != scalarFunctions.size()) { qContainer = qContainer.withScalarProcessors( - AttributeMap.builder().putAll(qContainer.scalarFunctions).put(attr, proc).build()); + AttributeMap.builder(qContainer.scalarFunctions).put(attr, proc).build()); } return new Tuple<>(qContainer, new ComputedRef(proc)); From 60185351d40df3492910c235133b30d8e2c52d74 Mon Sep 17 00:00:00 2001 From: Andras Palinkas Date: Fri, 6 Nov 2020 14:06:31 +0000 Subject: [PATCH 5/6] PR suggestions --- .../org/elasticsearch/xpack/ql/expression/AttributeMap.java | 4 ---- .../org/elasticsearch/xpack/sql/planner/QueryFolder.java | 5 ++--- 2 files changed, 2 insertions(+), 7 deletions(-) diff --git a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/AttributeMap.java b/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/AttributeMap.java index ab58740f0c400..7490af4a64b0e 100644 --- a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/AttributeMap.java +++ b/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/AttributeMap.java @@ -378,10 +378,6 @@ public Builder putAll(AttributeMap m) { return this; } - public boolean isEmpty() { - return map.isEmpty(); - } - public AttributeMap build() { // copy, in case someone would do a .build, .put, .build sequence AttributeMap m = new AttributeMap<>(); diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/planner/QueryFolder.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/planner/QueryFolder.java index 196307c93ddc6..86da3948eaf03 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/planner/QueryFolder.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/planner/QueryFolder.java @@ -426,12 +426,11 @@ static EsQueryExec fold(AggregateExec a, EsQueryExec exec) { AttributeMap.Builder aliases = AttributeMap.builder(); for (NamedExpression ne : a.aggregates()) { if (ne instanceof Alias) { - Alias alias = (Alias) ne; - aliases.put(alias.toAttribute(), alias.child()); + aliases.put(ne.toAttribute(), ((Alias) ne).child()); } } - if (aliases.isEmpty() == false) { + if (aliases.build().isEmpty() == false) { aliases.putAll(queryC.aliases()); queryC = queryC.withAliases(aliases.build()); } From 272624e0c275c830d064b9929bb48d80d21dac83 Mon Sep 17 00:00:00 2001 From: Andras Palinkas Date: Mon, 9 Nov 2020 16:43:02 +0000 Subject: [PATCH 6/6] Change behaviour: do not copy on each `build()` --- .../xpack/ql/expression/AttributeMap.java | 25 +++++++++++++------ .../ql/expression/AttributeMapTests.java | 4 +++ 2 files changed, 22 insertions(+), 7 deletions(-) diff --git a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/AttributeMap.java b/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/AttributeMap.java index 7490af4a64b0e..aec6102c6d906 100644 --- a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/AttributeMap.java +++ b/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/AttributeMap.java @@ -141,7 +141,7 @@ public String toString() { @SuppressWarnings("rawtypes") private static final AttributeMap EMPTY = new AttributeMap<>(); - + @SuppressWarnings("unchecked") public static final AttributeMap emptyAttributeMap() { return EMPTY; @@ -364,24 +364,35 @@ public static Builder builder(AttributeMap map) { } public static class Builder { - private final AttributeMap map = new AttributeMap<>(); + private AttributeMap map = null; + private AttributeMap previouslyBuiltMap = null; private Builder() {} + private AttributeMap map() { + if (map == null) { + map = new AttributeMap<>(); + if (previouslyBuiltMap != null) { + map.addAll(previouslyBuiltMap); + } + } + return map; + } + public Builder put(Attribute attr, E value) { - map.add(attr, value); + map().add(attr, value); return this; } public Builder putAll(AttributeMap m) { - map.addAll(m); + map().addAll(m); return this; } public AttributeMap build() { - // copy, in case someone would do a .build, .put, .build sequence - AttributeMap m = new AttributeMap<>(); - m.addAll(map); + AttributeMap m = map(); + previouslyBuiltMap = m; + map = null; return m; } } diff --git a/x-pack/plugin/ql/src/test/java/org/elasticsearch/xpack/ql/expression/AttributeMapTests.java b/x-pack/plugin/ql/src/test/java/org/elasticsearch/xpack/ql/expression/AttributeMapTests.java index e947ca460d8b8..298ade4db11da 100644 --- a/x-pack/plugin/ql/src/test/java/org/elasticsearch/xpack/ql/expression/AttributeMapTests.java +++ b/x-pack/plugin/ql/src/test/java/org/elasticsearch/xpack/ql/expression/AttributeMapTests.java @@ -93,8 +93,12 @@ public void testBuilder() { // defensive copying builder.put(a("four"), "four"); + AttributeMap m2 = builder.build(); assertThat(m.size(), is(3)); assertThat(m.isEmpty(), is(false)); + assertThat(m2.size(), is(4)); + assertThat(m.isEmpty(), is(false)); + assertThat(m2.attributeNames(), contains("one", "two", "three", "four")); } public void testSingleItemConstructor() {