From 4d6b58495d9087e7aeef0bf0eac37229b5df6fda Mon Sep 17 00:00:00 2001 From: Nik Everett Date: Tue, 9 Jan 2018 11:16:31 -0500 Subject: [PATCH] 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@fe3d7f721686c6dc9e612698f3711ce0aab2ece9 --- .../xpack/sql/expression/ExpressionId.java | 34 +++++++++++-------- .../sql/expression/ExpressionIdGenerator.java | 27 --------------- .../xpack/sql/expression/NamedExpression.java | 2 +- .../sql/expression/SubQueryExpression.java | 2 +- .../expression/UnresolvedNamedExpression.java | 2 +- .../sql/expression/ExpressionIdTests.java | 19 +++++++++++ 6 files changed, 41 insertions(+), 45 deletions(-) delete mode 100644 sql/server/src/main/java/org/elasticsearch/xpack/sql/expression/ExpressionIdGenerator.java create mode 100644 sql/server/src/test/java/org/elasticsearch/xpack/sql/expression/ExpressionIdTests.java diff --git a/sql/server/src/main/java/org/elasticsearch/xpack/sql/expression/ExpressionId.java b/sql/server/src/main/java/org/elasticsearch/xpack/sql/expression/ExpressionId.java index 0191e63e430..55f947a20ac 100644 --- a/sql/server/src/main/java/org/elasticsearch/xpack/sql/expression/ExpressionId.java +++ b/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. + *

+ * 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; - private final String jvmId; - - ExpressionId(int id, String jvmId) { - this.id = id; - this.jvmId = jvmId; + public ExpressionId() { + this.id = COUNTER.incrementAndGet(); } @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 || getClass() != obj.getClass()) { + if (obj == null || obj.getClass() != getClass()) { return false; } - ExpressionId other = (ExpressionId) obj; - return id == other.id - && Objects.equals(jvmId, other.jvmId); + return id == other.id; } @Override public String toString() { - return String.valueOf(id); - //#+ jvmId; + return Long.toString(id); } } diff --git a/sql/server/src/main/java/org/elasticsearch/xpack/sql/expression/ExpressionIdGenerator.java b/sql/server/src/main/java/org/elasticsearch/xpack/sql/expression/ExpressionIdGenerator.java deleted file mode 100644 index ee23f41e8de..00000000000 --- a/sql/server/src/main/java/org/elasticsearch/xpack/sql/expression/ExpressionIdGenerator.java +++ /dev/null @@ -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, "@"); - - public static ExpressionId newId() { - return new ExpressionId(GLOBAL_ID.getAndIncrement(), JVM_ID); - } -} diff --git a/sql/server/src/main/java/org/elasticsearch/xpack/sql/expression/NamedExpression.java b/sql/server/src/main/java/org/elasticsearch/xpack/sql/expression/NamedExpression.java index f123f60620e..274234b90a1 100644 --- a/sql/server/src/main/java/org/elasticsearch/xpack/sql/expression/NamedExpression.java +++ b/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 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; } diff --git a/sql/server/src/main/java/org/elasticsearch/xpack/sql/expression/SubQueryExpression.java b/sql/server/src/main/java/org/elasticsearch/xpack/sql/expression/SubQueryExpression.java index 90297088c0f..89e387f6a40 100644 --- a/sql/server/src/main/java/org/elasticsearch/xpack/sql/expression/SubQueryExpression.java +++ b/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() { diff --git a/sql/server/src/main/java/org/elasticsearch/xpack/sql/expression/UnresolvedNamedExpression.java b/sql/server/src/main/java/org/elasticsearch/xpack/sql/expression/UnresolvedNamedExpression.java index 1c56f87bb3e..ea35c382750 100644 --- a/sql/server/src/main/java/org/elasticsearch/xpack/sql/expression/UnresolvedNamedExpression.java +++ b/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 children) { - super(location, "", children, ExpressionIdGenerator.EMPTY); + super(location, "", children, new ExpressionId()); } @Override diff --git a/sql/server/src/test/java/org/elasticsearch/xpack/sql/expression/ExpressionIdTests.java b/sql/server/src/test/java/org/elasticsearch/xpack/sql/expression/ExpressionIdTests.java new file mode 100644 index 00000000000..546bd3be52a --- /dev/null +++ b/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()); + } +}