SQL: Remove the deprecated `AttributeMap(Map)` calls #64664

palesz · 2020-11-05T17:36:01Z

Uses the AttributeMap.builder() instead of the AttributeMap(Map) to
construct immutable AttributeMaps.

Cleans up after the ctor deprecation in #63710.

elasticmachine · 2020-11-05T17:36:03Z

Pinging @elastic/es-ql (:Query Languages/SQL)

palesz · 2020-11-05T17:45:57Z

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

Aaah. It seems that IDEA rereads the settings from .editorconfig all the time.

costin

Left some comments. Don't forget to list the reviewers otherwise there's little notification.

costin · 2020-11-05T18:01:25Z

.../plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/container/QueryContainer.java

-            procs.put(attr, proc);
-            qContainer = qContainer.withScalarProcessors(new AttributeMap<>(procs));
+            qContainer = qContainer.withScalarProcessors(
+                AttributeMap.<Pipe>builder().putAll(qContainer.scalarFunctions).put(attr, proc).build());


AttributeMap.builder(qContainer.scalarFunctions()).put().build()

costin · 2020-11-05T18:04:18Z

.../plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/container/QueryContainer.java

+                attr.nestedParent().name(), name,
+                SqlDataTypes.format(attr.field().getDataType()),


Please format only the touched lines - there's a plugin/setting for IDEA that does that.

For those such changes that touch code lines, it adds some more effort when checking the git history, or simply "blame". On the other hand, personally I have no problem when deleting whitespaces from empty lines, as long as it's in the touched files.

costin · 2020-11-05T18:06:49Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/planner/QueryFolder.java

-                queryC = queryC.withAliases(new AttributeMap<>(newAliases));
+            List<Alias> aliases = a.aggregates().stream()
+                .filter(ne -> ne instanceof Alias)
+                .map(ne -> (Alias)ne)


Extra space missing, should be (Alias) ne

costin · 2020-11-05T18:12:53Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/planner/QueryFolder.java

+            List<Alias> aliases = a.aggregates().stream()
+                .filter(ne -> ne instanceof Alias)
+                .map(ne -> (Alias)ne)
+                .collect(Collectors.toList());


The initial code was using a Map of attributes, the change uses a List of Aliases which are then converted into a Map.
The use of streams doesn't seem to simplify things either - instead of one iteration there are too.
This needs reworking - just create an AttributeMap with the children and then combine that with the existing aliases.

Ok, now that there is no chance of overwrite that makes more sense.

costin · 2020-11-05T18:13:12Z

.../plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/container/QueryContainer.java

@@ -171,7 +171,7 @@ public QueryContainer(Query query,

            sortingColumns.add(new Tuple<>(Integer.valueOf(atIndex), comp));
        }
-        
+


Again, unneeded formatting.

Seems I have to disable the EditorConfig, otherwise IDEA is too clever the resets these settings.

…map-cleanup

costin

If a micro-optimization starts leaking into a class definition, avoid it.

costin · 2020-11-06T09:54:34Z

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

+        public boolean isEmpty() {
+            return map.isEmpty();
+        }
+


A builder should just assemble a state, not question it's in-transit state.

costin · 2020-11-06T09:56:15Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/planner/QueryFolder.java

+            if (aliases.isEmpty() == false) {
+                aliases.putAll(queryC.aliases());
+                queryC = queryC.withAliases(aliases.build());


Shouldn't be called on a builder. Optimizations are good but in this case they simply get in the way.
Create two AttributeMaps if needed.

costin · 2020-11-06T09:57:24Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/planner/QueryFolder.java

+                    Alias alias = (Alias) ne;
+                    aliases.put(alias.toAttribute(), alias.child());


This change is not needed - there's no value in holding the alias reference.

…map-cleanup

costin

Left a comment. LGTM otherwise.

costin · 2020-11-09T07:21:36Z

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

+            // copy, in case someone would do a .build, .put, .build sequence
+            AttributeMap<E> m = new AttributeMap<>();
+            m.addAll(map);
+            return m;


Why not return the map directly? AttributeMap is immutable.
To guarantee that the builder is not used after build() is called, a flag could be used instead of copying things over.

Changed, won't copy on .build(), only will copy once on the first .put() after a .build().

Removed the defensive copy completely: c4e3fc4

…map-cleanup

Use the `AttributeMap.builder()` instead of the `AttributeMap(Map)` to construct immutable `AttributeMap`s. Clean up after the `ctor` deprecation in elastic#63710

* SQL: Remove the deprecated `AttributeMap(Map)` calls (#64664) Use the `AttributeMap.builder()` instead of the `AttributeMap(Map)` to construct immutable `AttributeMap`s. Clean up after the `ctor` deprecation in #63710

SQL: Removed all the AttributeMap(Map) usage

dbe03f9

palesz added >non-issue :Analytics/SQL SQL querying v8.0.0 Team:QL (Deprecated) Meta label for query languages team v7.11.0 labels Nov 5, 2020

Fix the AttributeSet type inference

ff70ca7

palesz commented Nov 5, 2020

View reviewed changes

costin requested changes Nov 5, 2020

View reviewed changes

Andras Palinkas added 3 commits November 5, 2020 23:08

PR suggestion and whitespace reverts

a46db58

Merge remote-tracking branch 'origin/master' into fix/56013-attribute…

280a692

…map-cleanup

Missed PR suggestion

62f01c1

costin requested changes Nov 6, 2020

View reviewed changes

PR suggestions

6018535

palesz requested a review from costin November 6, 2020 14:18

Merge remote-tracking branch 'origin/master' into fix/56013-attribute…

b44b060

…map-cleanup

costin approved these changes Nov 9, 2020

View reviewed changes

Andras Palinkas added 2 commits November 9, 2020 16:43

Change behaviour: do not copy on each build()

272624e

Merge remote-tracking branch 'origin/master' into fix/56013-attribute…

ee79cba

…map-cleanup

palesz merged commit 658e26d into elastic:master Nov 9, 2020

palesz added the backport pending label Nov 9, 2020

palesz deleted the fix/56013-attributemap-cleanup branch November 9, 2020 17:46

palesz removed the backport pending label Nov 10, 2020

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

		attr.nestedParent().name(), name,
		SqlDataTypes.format(attr.field().getDataType()),

		@@ -171,7 +171,7 @@ public QueryContainer(Query query,

		sortingColumns.add(new Tuple<>(Integer.valueOf(atIndex), comp));
		}

		Alias alias = (Alias) ne;
		aliases.put(alias.toAttribute(), alias.child());

SQL: Remove the deprecated AttributeMap(Map) calls #64664

SQL: Remove the deprecated AttributeMap(Map) calls #64664

Uh oh!

Conversation

palesz commented Nov 5, 2020

Uh oh!

elasticmachine commented Nov 5, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

costin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matriv Nov 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

costin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

costin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

SQL: Remove the deprecated `AttributeMap(Map)` calls #64664

SQL: Remove the deprecated `AttributeMap(Map)` calls #64664

matriv Nov 5, 2020 •

edited

Loading