Eliminate NOCOMMITS per discussion

Original commit: elastic/x-pack-elasticsearch@bd091a6608

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

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

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 = "JDBC drivers allows logging to Sys.out")
-    @SuppressForbidden(reason="temporary")
     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;
     }

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() {
+    public int hashCode() {
-        // NOCOMMIT making this final upsets checkstyle.
+        return Objects.hash(id, name, synthetic);
-        return id.hashCode();
     }
 
     @Override

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