SQL: remove all remaining NOCOMMITs

relates elastic/x-pack-elasticsearch#2873

Original commit: elastic/x-pack-elasticsearch@68b206efd2

dev-tools/ci

@@ -53,12 +53,11 @@ case $key in
         GRADLE_CLI_ARGS=(
             "--info"
             "check"
-            "-xforbiddenPatterns"   # NOCOMMIT this is required *for now* but will be removed when we remove the NOCOMMITs
             "-Dtests.network=true"
             "-Dtests.badapples=true"
         )
         ;;
-    smokeTestSql)  # NOCOMMIT remove this once we are ready to merge sql down
+    smokeTestSql)  # TODO remove this once we are ready to merge sql down
         GRADLE_CLI_ARGS=(
             "--info"
             "-psql"
@@ -68,10 +67,6 @@ case $key in
             ":x-pack-elasticsearch:qa:sql:multinode:check"
             ":x-pack-elasticsearch:qa:sql:no-security:check"
             ":x-pack-elasticsearch:qa:sql:security:check"
-            "-x:x-pack-elasticsearch:sql:cli:forbiddenPatterns"
-            "-x:x-pack-elasticsearch:sql:jdbc:forbiddenPatterns"
-            "-x:x-pack-elasticsearch:sql:server:forbiddenPatterns"
-            "-x:x-pack-elasticsearch:qa:sql:forbiddenPatterns"
         )
         ;;
     releaseTest)

docs/en/index.asciidoc

@@ -28,7 +28,8 @@ include::sql/index.asciidoc[]
 include::monitoring/index.asciidoc[]
 include::rest-api/index.asciidoc[]
 
-# NOCOMMIT before merging SQL we need to fiddle with this to make sure it is right
+# TODO before merging SQL we need to fiddle with this to make sure it is right
+# Tracked by https://github.com/elastic/x-pack-elasticsearch/issues/3084
 
 include::commands/index.asciidoc[]
 

qa/sql/src/main/resources/setup_mock_metadata_get_columns.sql

@@ -24,7 +24,7 @@ CREATE TABLE mock (
   IS_GENERATEDCOLUMN VARCHAR
 ) AS
 SELECT '', 'test1', 'name', 12, 'VARCHAR', 2147483647, null, null,
-  10, -- NOCOMMIT 10 seem wrong to hard code for stuff like strings
+  10, -- TODO 10 seem wrong to hard code for non-numbers see https://github.com/elastic/x-pack-elasticsearch/issues/3085
   1, -- columnNullable
   null, null, null, null, null, 1, 'YES', null, null, null, null, '', ''
 FROM DUAL

sql/cli/src/main/java/org/elasticsearch/xpack/sql/cli/Cli.java

