SQL: Simplify expression ID generation (elastic/x-pack-elasticsearch#3466)

Replaces the `ExpressionIdGenerator` class with a static method and drops
the `jvmId` field from the `ExpressionId` altogether because it isn't needed.

We still have the potential to roll over expression ids if the server lives for
a long time but it is quite unlikely and rolling over only matters if we roll
over in the same query. So long as each expression id is unique to a
particular query we're fine.

Original commit: elastic/x-pack-elasticsearch@fe3d7f7216

sql/server/src/main/java/org/elasticsearch/xpack/sql/expression/ExpressionId.java

@@ -6,40 +6,44 @@
 package org.elasticsearch.xpack.sql.expression;
 
 import java.util.Objects;
+import java.util.concurrent.atomic.AtomicLong;
 
+/**
+ * Unique identifier for an expression.
+ * <p>
+ * We use an {@link AtomicLong} to guarantee that they are unique
+ * and that they produce reproduceable values when run in subsequent
+ * tests. They don't produce reproduceable values in production, but
+ * you rarely debug with them in production and commonly do so in
+ * tests.
+ */
 public class ExpressionId {
+    private static final AtomicLong COUNTER = new AtomicLong();
+    private final long id;
 
-    private final int id;
+    public ExpressionId() {
-    private final String jvmId;
+        this.id = COUNTER.incrementAndGet();
-
-    ExpressionId(int id, String jvmId) {
-        this.id = id;
-        this.jvmId = jvmId;
     }
 
     @Override
     public int hashCode() {
-        return Objects.hash(id, jvmId);
+        return Objects.hash(id);
     }
 
     @Override
     public boolean equals(Object obj) {
-        if (this == obj) {
+        if (obj == this) {
             return true;
         }
-
+        if (obj == null || obj.getClass() != getClass()) {
-        if (obj == null || getClass() != obj.getClass()) {
             return false;
         }
-
         ExpressionId other = (ExpressionId) obj;
-        return id == other.id
+        return id == other.id;
-                && Objects.equals(jvmId, other.jvmId);
     }
 
     @Override
     public String toString() {
-        return String.valueOf(id);
+        return Long.toString(id);
-        //#+ jvmId;
     }
 }

sql/server/src/main/java/org/elasticsearch/xpack/sql/expression/ExpressionIdGenerator.java

@@ -1,27 +0,0 @@
-/*
- * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
- * or more contributor license agreements. Licensed under the Elastic License;
- * you may not use this file except in compliance with the Elastic License.
- */
-package org.elasticsearch.xpack.sql.expression;
-
-import java.util.UUID;
-import java.util.concurrent.atomic.AtomicInteger;
-
-//TODO: this class is thread-safe but used across multiple sessions might cause the
-// id to roll over and potentially generate an already assigned id
-// making this session scope would simplify things
-// (which also begs the question on whether thread-safety is needed than)
-
-// TODO: hook this into SqlSession#SessionContext
-public class ExpressionIdGenerator {
-
-    private static final AtomicInteger GLOBAL_ID = new AtomicInteger();
-    private static final String JVM_ID = "@" + UUID.randomUUID().toString();
-
-    public static final ExpressionId EMPTY = new ExpressionId(-1, "@<empty>");
-
-    public static ExpressionId newId() {
-        return new ExpressionId(GLOBAL_ID.getAndIncrement(), JVM_ID);
-    }
-}

sql/server/src/main/java/org/elasticsearch/xpack/sql/expression/NamedExpression.java

@@ -23,7 +23,7 @@ public abstract class NamedExpression extends Expression {
     public NamedExpression(Location location, String name, List<Expression> children, ExpressionId id, boolean synthetic) {
         super(location, children);
         this.name = name;
-        this.id = (id == null ? ExpressionIdGenerator.newId() : id);
+        this.id = id == null ? new ExpressionId() : id;
         this.synthetic = synthetic;
     }
 

sql/server/src/main/java/org/elasticsearch/xpack/sql/expression/SubQueryExpression.java

@@ -23,7 +23,7 @@ public abstract class SubQueryExpression extends Expression {
     public SubQueryExpression(Location location, LogicalPlan query, ExpressionId id) {
         super(location, Collections.emptyList());
         this.query = query;
-        this.id = (id == null ? ExpressionIdGenerator.newId() : id);
+        this.id = id == null ? new ExpressionId() : id;
     }
 
     public LogicalPlan query() {

sql/server/src/main/java/org/elasticsearch/xpack/sql/expression/UnresolvedNamedExpression.java

@@ -15,7 +15,7 @@ import java.util.List;
 abstract class UnresolvedNamedExpression extends NamedExpression implements Unresolvable {
 
     UnresolvedNamedExpression(Location location, List<Expression> children) {
-        super(location, "<unresolved>", children, ExpressionIdGenerator.EMPTY);
+        super(location, "<unresolved>", children, new ExpressionId());
     }
 
     @Override

sql/server/src/test/java/org/elasticsearch/xpack/sql/expression/ExpressionIdTests.java

@@ -0,0 +1,19 @@
+/*
+ * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
+ * or more contributor license agreements. Licensed under the Elastic License;
+ * you may not use this file except in compliance with the Elastic License.
+ */
+package org.elasticsearch.xpack.sql.expression;
+
+import org.elasticsearch.test.ESTestCase;
+
+public class ExpressionIdTests extends ESTestCase {
+    /**
+     * Each {@link ExpressionId} should be unique. Technically
+     * you can roll the {@link AtomicLong} that backs them but
+     * that is not going to happen within a single query.
+     */
+    public void testUnique() {
+        assertNotEquals(new ExpressionId(), new ExpressionId());
+    }
+}