From 9a0b43cd17c4076f548e2b7ba27157a62adca840 Mon Sep 17 00:00:00 2001 From: Costin Leau Date: Tue, 14 Nov 2017 01:08:10 +0200 Subject: [PATCH] Fix several NOCOMMITS (elastic/x-pack-elasticsearch#2968) * Fix several NOCOMMITS - renamed Assert to Check to make the intent clear - clarify esMajor/Minor inside connection (thse are actually our own methods, not part of JDBC API) - wire pageTimeout into Cursor#nextPage Original commit: elastic/x-pack-elasticsearch@7626c0a44aed36cbfbdcfc4166eb450f8c9f9ed1 --- .../xpack/qa/sql/jdbc/ConnectionTestCase.java | 7 +++--- .../xpack/sql/jdbc/net/protocol/Proto.java | 2 +- .../xpack/sql/jdbc/jdbc/JdbcConnection.java | 14 ++++++----- .../xpack/sql/execution/PlanExecutor.java | 6 ++--- .../sql/execution/search/ScrollCursor.java | 16 +++---------- .../function/AbstractFunctionRegistry.java | 4 ++-- .../xpack/sql/parser/AbstractBuilder.java | 10 ++++---- .../xpack/sql/planner/QueryFolder.java | 8 +++---- .../xpack/sql/planner/QueryTranslator.java | 16 ++++++------- .../sql/plugin/sql/action/SqlResponse.java | 5 ++-- .../plugin/sql/action/TransportSqlAction.java | 8 ++++--- .../xpack/sql/querydsl/agg/AggFilter.java | 4 ++-- .../xpack/sql/session/AbstractRowSet.java | 8 +++---- .../xpack/sql/session/Cursor.java | 2 +- .../xpack/sql/session/EmptyCursor.java | 2 +- .../elasticsearch/xpack/sql/session/Rows.java | 4 ++-- .../sql/session/SingletonExecutable.java | 4 ++-- .../xpack/sql/session/SqlSession.java | 10 ++------ .../xpack/sql/tree/NodeUtils.java | 12 +++++----- .../xpack/sql/type/DataTypeConversion.java | 3 ++- .../elasticsearch/xpack/sql/type/Schema.java | 4 ++-- .../sql/util/{Assert.java => Check.java} | 7 ++++-- .../xpack/sql/util/ReflectionUtils.java | 23 +++++++++++-------- .../sql/protocol/shared/AbstractProto.java | 13 +++++++++-- .../sql/protocol/shared/SqlDataInput.java | 2 +- 25 files changed, 101 insertions(+), 93 deletions(-) rename sql/server/src/main/java/org/elasticsearch/xpack/sql/util/{Assert.java => Check.java} (85%) diff --git a/qa/sql/src/main/java/org/elasticsearch/xpack/qa/sql/jdbc/ConnectionTestCase.java b/qa/sql/src/main/java/org/elasticsearch/xpack/qa/sql/jdbc/ConnectionTestCase.java index 6d8ac38b97a..444142b7138 100644 --- a/qa/sql/src/main/java/org/elasticsearch/xpack/qa/sql/jdbc/ConnectionTestCase.java +++ b/qa/sql/src/main/java/org/elasticsearch/xpack/qa/sql/jdbc/ConnectionTestCase.java @@ -6,9 +6,9 @@ package org.elasticsearch.xpack.qa.sql.jdbc; import org.elasticsearch.Version; -import org.elasticsearch.xpack.sql.jdbc.jdbc.JdbcConnection; import java.sql.Connection; +import java.sql.DatabaseMetaData; import java.sql.SQLException; /** @@ -19,8 +19,9 @@ public abstract class ConnectionTestCase extends JdbcIntegrationTestCase { try (Connection c = esJdbc()) { assertFalse(c.isClosed()); assertTrue(c.isReadOnly()); - assertEquals(Version.CURRENT.major, ((JdbcConnection) c).esInfoMajorVersion()); - assertEquals(Version.CURRENT.minor, ((JdbcConnection) c).esInfoMinorVersion()); + DatabaseMetaData md = c.getMetaData(); + assertEquals(Version.CURRENT.major, md.getDatabaseMajorVersion()); + assertEquals(Version.CURRENT.minor, md.getDatabaseMinorVersion()); } } diff --git a/sql/jdbc-proto/src/main/java/org/elasticsearch/xpack/sql/jdbc/net/protocol/Proto.java b/sql/jdbc-proto/src/main/java/org/elasticsearch/xpack/sql/jdbc/net/protocol/Proto.java index 2a9547542cc..2176dfdaf4a 100644 --- a/sql/jdbc-proto/src/main/java/org/elasticsearch/xpack/sql/jdbc/net/protocol/Proto.java +++ b/sql/jdbc-proto/src/main/java/org/elasticsearch/xpack/sql/jdbc/net/protocol/Proto.java @@ -36,7 +36,7 @@ public final class Proto extends AbstractProto { META_COLUMN(MetaColumnRequest::new), QUERY_INIT(QueryInitRequest::new), QUERY_PAGE(QueryPageRequest::new), -// QUERY_CLOSE(QueryClosenRequest::new), TODO implement me +// QUERY_CLOSE(QueryClosenRequest::new), NOCOMMIT implement me ; private final RequestReader reader; diff --git a/sql/jdbc/src/main/java/org/elasticsearch/xpack/sql/jdbc/jdbc/JdbcConnection.java b/sql/jdbc/src/main/java/org/elasticsearch/xpack/sql/jdbc/jdbc/JdbcConnection.java index cbd7fc805cd..8a860b37ff9 100644 --- a/sql/jdbc/src/main/java/org/elasticsearch/xpack/sql/jdbc/jdbc/JdbcConnection.java +++ b/sql/jdbc/src/main/java/org/elasticsearch/xpack/sql/jdbc/jdbc/JdbcConnection.java @@ -5,7 +5,6 @@ */ package org.elasticsearch.xpack.sql.jdbc.jdbc; -import org.elasticsearch.xpack.sql.client.shared.StringUtils; import org.elasticsearch.xpack.sql.jdbc.debug.Debug; import org.elasticsearch.xpack.sql.jdbc.net.client.JdbcHttpClient; @@ -141,6 +140,7 @@ public class JdbcConnection implements Connection, JdbcWrapper { @Override public boolean isReadOnly() throws SQLException { + checkOpen(); return true; } @@ -321,7 +321,7 @@ public class JdbcConnection implements Connection, JdbcWrapper { if (timeout < 0) { throw new SQLException("Negative timeout"); } - return client.ping(TimeUnit.SECONDS.toMillis(timeout)); + return !isClosed() && client.ping(TimeUnit.SECONDS.toMillis(timeout)); } private void checkOpenClientInfo() throws SQLClientInfoException { @@ -429,17 +429,19 @@ public class JdbcConnection implements Connection, JdbcWrapper { return userName; } - // NOCOMMIT should this be one of those wrapped things? - public int esInfoMajorVersion() throws SQLException { + // There's no checkOpen on these methods since they are used by + // DatabaseMetadata that can work on a closed connection as well + // in fact, this information is cached by the underlying client + // once retrieved + int esInfoMajorVersion() throws SQLException { return client.serverInfo().majorVersion; } - public int esInfoMinorVersion() throws SQLException { + int esInfoMinorVersion() throws SQLException { return client.serverInfo().minorVersion; } public void setTimeZone(TimeZone tz) { cfg.timeZone(tz); } - } \ No newline at end of file diff --git a/sql/server/src/main/java/org/elasticsearch/xpack/sql/execution/PlanExecutor.java b/sql/server/src/main/java/org/elasticsearch/xpack/sql/execution/PlanExecutor.java index 1a86ad67ea7..4768ce660a5 100644 --- a/sql/server/src/main/java/org/elasticsearch/xpack/sql/execution/PlanExecutor.java +++ b/sql/server/src/main/java/org/elasticsearch/xpack/sql/execution/PlanExecutor.java @@ -19,11 +19,11 @@ import org.elasticsearch.xpack.sql.plan.physical.EsQueryExec; import org.elasticsearch.xpack.sql.plan.physical.PhysicalPlan; import org.elasticsearch.xpack.sql.planner.Planner; import org.elasticsearch.xpack.sql.planner.PlanningException; +import org.elasticsearch.xpack.sql.session.Configuration; import org.elasticsearch.xpack.sql.session.Cursor; import org.elasticsearch.xpack.sql.session.RowSet; import org.elasticsearch.xpack.sql.session.SchemaRowSet; import org.elasticsearch.xpack.sql.session.SqlSession; -import org.elasticsearch.xpack.sql.session.Configuration; import java.util.function.Function; import java.util.function.Supplier; @@ -82,7 +82,7 @@ public class PlanExecutor { } } - public void nextPage(Cursor cursor, ActionListener listener) { - cursor.nextPage(client, listener); + public void nextPage(Configuration cfg, Cursor cursor, ActionListener listener) { + cursor.nextPage(cfg, client, listener); } } diff --git a/sql/server/src/main/java/org/elasticsearch/xpack/sql/execution/search/ScrollCursor.java b/sql/server/src/main/java/org/elasticsearch/xpack/sql/execution/search/ScrollCursor.java index 590b83e86bd..f9b3e3f236b 100644 --- a/sql/server/src/main/java/org/elasticsearch/xpack/sql/execution/search/ScrollCursor.java +++ b/sql/server/src/main/java/org/elasticsearch/xpack/sql/execution/search/ScrollCursor.java @@ -18,21 +18,17 @@ import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.io.stream.StreamOutput; import org.elasticsearch.xpack.sql.execution.search.extractor.HitExtractor; import org.elasticsearch.xpack.sql.execution.search.extractor.HitExtractors; +import org.elasticsearch.xpack.sql.session.Configuration; import org.elasticsearch.xpack.sql.session.Cursor; import org.elasticsearch.xpack.sql.session.RowSet; -import org.elasticsearch.xpack.sql.type.DataType; -import org.elasticsearch.xpack.sql.type.Schema; import java.io.IOException; import java.io.InputStream; import java.io.OutputStream; -import java.util.ArrayList; import java.util.Base64; import java.util.List; import java.util.Objects; -import static org.elasticsearch.common.unit.TimeValue.timeValueSeconds; - public class ScrollCursor implements Cursor { public static final String NAME = "s"; /** @@ -109,14 +105,8 @@ public class ScrollCursor implements Cursor { } @Override - public void nextPage(Client client, ActionListener listener) { - // NOCOMMIT add keep alive to the settings and pass it here - /* Or something. The trouble is that settings is for *starting* - * queries, but maybe we should actually have two sets of settings, - * one for things that are only valid when going to the next page - * and one that is valid for starting queries. - */ - SearchScrollRequest request = new SearchScrollRequest(scrollId).scroll(timeValueSeconds(90)); + public void nextPage(Configuration cfg, Client client, ActionListener listener) { + SearchScrollRequest request = new SearchScrollRequest(scrollId).scroll(cfg.pageTimeout()); client.searchScroll(request, ActionListener.wrap((SearchResponse response) -> { int limitHits = limit; listener.onResponse(new ScrolledSearchHitRowSet(extractors, response.getHits().getHits(), diff --git a/sql/server/src/main/java/org/elasticsearch/xpack/sql/expression/function/AbstractFunctionRegistry.java b/sql/server/src/main/java/org/elasticsearch/xpack/sql/expression/function/AbstractFunctionRegistry.java index 760f590ac90..e5c8b11f109 100644 --- a/sql/server/src/main/java/org/elasticsearch/xpack/sql/expression/function/AbstractFunctionRegistry.java +++ b/sql/server/src/main/java/org/elasticsearch/xpack/sql/expression/function/AbstractFunctionRegistry.java @@ -14,7 +14,7 @@ import org.elasticsearch.xpack.sql.session.Configuration; import org.elasticsearch.xpack.sql.tree.Node; import org.elasticsearch.xpack.sql.tree.NodeUtils; import org.elasticsearch.xpack.sql.tree.NodeUtils.NodeInfo; -import org.elasticsearch.xpack.sql.util.Assert; +import org.elasticsearch.xpack.sql.util.Check; import org.elasticsearch.xpack.sql.util.StringUtils; import java.lang.reflect.InvocationTargetException; @@ -37,7 +37,7 @@ abstract class AbstractFunctionRegistry implements FunctionRegistry { FunctionDefinition def = def(f, aliases()); defs.put(def.name(), def); for (String alias : def.aliases()) { - Assert.isTrue(defs.containsKey(alias) == false, "Alias %s already exists", alias); + Check.isTrue(defs.containsKey(alias) == false, "Alias %s already exists", alias); // alias should be already normalized but to be double sure defs.put(normalize(alias), def); } diff --git a/sql/server/src/main/java/org/elasticsearch/xpack/sql/parser/AbstractBuilder.java b/sql/server/src/main/java/org/elasticsearch/xpack/sql/parser/AbstractBuilder.java index 82f649db34e..cedf5d30d6f 100644 --- a/sql/server/src/main/java/org/elasticsearch/xpack/sql/parser/AbstractBuilder.java +++ b/sql/server/src/main/java/org/elasticsearch/xpack/sql/parser/AbstractBuilder.java @@ -16,7 +16,7 @@ import org.elasticsearch.common.Strings; import org.elasticsearch.xpack.sql.parser.SqlBaseBaseVisitor; import org.elasticsearch.xpack.sql.plan.logical.LogicalPlan; import org.elasticsearch.xpack.sql.tree.Location; -import org.elasticsearch.xpack.sql.util.Assert; +import org.elasticsearch.xpack.sql.util.Check; import static java.util.stream.Collectors.toList; @@ -25,7 +25,7 @@ abstract class AbstractBuilder extends SqlBaseBaseVisitor { @Override public Object visit(ParseTree tree) { Object result = super.visit(tree); - Assert.notNull(result, "Don't know how to handle context [%s] with value [%s]", tree.getClass(), tree.getText()); + Check.notNull(result, "Don't know how to handle context [%s] with value [%s]", tree.getClass(), tree.getText()); return result; } @@ -71,17 +71,17 @@ abstract class AbstractBuilder extends SqlBaseBaseVisitor { } static Location source(TerminalNode terminalNode) { - Assert.notNull(terminalNode, "terminalNode is null"); + Check.notNull(terminalNode, "terminalNode is null"); return source(terminalNode.getSymbol()); } static Location source(ParserRuleContext parserRuleContext) { - Assert.notNull(parserRuleContext, "parserRuleContext is null"); + Check.notNull(parserRuleContext, "parserRuleContext is null"); return source(parserRuleContext.getStart()); } static Location source(Token token) { - Assert.notNull(token, "token is null"); + Check.notNull(token, "token is null"); return new Location(token.getLine(), token.getCharPositionInLine()); } diff --git a/sql/server/src/main/java/org/elasticsearch/xpack/sql/planner/QueryFolder.java b/sql/server/src/main/java/org/elasticsearch/xpack/sql/planner/QueryFolder.java index 4cbadc96a62..44e1f7fde74 100644 --- a/sql/server/src/main/java/org/elasticsearch/xpack/sql/planner/QueryFolder.java +++ b/sql/server/src/main/java/org/elasticsearch/xpack/sql/planner/QueryFolder.java @@ -52,7 +52,7 @@ import org.elasticsearch.xpack.sql.querydsl.query.Query; import org.elasticsearch.xpack.sql.rule.Rule; import org.elasticsearch.xpack.sql.rule.RuleExecutor; import org.elasticsearch.xpack.sql.session.EmptyExecutable; -import org.elasticsearch.xpack.sql.util.Assert; +import org.elasticsearch.xpack.sql.util.Check; import org.elasticsearch.xpack.sql.util.StringUtils; import java.util.Arrays; @@ -342,12 +342,12 @@ class QueryFolder extends RuleExecutor { } // attributes can only refer to declared groups if (child instanceof Attribute) { - Assert.notNull(matchingGroup, "Cannot find group '%s'", Expressions.name(child)); + Check.notNull(matchingGroup, "Cannot find group '%s'", Expressions.name(child)); queryC = queryC.addAggColumn(matchingGroup.propertyPath()); } else { // the only thing left is agg function - Assert.isTrue(Functions.isAggregateFunction(child), "Expected aggregate function inside alias; got %s", child.nodeString()); + Check.isTrue(Functions.isAggregateFunction(child), "Expected aggregate function inside alias; got %s", child.nodeString()); Tuple withAgg = addAggFunction(matchingGroup, (AggregateFunction) child, compoundAggMap, queryC); //FIXME: what about inner key queryC = withAgg.v1().addAggColumn(withAgg.v2().context()); @@ -362,7 +362,7 @@ class QueryFolder extends RuleExecutor { GroupingAgg matchingGroup = null; if (groupingContext != null) { matchingGroup = groupingContext.groupFor(ne); - Assert.notNull(matchingGroup, "Cannot find group '%s'", Expressions.name(ne)); + Check.notNull(matchingGroup, "Cannot find group '%s'", Expressions.name(ne)); queryC = queryC.addAggColumn(matchingGroup.propertyPath()); } } diff --git a/sql/server/src/main/java/org/elasticsearch/xpack/sql/planner/QueryTranslator.java b/sql/server/src/main/java/org/elasticsearch/xpack/sql/planner/QueryTranslator.java index 05e2b272b13..e8823dd550c 100644 --- a/sql/server/src/main/java/org/elasticsearch/xpack/sql/planner/QueryTranslator.java +++ b/sql/server/src/main/java/org/elasticsearch/xpack/sql/planner/QueryTranslator.java @@ -87,7 +87,7 @@ import org.elasticsearch.xpack.sql.querydsl.query.TermQuery; import org.elasticsearch.xpack.sql.querydsl.query.WildcardQuery; import org.elasticsearch.xpack.sql.tree.Location; import org.elasticsearch.xpack.sql.type.DataTypes; -import org.elasticsearch.xpack.sql.util.Assert; +import org.elasticsearch.xpack.sql.util.Check; import org.elasticsearch.xpack.sql.util.ReflectionUtils; import java.util.Arrays; @@ -293,7 +293,7 @@ abstract class QueryTranslator { } static QueryTranslation and(Location loc, QueryTranslation left, QueryTranslation right) { - Assert.isTrue(left != null || right != null, "Both expressions are null"); + Check.isTrue(left != null || right != null, "Both expressions are null"); if (left == null) { return right; } @@ -322,7 +322,7 @@ abstract class QueryTranslator { } static Query and(Location loc, Query left, Query right) { - Assert.isTrue(left != null || right != null, "Both expressions are null"); + Check.isTrue(left != null || right != null, "Both expressions are null"); if (left == null) { return right; } @@ -333,7 +333,7 @@ abstract class QueryTranslator { } static QueryTranslation or(Location loc, QueryTranslation left, QueryTranslation right) { - Assert.isTrue(left != null || right != null, "Both expressions are null"); + Check.isTrue(left != null || right != null, "Both expressions are null"); if (left == null) { return right; } @@ -362,7 +362,7 @@ abstract class QueryTranslator { } static Query or(Location loc, Query left, Query right) { - Assert.isTrue(left != null || right != null, "Both expressions are null"); + Check.isTrue(left != null || right != null, "Both expressions are null"); if (left == null) { return right; @@ -374,7 +374,7 @@ abstract class QueryTranslator { } static Query not(Query query) { - Assert.isTrue(query != null, "Expressions is null"); + Check.isTrue(query != null, "Expressions is null"); return new NotQuery(query.location(), query); } @@ -524,7 +524,7 @@ abstract class QueryTranslator { @Override protected QueryTranslation asQuery(BinaryComparison bc, boolean onAggs) { - Assert.isTrue(bc.right().foldable(), "don't know how to translate right %s in %s", bc.right().nodeString(), bc); + Check.isTrue(bc.right().foldable(), "don't know how to translate right %s in %s", bc.right().nodeString(), bc); if (bc.left() instanceof NamedExpression) { NamedExpression ne = (NamedExpression) bc.left(); @@ -619,7 +619,7 @@ abstract class QueryTranslator { return new TermQuery(loc, name, value); } - Assert.isTrue(false, "don't know how to translate binary comparison %s in %s", bc.right().nodeString(), bc); + Check.isTrue(false, "don't know how to translate binary comparison %s in %s", bc.right().nodeString(), bc); return null; } } diff --git a/sql/server/src/main/java/org/elasticsearch/xpack/sql/plugin/sql/action/SqlResponse.java b/sql/server/src/main/java/org/elasticsearch/xpack/sql/plugin/sql/action/SqlResponse.java index 2128325b581..c28b43c13be 100644 --- a/sql/server/src/main/java/org/elasticsearch/xpack/sql/plugin/sql/action/SqlResponse.java +++ b/sql/server/src/main/java/org/elasticsearch/xpack/sql/plugin/sql/action/SqlResponse.java @@ -36,8 +36,9 @@ public class SqlResponse extends ActionResponse implements ToXContentObject { public SqlResponse(Cursor cursor, long size, int columnCount, @Nullable List columns, List> rows) { this.cursor = cursor; - this.size = size; // NOCOMMIT Probably should be removed. - // Size isn't the total number of results like ES uses, it is the size of the rows list. + this.size = size; + // Size isn't the total number of results like ES uses, it is the size of the current rows list. + // While not necessary internally, it is useful for REST responses this.columnCount = columnCount; this.columns = columns; this.rows = rows; diff --git a/sql/server/src/main/java/org/elasticsearch/xpack/sql/plugin/sql/action/TransportSqlAction.java b/sql/server/src/main/java/org/elasticsearch/xpack/sql/plugin/sql/action/TransportSqlAction.java index 07fb17e953e..80bc6cee555 100644 --- a/sql/server/src/main/java/org/elasticsearch/xpack/sql/plugin/sql/action/TransportSqlAction.java +++ b/sql/server/src/main/java/org/elasticsearch/xpack/sql/plugin/sql/action/TransportSqlAction.java @@ -53,13 +53,15 @@ public class TransportSqlAction extends HandledTransportAction listener) { + // The configuration is always created however when dealing with the next page, only the timeouts are relevant + // the rest having default values (since the query is already created) + Configuration cfg = new Configuration(request.timeZone(), request.fetchSize(), request.requestTimeout(), request.pageTimeout()); + if (request.cursor() == Cursor.EMPTY) { - Configuration cfg = new Configuration(request.timeZone(), request.fetchSize(), - request.requestTimeout(), request.pageTimeout()); planExecutor.sql(cfg, request.query(), ActionListener.wrap(rowSet -> listener.onResponse(createResponse(rowSet)), listener::onFailure)); } else { - planExecutor.nextPage(request.cursor(), + planExecutor.nextPage(cfg, request.cursor(), ActionListener.wrap(rowSet -> listener.onResponse(createResponse(rowSet, null)), listener::onFailure)); } } diff --git a/sql/server/src/main/java/org/elasticsearch/xpack/sql/querydsl/agg/AggFilter.java b/sql/server/src/main/java/org/elasticsearch/xpack/sql/querydsl/agg/AggFilter.java index c497edca8a5..1b8f6c99d61 100644 --- a/sql/server/src/main/java/org/elasticsearch/xpack/sql/querydsl/agg/AggFilter.java +++ b/sql/server/src/main/java/org/elasticsearch/xpack/sql/querydsl/agg/AggFilter.java @@ -12,7 +12,7 @@ import java.util.Objects; import org.elasticsearch.script.Script; import org.elasticsearch.search.aggregations.PipelineAggregationBuilder; import org.elasticsearch.xpack.sql.expression.function.scalar.script.ScriptTemplate; -import org.elasticsearch.xpack.sql.util.Assert; +import org.elasticsearch.xpack.sql.util.Check; import static org.elasticsearch.search.aggregations.pipeline.PipelineAggregatorBuilders.bucketSelector; @@ -23,7 +23,7 @@ public class AggFilter extends PipelineAgg { public AggFilter(String name, ScriptTemplate scriptTemplate) { super(name); - Assert.isTrue(scriptTemplate != null, "a valid script is required"); + Check.isTrue(scriptTemplate != null, "a valid script is required"); this.scriptTemplate = scriptTemplate; this.aggPaths = scriptTemplate.aggPaths(); } diff --git a/sql/server/src/main/java/org/elasticsearch/xpack/sql/session/AbstractRowSet.java b/sql/server/src/main/java/org/elasticsearch/xpack/sql/session/AbstractRowSet.java index d6b7a02d9c4..ed36871d3fc 100644 --- a/sql/server/src/main/java/org/elasticsearch/xpack/sql/session/AbstractRowSet.java +++ b/sql/server/src/main/java/org/elasticsearch/xpack/sql/session/AbstractRowSet.java @@ -5,16 +5,16 @@ */ package org.elasticsearch.xpack.sql.session; -import org.elasticsearch.xpack.sql.util.Assert; +import org.elasticsearch.xpack.sql.util.Check; public abstract class AbstractRowSet implements RowSet { private boolean terminated = false; @Override public Object column(int index) { - Assert.isTrue(index >= 0, "Invalid index %d; needs to be positive", index); - Assert.isTrue(index < columnCount(), "Invalid index %d for row of size %d", index, columnCount()); - Assert.isTrue(hasCurrentRow(), "RowSet contains no (more) entries; use hasCurrent() to check its status"); + Check.isTrue(index >= 0, "Invalid index %d; needs to be positive", index); + Check.isTrue(index < columnCount(), "Invalid index %d for row of size %d", index, columnCount()); + Check.isTrue(hasCurrentRow(), "RowSet contains no (more) entries; use hasCurrent() to check its status"); return getColumn(index); } diff --git a/sql/server/src/main/java/org/elasticsearch/xpack/sql/session/Cursor.java b/sql/server/src/main/java/org/elasticsearch/xpack/sql/session/Cursor.java index 45024f27750..052759e6327 100644 --- a/sql/server/src/main/java/org/elasticsearch/xpack/sql/session/Cursor.java +++ b/sql/server/src/main/java/org/elasticsearch/xpack/sql/session/Cursor.java @@ -27,7 +27,7 @@ public interface Cursor extends NamedWriteable { /** * Request the next page of data. */ - void nextPage(Client client, ActionListener listener); + void nextPage(Configuration cfg, Client client, ActionListener listener); /** * Write the {@linkplain Cursor} to a String for serialization over xcontent. */ diff --git a/sql/server/src/main/java/org/elasticsearch/xpack/sql/session/EmptyCursor.java b/sql/server/src/main/java/org/elasticsearch/xpack/sql/session/EmptyCursor.java index 56c7827ba6d..10db79f42f6 100644 --- a/sql/server/src/main/java/org/elasticsearch/xpack/sql/session/EmptyCursor.java +++ b/sql/server/src/main/java/org/elasticsearch/xpack/sql/session/EmptyCursor.java @@ -35,7 +35,7 @@ class EmptyCursor implements Cursor { } @Override - public void nextPage(Client client, ActionListener listener) { + public void nextPage(Configuration cfg, Client client, ActionListener listener) { throw new IllegalArgumentException("there is no next page"); } diff --git a/sql/server/src/main/java/org/elasticsearch/xpack/sql/session/Rows.java b/sql/server/src/main/java/org/elasticsearch/xpack/sql/session/Rows.java index 34395dbe837..a5a99645fee 100644 --- a/sql/server/src/main/java/org/elasticsearch/xpack/sql/session/Rows.java +++ b/sql/server/src/main/java/org/elasticsearch/xpack/sql/session/Rows.java @@ -11,7 +11,7 @@ import java.util.List; import org.elasticsearch.xpack.sql.expression.Attribute; import org.elasticsearch.xpack.sql.type.DataType; import org.elasticsearch.xpack.sql.type.Schema; -import org.elasticsearch.xpack.sql.util.Assert; +import org.elasticsearch.xpack.sql.util.Check; import static java.util.Collections.singletonList; @@ -64,7 +64,7 @@ public abstract class Rows { } public static SchemaRowSet singleton(List attrs, Object... values) { - Assert.isTrue(attrs.size() == values.length, "Schema %s and values %s are out of sync", attrs, values); + Check.isTrue(attrs.size() == values.length, "Schema %s and values %s are out of sync", attrs, values); return new SingletonRowSet(schema(attrs), values); } diff --git a/sql/server/src/main/java/org/elasticsearch/xpack/sql/session/SingletonExecutable.java b/sql/server/src/main/java/org/elasticsearch/xpack/sql/session/SingletonExecutable.java index 9785286e674..126c978ef4e 100644 --- a/sql/server/src/main/java/org/elasticsearch/xpack/sql/session/SingletonExecutable.java +++ b/sql/server/src/main/java/org/elasticsearch/xpack/sql/session/SingletonExecutable.java @@ -7,7 +7,7 @@ package org.elasticsearch.xpack.sql.session; import org.elasticsearch.action.ActionListener; import org.elasticsearch.xpack.sql.expression.Attribute; -import org.elasticsearch.xpack.sql.util.Assert; +import org.elasticsearch.xpack.sql.util.Check; import java.util.List; @@ -17,7 +17,7 @@ public class SingletonExecutable implements Executable { private final Object[] values; public SingletonExecutable(List output, Object... values) { - Assert.isTrue(output.size() == values.length, "Output %s and values %s are out of sync", output, values); + Check.isTrue(output.size() == values.length, "Output %s and values %s are out of sync", output, values); this.output = output; this.values = values; } diff --git a/sql/server/src/main/java/org/elasticsearch/xpack/sql/session/SqlSession.java b/sql/server/src/main/java/org/elasticsearch/xpack/sql/session/SqlSession.java index 910a71c61b7..460a2bcb04e 100644 --- a/sql/server/src/main/java/org/elasticsearch/xpack/sql/session/SqlSession.java +++ b/sql/server/src/main/java/org/elasticsearch/xpack/sql/session/SqlSession.java @@ -33,7 +33,6 @@ public class SqlSession { private final Planner planner; private final Analyzer analyzer; - private final Configuration defaults; // NOCOMMIT this doesn't look used - it is for RESET SESSION private Configuration settings; // thread-local used for sharing settings across the plan compilation @@ -46,7 +45,7 @@ public class SqlSession { }; public SqlSession(SqlSession other) { - this(other.defaults(), other.client(), other.catalog(), other.functionRegistry(), + this(other.settings(), other.client(), other.catalog(), other.functionRegistry(), other.parser, other.optimizer(), other.planner()); } @@ -65,7 +64,6 @@ public class SqlSession { this.optimizer = optimizer; this.planner = planner; - this.defaults = defaults; this.settings = defaults; } @@ -137,10 +135,6 @@ public class SqlSession { executable(sql).execute(this, listener); } - public Configuration defaults() { - return defaults; - } - public Configuration settings() { return settings; } @@ -148,4 +142,4 @@ public class SqlSession { public void execute(PhysicalPlan plan, ActionListener listener) { plan.execute(this, listener); } -} +} \ No newline at end of file diff --git a/sql/server/src/main/java/org/elasticsearch/xpack/sql/tree/NodeUtils.java b/sql/server/src/main/java/org/elasticsearch/xpack/sql/tree/NodeUtils.java index 89c060495fb..1455ea475fb 100644 --- a/sql/server/src/main/java/org/elasticsearch/xpack/sql/tree/NodeUtils.java +++ b/sql/server/src/main/java/org/elasticsearch/xpack/sql/tree/NodeUtils.java @@ -7,7 +7,7 @@ package org.elasticsearch.xpack.sql.tree; import org.elasticsearch.common.util.CollectionUtils; import org.elasticsearch.xpack.sql.SqlIllegalArgumentException; -import org.elasticsearch.xpack.sql.util.Assert; +import org.elasticsearch.xpack.sql.util.Check; import java.lang.reflect.Constructor; import java.lang.reflect.InvocationTargetException; @@ -69,11 +69,11 @@ public abstract class NodeUtils { // public Literal right() { return right; } // } public static > T copyTree(Node tree, List newChildren) { - Assert.notNull(tree, "Non-null tree expected"); + Check.notNull(tree, "Non-null tree expected"); // basic sanity check List currentChildren = tree.children(); - Assert.isTrue(currentChildren.size() == newChildren.size(), "Cannot make copy; expected %s children but received %s", currentChildren.size(), newChildren.size()); + Check.isTrue(currentChildren.size() == newChildren.size(), "Cannot make copy; expected %s children but received %s", currentChildren.size(), newChildren.size()); NodeInfo info = info(tree.getClass()); Object[] props = properties(tree, info); @@ -126,7 +126,7 @@ public abstract class NodeUtils { // perform discovery (and cache it) if (treeNodeInfo == null) { Constructor[] constructors = clazz.getConstructors(); - Assert.isTrue(!CollectionUtils.isEmpty(constructors), "No public constructors found for class %s", clazz); + Check.isTrue(!CollectionUtils.isEmpty(constructors), "No public constructors found for class %s", clazz); // find the longest constructor Constructor ctr = null; @@ -146,7 +146,7 @@ public abstract class NodeUtils { Parameter[] parameters = ctr.getParameters(); for (int paramIndex = 0; paramIndex < parameters.length; paramIndex++) { Parameter param = parameters[paramIndex]; - Assert.isTrue(param.isNamePresent(), "Can't find constructor parameter names for [%s]. Is class debug information available?", clazz.toGenericString()); + Check.isTrue(param.isNamePresent(), "Can't find constructor parameter names for [%s]. Is class debug information available?", clazz.toGenericString()); String paramName = param.getName(); if (paramName.equals("children")) { @@ -165,7 +165,7 @@ public abstract class NodeUtils { Class expected = param.getType(); Class found = getter.getReturnType(); // found == Object if we're dealing with generics - Assert.isTrue(found == Object.class || expected.isAssignableFrom(found), "Constructor param [%s] in class [%s] has type [%s] but found getter [%s]", paramName, clazz, expected, getter.toGenericString()); + Check.isTrue(found == Object.class || expected.isAssignableFrom(found), "Constructor param [%s] in class [%s] has type [%s] but found getter [%s]", paramName, clazz, expected, getter.toGenericString()); params.put(paramName, getter); } diff --git a/sql/server/src/main/java/org/elasticsearch/xpack/sql/type/DataTypeConversion.java b/sql/server/src/main/java/org/elasticsearch/xpack/sql/type/DataTypeConversion.java index f9361badd13..754949c6ef8 100644 --- a/sql/server/src/main/java/org/elasticsearch/xpack/sql/type/DataTypeConversion.java +++ b/sql/server/src/main/java/org/elasticsearch/xpack/sql/type/DataTypeConversion.java @@ -17,6 +17,8 @@ import java.util.function.LongFunction; /** * Conversions from one data type to another. + * This class throws {@link SqlIllegalArgumentException} to differentiate between validation + * errors inside SQL as oppose to the rest of ES. */ public abstract class DataTypeConversion { @@ -280,7 +282,6 @@ public abstract class DataTypeConversion { public static int safeToInt(long x) { if (x > Integer.MAX_VALUE || x < Integer.MIN_VALUE) { - // NOCOMMIT should these instead be regular IllegalArgumentExceptions so we throw a 400 error? Or something else? throw new SqlIllegalArgumentException("numeric %d out of int range", Long.toString(x)); } return (int) x; diff --git a/sql/server/src/main/java/org/elasticsearch/xpack/sql/type/Schema.java b/sql/server/src/main/java/org/elasticsearch/xpack/sql/type/Schema.java index eae21567b91..4370b232f68 100644 --- a/sql/server/src/main/java/org/elasticsearch/xpack/sql/type/Schema.java +++ b/sql/server/src/main/java/org/elasticsearch/xpack/sql/type/Schema.java @@ -14,7 +14,7 @@ import java.util.stream.Stream; import java.util.stream.StreamSupport; import org.elasticsearch.xpack.sql.type.Schema.Entry; -import org.elasticsearch.xpack.sql.util.Assert; +import org.elasticsearch.xpack.sql.util.Check; import static java.util.Collections.emptyList; @@ -51,7 +51,7 @@ public class Schema implements Iterable { private final List types; public Schema(List names, List types) { - Assert.isTrue(names.size() == types.size(), "Different # of names %s vs types %s", names, types); + Check.isTrue(names.size() == types.size(), "Different # of names %s vs types %s", names, types); this.types = types; this.names = names; } diff --git a/sql/server/src/main/java/org/elasticsearch/xpack/sql/util/Assert.java b/sql/server/src/main/java/org/elasticsearch/xpack/sql/util/Check.java similarity index 85% rename from sql/server/src/main/java/org/elasticsearch/xpack/sql/util/Assert.java rename to sql/server/src/main/java/org/elasticsearch/xpack/sql/util/Check.java index d1b85ac0e0b..2fa8164a1cc 100644 --- a/sql/server/src/main/java/org/elasticsearch/xpack/sql/util/Assert.java +++ b/sql/server/src/main/java/org/elasticsearch/xpack/sql/util/Check.java @@ -7,8 +7,11 @@ package org.elasticsearch.xpack.sql.util; import org.elasticsearch.xpack.sql.SqlIllegalArgumentException; -public abstract class Assert { - // NOCOMMIT we should investigate using java assertions like the rest of the code base +/** + * Utility class used for checking various conditions at runtime, inside SQL (hence the specific exception) with + * minimum amount of code + */ +public abstract class Check { public static void isTrue(boolean expression, String message, Object... values) { if (!expression) { diff --git a/sql/server/src/main/java/org/elasticsearch/xpack/sql/util/ReflectionUtils.java b/sql/server/src/main/java/org/elasticsearch/xpack/sql/util/ReflectionUtils.java index ca55c75c8f1..c30852ef328 100644 --- a/sql/server/src/main/java/org/elasticsearch/xpack/sql/util/ReflectionUtils.java +++ b/sql/server/src/main/java/org/elasticsearch/xpack/sql/util/ReflectionUtils.java @@ -5,14 +5,14 @@ */ package org.elasticsearch.xpack.sql.util; +import org.elasticsearch.xpack.sql.SqlIllegalArgumentException; + import java.lang.reflect.GenericArrayType; import java.lang.reflect.ParameterizedType; import java.lang.reflect.Type; import java.lang.reflect.WildcardType; import java.util.Arrays; -import org.elasticsearch.xpack.sql.SqlIllegalArgumentException; - public class ReflectionUtils { @SuppressWarnings("unchecked") @@ -22,8 +22,9 @@ public class ReflectionUtils { } if (t instanceof ParameterizedType) { Type[] typeArguments = ((ParameterizedType) t).getActualTypeArguments(); - Assert.isTrue(typeArguments.length == 1, - "Unexpected number of type arguments %s for %s", Arrays.toString(typeArguments), t); + if (typeArguments.length == 1) { + throw new SqlIllegalArgumentException("Unexpected number of type arguments %s for %s", Arrays.toString(typeArguments), t); + } return detectType(typeArguments[0]); } @@ -33,8 +34,10 @@ public class ReflectionUtils { return detectType(wt.getLowerBounds()[0]); } Type[] upperBounds = wt.getUpperBounds(); - Assert.isTrue(upperBounds.length == 1, - "Unexpected number of upper bounds %s for %s", Arrays.toString(upperBounds), t); + + if (upperBounds.length == 1) { + throw new SqlIllegalArgumentException("Unexpected number of upper bounds %s for %s", Arrays.toString(upperBounds), t); + } return detectType(upperBounds[0]); } @@ -51,8 +54,10 @@ public class ReflectionUtils { for (Type type = clazz.getGenericSuperclass(); clazz != Object.class; type = clazz.getGenericSuperclass()) { if (type instanceof ParameterizedType) { Type[] typeArguments = ((ParameterizedType) type).getActualTypeArguments(); - Assert.isTrue(typeArguments.length == 2 || typeArguments.length == 1, - "Unexpected number of type arguments %s for %s", Arrays.toString(typeArguments), c); + if (typeArguments.length == 2 || typeArguments.length == 1) { + throw new SqlIllegalArgumentException("Unexpected number of type arguments %s for %s", Arrays.toString(typeArguments), + c); + } return (Class) typeArguments[0]; } @@ -73,4 +78,4 @@ public class ReflectionUtils { return className; } } -} +} \ No newline at end of file diff --git a/sql/shared-proto/src/main/java/org/elasticsearch/xpack/sql/protocol/shared/AbstractProto.java b/sql/shared-proto/src/main/java/org/elasticsearch/xpack/sql/protocol/shared/AbstractProto.java index 94a8725fbeb..718e490f531 100644 --- a/sql/shared-proto/src/main/java/org/elasticsearch/xpack/sql/protocol/shared/AbstractProto.java +++ b/sql/shared-proto/src/main/java/org/elasticsearch/xpack/sql/protocol/shared/AbstractProto.java @@ -36,12 +36,21 @@ public abstract class AbstractProto { request.writeTo(new SqlDataOutput(out, CURRENT_VERSION)); } - public Request readRequest(DataInput in) throws IOException { + public SqlDataInput clientStream(DataInput in) throws IOException { int clientVersion = readHeader(in); if (clientVersion > CURRENT_VERSION) { throw new IOException("Unknown client version [" + clientVersion + "]. Always upgrade client last."); } - return readRequestType(in).reader().read(new SqlDataInput(in, clientVersion)); + return new SqlDataInput(in, clientVersion); + } + + public Request readRequest(SqlDataInput in) throws IOException { + return readRequestType(in).reader().read(in); + } + + public Request readRequest(DataInput in) throws IOException { + SqlDataInput client = clientStream(in); + return readRequest(client); } public void writeResponse(Response response, int clientVersion, DataOutput out) throws IOException { diff --git a/sql/shared-proto/src/main/java/org/elasticsearch/xpack/sql/protocol/shared/SqlDataInput.java b/sql/shared-proto/src/main/java/org/elasticsearch/xpack/sql/protocol/shared/SqlDataInput.java index bfce72ab968..e6d692661ce 100644 --- a/sql/shared-proto/src/main/java/org/elasticsearch/xpack/sql/protocol/shared/SqlDataInput.java +++ b/sql/shared-proto/src/main/java/org/elasticsearch/xpack/sql/protocol/shared/SqlDataInput.java @@ -108,4 +108,4 @@ import java.io.IOException; public String readUTF() throws IOException { return delegate.readUTF(); } -} +} \ No newline at end of file