From a4915a571400c404a5aad42bc4fbb8bf9d41a95a Mon Sep 17 00:00:00 2001 From: Igor Motov Date: Tue, 21 Nov 2017 14:37:59 -0500 Subject: [PATCH] SQL: remove all remaining NOCOMMITs relates elastic/x-pack-elasticsearch#2873 Original commit: elastic/x-pack-elasticsearch@68b206efd2f5a1efc269ca82bbd9618efd5ac36b --- dev-tools/ci | 7 +------ docs/en/index.asciidoc | 3 ++- .../src/main/resources/setup_mock_metadata_get_columns.sql | 2 +- .../src/main/java/org/elasticsearch/xpack/sql/cli/Cli.java | 3 ++- .../xpack/sql/jdbc/net/client/HttpClient.java | 4 ++-- sql/server/build.gradle | 2 +- .../elasticsearch/xpack/sql/analysis/catalog/EsIndex.java | 3 ++- .../sql/execution/search/extractor/DocValueExtractor.java | 3 ++- .../sql/execution/search/extractor/SourceExtractor.java | 3 ++- .../xpack/sql/plugin/AbstractSqlProtocolRestAction.java | 3 ++- .../org/elasticsearch/xpack/sql/plugin/CliFormatter.java | 6 ++++-- .../execution/search/extractor/InnerHitExtractorTests.java | 3 ++- 12 files changed, 23 insertions(+), 19 deletions(-) diff --git a/dev-tools/ci b/dev-tools/ci index 2a19e88dfb1..336928f4240 100755 --- a/dev-tools/ci +++ b/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) diff --git a/docs/en/index.asciidoc b/docs/en/index.asciidoc index 1f8fdecde97..3999f7593ed 100644 --- a/docs/en/index.asciidoc +++ b/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[] diff --git a/qa/sql/src/main/resources/setup_mock_metadata_get_columns.sql b/qa/sql/src/main/resources/setup_mock_metadata_get_columns.sql index 8b39212d08f..6d221f6c135 100644 --- a/qa/sql/src/main/resources/setup_mock_metadata_get_columns.sql +++ b/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 diff --git a/sql/cli/src/main/java/org/elasticsearch/xpack/sql/cli/Cli.java b/sql/cli/src/main/java/org/elasticsearch/xpack/sql/cli/Cli.java index 169b4bab4d2..f0ab8202a3c 100644 --- a/sql/cli/src/main/java/org/elasticsearch/xpack/sql/cli/Cli.java +++ b/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) { diff --git a/sql/jdbc/src/main/java/org/elasticsearch/xpack/sql/jdbc/net/client/HttpClient.java b/sql/jdbc/src/main/java/org/elasticsearch/xpack/sql/jdbc/net/client/HttpClient.java index d4fbb417952..48849787536 100644 --- a/sql/jdbc/src/main/java/org/elasticsearch/xpack/sql/jdbc/net/client/HttpClient.java +++ b/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]"); diff --git a/sql/server/build.gradle b/sql/server/build.gradle index 8a2779dcf68..0e7b46dd1ad 100644 --- a/sql/server/build.gradle +++ b/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" diff --git a/sql/server/src/main/java/org/elasticsearch/xpack/sql/analysis/catalog/EsIndex.java b/sql/server/src/main/java/org/elasticsearch/xpack/sql/analysis/catalog/EsIndex.java index 7421c90c41d..43694b39ad8 100644 --- a/sql/server/src/main/java/org/elasticsearch/xpack/sql/analysis/catalog/EsIndex.java +++ b/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 mapping; private final List aliases; diff --git a/sql/server/src/main/java/org/elasticsearch/xpack/sql/execution/search/extractor/DocValueExtractor.java b/sql/server/src/main/java/org/elasticsearch/xpack/sql/execution/search/extractor/DocValueExtractor.java index 7e1c90bd748..1d05748e0a0 100644 --- a/sql/server/src/main/java/org/elasticsearch/xpack/sql/execution/search/extractor/DocValueExtractor.java +++ b/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(); diff --git a/sql/server/src/main/java/org/elasticsearch/xpack/sql/execution/search/extractor/SourceExtractor.java b/sql/server/src/main/java/org/elasticsearch/xpack/sql/execution/search/extractor/SourceExtractor.java index 94313bb2450..2de0fbc862e 100644 --- a/sql/server/src/main/java/org/elasticsearch/xpack/sql/execution/search/extractor/SourceExtractor.java +++ b/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 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; } diff --git a/sql/server/src/main/java/org/elasticsearch/xpack/sql/plugin/AbstractSqlProtocolRestAction.java b/sql/server/src/main/java/org/elasticsearch/xpack/sql/plugin/AbstractSqlProtocolRestAction.java index b763fe7f1dd..80ccf741805 100644 --- a/sql/server/src/main/java/org/elasticsearch/xpack/sql/plugin/AbstractSqlProtocolRestAction.java +++ b/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()); diff --git a/sql/server/src/main/java/org/elasticsearch/xpack/sql/plugin/CliFormatter.java b/sql/server/src/main/java/org/elasticsearch/xpack/sql/plugin/CliFormatter.java index da7dd476766..a88248fcc71 100644 --- a/sql/server/src/main/java/org/elasticsearch/xpack/sql/plugin/CliFormatter.java +++ b/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 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 diff --git a/sql/server/src/test/java/org/elasticsearch/xpack/sql/execution/search/extractor/InnerHitExtractorTests.java b/sql/server/src/test/java/org/elasticsearch/xpack/sql/execution/search/extractor/InnerHitExtractorTests.java index 099c682bb4d..b05cb8710fd 100644 --- a/sql/server/src/test/java/org/elasticsearch/xpack/sql/execution/search/extractor/InnerHitExtractorTests.java +++ b/sql/server/src/test/java/org/elasticsearch/xpack/sql/execution/search/extractor/InnerHitExtractorTests.java @@ -30,8 +30,9 @@ public class InnerHitExtractorTests extends AbstractWireSerializingTestCase