From 2bbb86eff76e342aaba11409dd7f28ec0cd4a9cb Mon Sep 17 00:00:00 2001 From: Costin Leau Date: Fri, 13 Oct 2017 00:29:55 +0300 Subject: [PATCH] Eliminate NOCOMMITS per discussion Original commit: elastic/x-pack-elasticsearch@bd091a6608c9dc6d7a6977046fbf63551d4e3ea5 --- .../xpack/sql/cli/ResponseToString.java | 6 ++---- .../sql/jdbc/net/protocol/ProtoUtils.java | 6 +++--- .../xpack/sql/jdbc/debug/Debug.java | 21 ++++++++++++++++--- .../xpack/sql/expression/NamedExpression.java | 9 ++++---- .../xpack/sql/session/SingletonRowSet.java | 5 +++-- 5 files changed, 30 insertions(+), 17 deletions(-) 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 51a559f1b71..d02b96c689c 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 @@ -10,7 +10,6 @@ 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.Proto.ResponseType; 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; @@ -18,6 +17,7 @@ import java.io.IOException; import java.nio.charset.StandardCharsets; import java.nio.file.Files; import java.nio.file.Path; +import java.nio.file.Paths; import static org.jline.utils.AttributedStyle.BOLD; import static org.jline.utils.AttributedStyle.BRIGHT; @@ -73,12 +73,10 @@ abstract class ResponseToString { sb.append("]", BOLD.underlineOff().foreground(RED)); } - // 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", ".dot"); + Path dotTempFile = Files.createTempFile(Paths.get("."), "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(); 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 b515de1babc..269871d2f84 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 @@ -21,11 +21,11 @@ public class ProtoUtils { if (hasNext == 0) { // TODO feels like a bitmask at the start of the row would be better. return null; } - // NOCOMMIT we ought to make sure we use all of these + // TODO we ought to make sure we use all of these switch (type) { case NULL: // used to move the stream forward - // NOCOMMIT why serialize NULL types at all? + // TODO why serialize NULL types at all? in.readBoolean(); return null; case BIT: @@ -83,7 +83,7 @@ public class ProtoUtils { out.writeByte(1); switch (type) { - // NOCOMMIT we ought to make sure we use all of these + // TODO: we ought to make sure we use all of these case NULL: // used to move the stream forward out.writeBoolean(false); diff --git a/sql/jdbc/src/main/java/org/elasticsearch/xpack/sql/jdbc/debug/Debug.java b/sql/jdbc/src/main/java/org/elasticsearch/xpack/sql/jdbc/debug/Debug.java index 186985d0e17..2e32d733b6a 100644 --- a/sql/jdbc/src/main/java/org/elasticsearch/xpack/sql/jdbc/debug/Debug.java +++ b/sql/jdbc/src/main/java/org/elasticsearch/xpack/sql/jdbc/debug/Debug.java @@ -31,6 +31,21 @@ import java.util.Map; import javax.sql.DataSource; +/** + * Class handling debug logging. Typically disabled (hence why it's called debug). + * JDBC carries a lot of legacy conventions, logging being one of them - in JDBC logging was expected to + * be to System.Err/Out since there were no logging frameworks at the time. + * This didn't work so the API was changed through {@link DriverManager#getLogStream()} however that also had issues + * being global and not working well with encoding (hence why {@link DriverManager#getLogWriter()} was introduced) + * and was changed again through {@link DataSource#getLogWriter()}. + * However by then the damage was done and most drivers don't use either and have their own logging implementation. + * + * This class tries to cater to both audience - use the legacy, Writer way if needed though strive to use the + * proper typical approach, that of specifying intention and output (file) in the URL. + * + * For this reason the {@link System#out} and {@link System#err} are being refered in this class though are used only + * when needed. + */ public final class Debug { // cache for streams created by ourselves @@ -211,12 +226,12 @@ public final class Debug { OUTPUT_MANAGED.clear(); } - // NOCOMMIT loggers instead, I think - CL: what if the user wants to output to system.err/out like in an embedded app? - @SuppressForbidden(reason="temporary") + @SuppressForbidden(reason = "JDBC drivers allows logging to Sys.out") private static PrintStream stdout() { return System.out; } - @SuppressForbidden(reason="temporary") + + @SuppressForbidden(reason = "JDBC drivers allows logging to Sys.err") private static PrintStream stderr() { return System.err; } 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 c8140bbfeb7..d80e8bfbd51 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 @@ -5,11 +5,11 @@ */ package org.elasticsearch.xpack.sql.expression; +import org.elasticsearch.xpack.sql.tree.Location; + import java.util.List; import java.util.Objects; -import org.elasticsearch.xpack.sql.tree.Location; - public abstract class NamedExpression extends Expression { private final String name; @@ -42,9 +42,8 @@ public abstract class NamedExpression extends Expression { public abstract Attribute toAttribute(); @Override - public final int hashCode() { - // NOCOMMIT making this final upsets checkstyle. - return id.hashCode(); + public int hashCode() { + return Objects.hash(id, name, synthetic); } @Override diff --git a/sql/server/src/main/java/org/elasticsearch/xpack/sql/session/SingletonRowSet.java b/sql/server/src/main/java/org/elasticsearch/xpack/sql/session/SingletonRowSet.java index fd2a3986803..4236dbb1b47 100644 --- a/sql/server/src/main/java/org/elasticsearch/xpack/sql/session/SingletonRowSet.java +++ b/sql/server/src/main/java/org/elasticsearch/xpack/sql/session/SingletonRowSet.java @@ -7,7 +7,8 @@ package org.elasticsearch.xpack.sql.session; import org.elasticsearch.xpack.sql.type.Schema; -class SingletonRowSet extends AbstractRowSet { // NOCOMMIT is it worth keeping this when we have ListRowSet? +//TODO is it worth keeping this when we have ListRowSet? +class SingletonRowSet extends AbstractRowSet { private final Object[] values; @@ -45,4 +46,4 @@ class SingletonRowSet extends AbstractRowSet { // NOCOMMIT is it worth keeping t public Cursor nextPageCursor() { return Cursor.EMPTY; } -} +} \ No newline at end of file