Fix some NOCOMMITs

Original commit: elastic/x-pack-elasticsearch@1a6ac1e6c6

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",

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();
         }
     }
 }

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];

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;

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);
     }
 }

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) {

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);
     }
 }