diff --git a/qa/sql/src/main/java/org/elasticsearch/xpack/qa/sql/jdbc/SqlSpecTestCase.java b/qa/sql/src/main/java/org/elasticsearch/xpack/qa/sql/jdbc/SqlSpecTestCase.java index 613c4454777..fa1a7121655 100644 --- a/qa/sql/src/main/java/org/elasticsearch/xpack/qa/sql/jdbc/SqlSpecTestCase.java +++ b/qa/sql/src/main/java/org/elasticsearch/xpack/qa/sql/jdbc/SqlSpecTestCase.java @@ -38,7 +38,7 @@ public abstract class SqlSpecTestCase extends SpecBaseIntegrationTestCase { ); } - // NOCOMMIT: add tests for nested docs when interplug communication is enabled + // TODO: add tests for nested docs when interplug communication is enabled // "DESCRIBE emp.emp", // "SELECT dep FROM emp.emp", // "SELECT dep.dept_name, first_name, last_name FROM emp.emp WHERE emp_no = 10020", diff --git a/sql/cli/src/main/java/org/elasticsearch/xpack/sql/cli/ResponseToString.java b/sql/cli/src/main/java/org/elasticsearch/xpack/sql/cli/ResponseToString.java index 06c571bef92..739c5489a12 100644 --- a/sql/cli/src/main/java/org/elasticsearch/xpack/sql/cli/ResponseToString.java +++ b/sql/cli/src/main/java/org/elasticsearch/xpack/sql/cli/ResponseToString.java @@ -5,16 +5,14 @@ */ package org.elasticsearch.xpack.sql.cli; -import org.elasticsearch.xpack.sql.cli.net.protocol.QueryResponse; import org.elasticsearch.xpack.sql.cli.net.protocol.ErrorResponse; import org.elasticsearch.xpack.sql.cli.net.protocol.ExceptionResponse; import org.elasticsearch.xpack.sql.cli.net.protocol.InfoResponse; +import org.elasticsearch.xpack.sql.cli.net.protocol.QueryResponse; import org.elasticsearch.xpack.sql.net.client.SuppressForbidden; import org.elasticsearch.xpack.sql.protocol.shared.Response; import org.jline.utils.AttributedStringBuilder; -import java.awt.Desktop; -import java.io.File; import java.io.IOException; import java.nio.charset.StandardCharsets; import java.nio.file.Files; @@ -37,7 +35,7 @@ abstract class ResponseToString { if (cmd.data != null) { String data = cmd.data.toString(); if (data.startsWith("digraph ")) { - displayGraphviz(data); + sb.append(handleGraphviz(data), DEFAULT.foreground(WHITE)); } else { sb.append(data, DEFAULT.foreground(WHITE)); @@ -69,20 +67,22 @@ abstract class ResponseToString { return sb; } - @SuppressForbidden(reason="ignore for now") // NOCOMMIT replace this with saving the file and printing a message - private static void displayGraphviz(String str) { + // NOCOMMIT - is using the default temp folder a problem? + @SuppressForbidden(reason = "need to use temporary file") + private static String handleGraphviz(String str) { try { // save the content to a temp file - Path dotTempFile = Files.createTempFile("sql-gv", ".dot2img"); + Path dotTempFile = Files.createTempFile("sql-gv", ".dot"); Files.write(dotTempFile, str.getBytes(StandardCharsets.UTF_8)); // run graphviz on it (dot needs to be on the file path) - Desktop desktop = Desktop.getDesktop(); - File f = dotTempFile.toFile(); - desktop.open(f); - f.deleteOnExit(); + //Desktop desktop = Desktop.getDesktop(); + //File f = dotTempFile.toFile(); + //desktop.open(f); + //f.deleteOnExit(); + return "Saved graph file at " + dotTempFile; } catch (IOException ex) { - // nope + return "Cannot save graph file; " + ex.getMessage(); } } } diff --git a/sql/jdbc-proto/src/main/java/org/elasticsearch/xpack/sql/jdbc/net/protocol/Page.java b/sql/jdbc-proto/src/main/java/org/elasticsearch/xpack/sql/jdbc/net/protocol/Page.java index e1244270660..ac846412150 100644 --- a/sql/jdbc-proto/src/main/java/org/elasticsearch/xpack/sql/jdbc/net/protocol/Page.java +++ b/sql/jdbc-proto/src/main/java/org/elasticsearch/xpack/sql/jdbc/net/protocol/Page.java @@ -75,7 +75,7 @@ public class Page implements Payload { Object[] column(int index) { if (index < 0 || index >= data.length) { - // NOCOMMIT this was once JdbcException. Make sure it isn't now busted + // NB: exception is caught higher up in the JDBC driver throw new IllegalArgumentException("Invalid column [" + index + "] (max is [" + (data.length - 1) + "])"); } @@ -84,7 +84,7 @@ public class Page implements Payload { public Object entry(int row, int column) { if (row < 0 || row >= rows) { - // NOCOMMIT this was once JdbcException. Make sure it isn't now busted + // NB: exception is caught higher up in the JDBC driver throw new IllegalArgumentException("Invalid row [" + row + "] (max is [" + (rows -1) + "])"); } return column(column)[row]; diff --git a/sql/jdbc-proto/src/main/java/org/elasticsearch/xpack/sql/jdbc/net/protocol/ProtoUtils.java b/sql/jdbc-proto/src/main/java/org/elasticsearch/xpack/sql/jdbc/net/protocol/ProtoUtils.java index 72eeb6fca86..b515de1babc 100644 --- a/sql/jdbc-proto/src/main/java/org/elasticsearch/xpack/sql/jdbc/net/protocol/ProtoUtils.java +++ b/sql/jdbc-proto/src/main/java/org/elasticsearch/xpack/sql/jdbc/net/protocol/ProtoUtils.java @@ -138,6 +138,8 @@ public class ProtoUtils { /** * The type of the array used to store columns of this type. */ + // NB: JDBC requires the use of Objects not primitive + // (in fact primitives are never used through-out the API) public static Class classOf(JDBCType jdbcType) { switch (jdbcType) { case NUMERIC: @@ -151,7 +153,6 @@ public class ProtoUtils { case SMALLINT: return Short.class; case INTEGER: - // NOCOMMIT should we be using primitives instead? return Integer.class; case BIGINT: return Long.class; diff --git a/sql/jdbc-proto/src/main/java/org/elasticsearch/xpack/sql/jdbc/net/protocol/QueryInitResponse.java b/sql/jdbc-proto/src/main/java/org/elasticsearch/xpack/sql/jdbc/net/protocol/QueryInitResponse.java index 4ce52419ae1..0d98156fab7 100644 --- a/sql/jdbc-proto/src/main/java/org/elasticsearch/xpack/sql/jdbc/net/protocol/QueryInitResponse.java +++ b/sql/jdbc-proto/src/main/java/org/elasticsearch/xpack/sql/jdbc/net/protocol/QueryInitResponse.java @@ -70,18 +70,19 @@ public class QueryInitResponse extends AbstractQueryResponse { return ResponseType.QUERY_INIT; } + @Override + public int hashCode() { + return Objects.hash(super.hashCode(), columns, data); + } + + @Override public boolean equals(Object obj) { if (false == super.equals(obj)) { return false; } QueryInitResponse other = (QueryInitResponse) obj; - return columns.equals(other.columns); - // NOCOMMIT equals should take into account data - } - - @Override - public int hashCode() { - return Objects.hash(super.hashCode(), columns); // NOCOMMIT equals should take into account data + return Objects.equals(columns, other.columns) + && Objects.equals(data, other.data); } } \ No newline at end of file diff --git a/sql/server/src/main/java/org/elasticsearch/xpack/sql/parser/CaseInsensitiveStream.java b/sql/server/src/main/java/org/elasticsearch/xpack/sql/parser/CaseInsensitiveStream.java index f5844bf7742..fff954ab592 100644 --- a/sql/server/src/main/java/org/elasticsearch/xpack/sql/parser/CaseInsensitiveStream.java +++ b/sql/server/src/main/java/org/elasticsearch/xpack/sql/parser/CaseInsensitiveStream.java @@ -5,15 +5,18 @@ */ package org.elasticsearch.xpack.sql.parser; -import java.util.Locale; - import org.antlr.v4.runtime.ANTLRInputStream; import org.antlr.v4.runtime.IntStream; -// extension of ANTLR that does the uppercasing once for the whole stream +import java.util.Locale; + +// extension of ANTLR that does the upper-casing once for the whole stream // the ugly part is that it has to duplicate LA method + +// This approach is the official solution from the ANTLR authors +// in that it's both faster and easier than having a dedicated lexer +// see https://github.com/antlr/antlr4/issues/1002 class CaseInsensitiveStream extends ANTLRInputStream { - // NOCOMMIT maybe we can fix this in the lexer or on the way in so we don't need the LA override protected char[] uppedChars; CaseInsensitiveStream(String input) { diff --git a/sql/server/src/main/java/org/elasticsearch/xpack/sql/plugin/SqlResponsePayload.java b/sql/server/src/main/java/org/elasticsearch/xpack/sql/plugin/SqlResponsePayload.java index dd88d88d0d5..4ae68da490e 100644 --- a/sql/server/src/main/java/org/elasticsearch/xpack/sql/plugin/SqlResponsePayload.java +++ b/sql/server/src/main/java/org/elasticsearch/xpack/sql/plugin/SqlResponsePayload.java @@ -15,6 +15,7 @@ import org.joda.time.ReadableInstant; import java.io.IOException; import java.sql.JDBCType; import java.util.List; +import java.util.Objects; /** * Implementation {@link Payload} that adapts it to data from @@ -51,6 +52,25 @@ class SqlResponsePayload implements Payload { ProtoUtils.writeValue(out, value, type); } } - + } + + @Override + public int hashCode() { + return Objects.hash(typeLookup, rows); + } + + @Override + public boolean equals(Object obj) { + if (this == obj) { + return true; + } + + if (obj == null || getClass() != obj.getClass()) { + return false; + } + + SqlResponsePayload other = (SqlResponsePayload) obj; + return Objects.equals(typeLookup, other.typeLookup) + && Objects.equals(rows, other.rows); } }