@@ -69,7 +69,8 @@ public class Cli {
             }
             user = parsed.getUserInfo();
             if (user != null) {
-                // NOCOMMIT just use a URI the whole time
+                // TODO just use a URI the whole time
+                // Tracked by https://github.com/elastic/x-pack-elasticsearch/issues/2882
                 hostAndPort = parsed.getScheme() + "://" + parsed.getHost() + ":" + parsed.getPort();
                 int colonIndex = user.indexOf(':');
                 if (colonIndex >= 0) {

sql/jdbc/src/main/java/org/elasticsearch/xpack/sql/jdbc/net/client/HttpClient.java

@@ -5,7 +5,6 @@
  */
 package org.elasticsearch.xpack.sql.jdbc.net.client;
 
-import org.elasticsearch.xpack.sql.client.shared.CheckedConsumer;
 import org.elasticsearch.xpack.sql.client.shared.ClientException;
 import org.elasticsearch.xpack.sql.client.shared.JreHttpUrlConnection;
 import org.elasticsearch.xpack.sql.jdbc.JdbcException;
@@ -37,7 +36,8 @@ class HttpClient {
         URL baseUrl = connectionInfo.asUrl();
         try {
             // the baseUrl ends up / so the suffix can be directly appended
-            // NOCOMMIT Do something with the error trace. Useful for filing bugs and debugging.
+            // TODO Do something with the error trace. Useful for filing bugs and debugging.
+            // Tracked by https://github.com/elastic/x-pack-elasticsearch/issues/3079
             this.url = new URL(baseUrl, "_sql/jdbc?error_trace=true");
         } catch (MalformedURLException ex) {
             throw new JdbcException(ex, "Cannot connect to JDBC endpoint [" + baseUrl.toString() + "_sql/jdbc]");

sql/server/build.gradle

@@ -19,7 +19,7 @@ dependencyLicenses {
   ignoreSha 'shared-proto'
 }
 
-// NOCOMMIT probably not a good thing to rely on.....
+// TODO probably not a good thing to rely on. See https://github.com/elastic/x-pack-elasticsearch/issues/2871
 compileJava.options.compilerArgs << "-parameters"
 compileTestJava.options.compilerArgs << "-parameters"
 

sql/server/src/main/java/org/elasticsearch/xpack/sql/analysis/catalog/EsIndex.java

@@ -19,7 +19,8 @@ public class EsIndex {
 
     public static final EsIndex NOT_FOUND = new EsIndex(StringUtils.EMPTY, emptyMap(), emptyList(), Settings.EMPTY);
 
-    // NOCOMMIT Double check that we need these and that we're securing them properly.
+    // TODO Double check that we need these and that we're securing them properly.
+    // Tracked by https://github.com/elastic/x-pack-elasticsearch/issues/3076
     private final String name;
     private final Map<String, DataType> mapping;
     private final List<String> aliases;

sql/server/src/main/java/org/elasticsearch/xpack/sql/execution/search/extractor/DocValueExtractor.java

@@ -40,7 +40,8 @@ public class DocValueExtractor implements HitExtractor {
 
     @Override
     public Object get(SearchHit hit) {
-        // NOCOMMIT we should think about what to do with multi-valued fields.
+        // TODO we should think about what to do with multi-valued fields.
+        // Tracked by https://github.com/elastic/x-pack-elasticsearch/issues/2874
         DocumentField field = hit.field(fieldName);
         if (field != null) {
             Object value = field.getValue();

sql/server/src/main/java/org/elasticsearch/xpack/sql/execution/search/extractor/SourceExtractor.java

@@ -37,8 +37,9 @@ public class SourceExtractor implements HitExtractor {
     @Override
     public Object get(SearchHit hit) {
         Map<String, Object> source = hit.getSourceAsMap();
-        // NOCOMMIT I think this will not work with dotted field names (objects or actual dots in the names)
+        // TODO I think this will not work with dotted field names (objects or actual dots in the names)
         // confusingly, I think this is actually handled by InnerHitExtractor. This needs investigating or renaming
+        // Tracked by https://github.com/elastic/x-pack-elasticsearch/issues/2874
         return source != null ? source.get(fieldName) : null;
     }
 

sql/server/src/main/java/org/elasticsearch/xpack/sql/plugin/AbstractSqlProtocolRestAction.java

@@ -50,7 +50,8 @@ public abstract class AbstractSqlProtocolRestAction extends BaseRestHandler {
             public RestResponse buildResponse(T response) throws Exception {
                 try (BytesStreamOutput bytesStreamOutput = new BytesStreamOutput()) {
                     try (DataOutputStream dataOutputStream = new DataOutputStream(bytesStreamOutput)) {
-                        // NOCOMMIT use the version from the client
+                        // TODO use the version from the client
+                        // Tracked by https://github.com/elastic/x-pack-elasticsearch/issues/3080
                         proto.writeResponse(responseBuilder.apply(response), Proto.CURRENT_VERSION, dataOutputStream);
                     }
                     return new BytesRestResponse(OK, TEXT_CONTENT_TYPE, bytesStreamOutput.bytes());

sql/server/src/main/java/org/elasticsearch/xpack/sql/plugin/CliFormatter.java

@@ -42,7 +42,8 @@ public class CliFormatter implements Writeable {
         // 2. Expand columns to fit the largest value
         for (List<Object> row : response.rows()) {
             for (int i = 0; i < width.length; i++) {
-                // NOCOMMIT are we sure toString is correct here? What about dates that come back as longs.
+                // TODO are we sure toString is correct here? What about dates that come back as longs.
+                // Tracked by https://github.com/elastic/x-pack-elasticsearch/issues/3081
                 width[i] = Math.max(width[i], Objects.toString(row.get(i)).length());
             }
         }
@@ -115,7 +116,8 @@ public class CliFormatter implements Writeable {
                     sb.append('|');
                 }
 
-                // NOCOMMIT are we sure toString is correct here? What about dates that come back as longs.
+                // TODO are we sure toString is correct here? What about dates that come back as longs.
+                // Tracked by https://github.com/elastic/x-pack-elasticsearch/issues/3081
                 String string = Objects.toString(row.get(i));
                 if (string.length() <= width[i]) {
                     // Pad

sql/server/src/test/java/org/elasticsearch/xpack/sql/execution/search/extractor/InnerHitExtractorTests.java

@@ -30,8 +30,9 @@ public class InnerHitExtractorTests extends AbstractWireSerializingTestCase<Inne
         return new InnerHitExtractor(instance.hitName() + "mustated", instance.fieldName(), true);
     }
 
+    @AwaitsFix(bugUrl = "https://github.com/elastic/x-pack-elasticsearch/issues/3082")
     public void testGet() throws IOException {
-        // NOCOMMIT implement after we're sure of the InnerHitExtractor's implementation
+        fail("implement after we're sure of the InnerHitExtractor's implementation");
     }
 
     public void testToString() {