From 5373a77fb9a94915d9b2c44279d0dc4edee9a0ef Mon Sep 17 00:00:00 2001 From: Costin Leau Date: Thu, 13 Feb 2020 14:14:42 +0200 Subject: [PATCH] QL: Extract common Failure class (#52281) Shared across SQL and EQL (cherry picked from commit 1aeda20d3ec3d6c885de03c6043dd1e8eab9f230) --- .../xpack/eql/analysis/Analyzer.java | 2 +- .../eql/analysis/VerificationException.java | 2 +- .../xpack/eql/analysis/Verifier.java | 4 +- .../xpack/eql/planner/Planner.java | 2 +- .../xpack/eql/planner/PlanningException.java | 2 +- .../xpack/eql/planner/Verifier.java | 4 +- .../xpack/ql}/common/Failure.java | 8 +-- .../xpack/sql/qa/cli/ErrorsTestCase.java | 2 +- .../xpack/sql/qa/jdbc/ErrorsTestCase.java | 20 +++---- .../xpack/sql/analysis/AnalysisException.java | 59 ------------------- .../xpack/sql/analysis/analyzer/Analyzer.java | 2 +- .../analyzer/VerificationException.java | 25 +++----- .../xpack/sql/analysis/analyzer/Verifier.java | 50 +--------------- .../xpack/sql/planner/Planner.java | 6 +- .../xpack/sql/planner/PlanningException.java | 15 +---- .../xpack/sql/planner/Verifier.java | 46 +-------------- .../analyzer/FieldAttributeTests.java | 10 ++-- .../analyzer/VerifierErrorMessagesTests.java | 8 ++- .../sql/execution/search/QuerierTests.java | 2 +- .../planner/PostOptimizerVerifierTests.java | 2 +- 20 files changed, 52 insertions(+), 219 deletions(-) rename x-pack/plugin/{eql/src/main/java/org/elasticsearch/xpack/eql => ql/src/main/java/org/elasticsearch/xpack/ql}/common/Failure.java (89%) delete mode 100644 x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/AnalysisException.java diff --git a/x-pack/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/analysis/Analyzer.java b/x-pack/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/analysis/Analyzer.java index 5af9d9ec747..82ce55ac74f 100644 --- a/x-pack/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/analysis/Analyzer.java +++ b/x-pack/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/analysis/Analyzer.java @@ -6,7 +6,7 @@ package org.elasticsearch.xpack.eql.analysis; -import org.elasticsearch.xpack.eql.common.Failure; +import org.elasticsearch.xpack.ql.common.Failure; import org.elasticsearch.xpack.ql.expression.Attribute; import org.elasticsearch.xpack.ql.expression.NamedExpression; import org.elasticsearch.xpack.ql.expression.UnresolvedAttribute; diff --git a/x-pack/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/analysis/VerificationException.java b/x-pack/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/analysis/VerificationException.java index afb3297ab2e..b5f7fdab9d8 100644 --- a/x-pack/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/analysis/VerificationException.java +++ b/x-pack/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/analysis/VerificationException.java @@ -7,7 +7,7 @@ package org.elasticsearch.xpack.eql.analysis; import org.elasticsearch.rest.RestStatus; import org.elasticsearch.xpack.eql.EqlClientException; -import org.elasticsearch.xpack.eql.common.Failure; +import org.elasticsearch.xpack.ql.common.Failure; import java.util.Collection; diff --git a/x-pack/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/analysis/Verifier.java b/x-pack/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/analysis/Verifier.java index 66c363fe844..0d526c4d052 100644 --- a/x-pack/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/analysis/Verifier.java +++ b/x-pack/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/analysis/Verifier.java @@ -6,8 +6,8 @@ package org.elasticsearch.xpack.eql.analysis; -import org.elasticsearch.xpack.eql.common.Failure; import org.elasticsearch.xpack.ql.capabilities.Unresolvable; +import org.elasticsearch.xpack.ql.common.Failure; import org.elasticsearch.xpack.ql.expression.Attribute; import org.elasticsearch.xpack.ql.expression.UnresolvedAttribute; import org.elasticsearch.xpack.ql.plan.logical.LogicalPlan; @@ -23,7 +23,7 @@ import java.util.Map; import java.util.Set; import static java.util.stream.Collectors.toMap; -import static org.elasticsearch.xpack.eql.common.Failure.fail; +import static org.elasticsearch.xpack.ql.common.Failure.fail; /** * The verifier has the role of checking the analyzed tree for failures and build a list of failures following this check. diff --git a/x-pack/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/planner/Planner.java b/x-pack/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/planner/Planner.java index b0bcce4d72f..e20b03533de 100644 --- a/x-pack/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/planner/Planner.java +++ b/x-pack/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/planner/Planner.java @@ -6,8 +6,8 @@ package org.elasticsearch.xpack.eql.planner; -import org.elasticsearch.xpack.eql.common.Failure; import org.elasticsearch.xpack.eql.plan.physical.PhysicalPlan; +import org.elasticsearch.xpack.ql.common.Failure; import org.elasticsearch.xpack.ql.plan.logical.LogicalPlan; import java.util.List; diff --git a/x-pack/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/planner/PlanningException.java b/x-pack/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/planner/PlanningException.java index d2f959c0e51..93c4c8638e2 100644 --- a/x-pack/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/planner/PlanningException.java +++ b/x-pack/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/planner/PlanningException.java @@ -7,7 +7,7 @@ package org.elasticsearch.xpack.eql.planner; import org.elasticsearch.rest.RestStatus; import org.elasticsearch.xpack.eql.EqlClientException; -import org.elasticsearch.xpack.eql.common.Failure; +import org.elasticsearch.xpack.ql.common.Failure; import java.util.Collection; diff --git a/x-pack/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/planner/Verifier.java b/x-pack/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/planner/Verifier.java index de4a68f28cf..82c09890b26 100644 --- a/x-pack/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/planner/Verifier.java +++ b/x-pack/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/planner/Verifier.java @@ -5,15 +5,15 @@ */ package org.elasticsearch.xpack.eql.planner; -import org.elasticsearch.xpack.eql.common.Failure; import org.elasticsearch.xpack.eql.plan.physical.PhysicalPlan; import org.elasticsearch.xpack.eql.plan.physical.Unexecutable; import org.elasticsearch.xpack.eql.plan.physical.UnplannedExec; +import org.elasticsearch.xpack.ql.common.Failure; import java.util.ArrayList; import java.util.List; -import static org.elasticsearch.xpack.eql.common.Failure.fail; +import static org.elasticsearch.xpack.ql.common.Failure.fail; abstract class Verifier { diff --git a/x-pack/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/common/Failure.java b/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/common/Failure.java similarity index 89% rename from x-pack/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/common/Failure.java rename to x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/common/Failure.java index 2172f6f5752..3c3e95aada2 100644 --- a/x-pack/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/common/Failure.java +++ b/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/common/Failure.java @@ -4,7 +4,7 @@ * you may not use this file except in compliance with the Elastic License. */ -package org.elasticsearch.xpack.eql.common; +package org.elasticsearch.xpack.ql.common; import org.elasticsearch.xpack.ql.tree.Location; import org.elasticsearch.xpack.ql.tree.Node; @@ -36,7 +36,7 @@ public class Failure { @Override public int hashCode() { - return Objects.hash(message, node); + return Objects.hash(node); } @Override @@ -50,7 +50,7 @@ public class Failure { } Failure other = (Failure) obj; - return Objects.equals(message, other.message) && Objects.equals(node, other.node); + return Objects.equals(node, other.node); } @Override @@ -67,7 +67,7 @@ public class Failure { Location l = f.node().source().source(); return "line " + l.getLineNumber() + ":" + l.getColumnNumber() + ": " + f.message(); }).collect(Collectors.joining(StringUtils.NEW_LINE, - format("Found {} problem{}\n", failures.size(), failures.size() > 1 ? "s" : StringUtils.EMPTY), + format("Found {} problem{}\n", failures.size(), failures.size() > 1 ? "s" : StringUtils.EMPTY), StringUtils.EMPTY)); } } diff --git a/x-pack/plugin/sql/qa/src/main/java/org/elasticsearch/xpack/sql/qa/cli/ErrorsTestCase.java b/x-pack/plugin/sql/qa/src/main/java/org/elasticsearch/xpack/sql/qa/cli/ErrorsTestCase.java index bd295d09029..2bef0cd60ae 100644 --- a/x-pack/plugin/sql/qa/src/main/java/org/elasticsearch/xpack/sql/qa/cli/ErrorsTestCase.java +++ b/x-pack/plugin/sql/qa/src/main/java/org/elasticsearch/xpack/sql/qa/cli/ErrorsTestCase.java @@ -106,6 +106,6 @@ public abstract class ErrorsTestCase extends CliIntegrationTestCase implements o } public static void assertFoundOneProblem(String commandResult) { - assertEquals(START + "Bad request [[3;33;22mFound 1 problem(s)", commandResult); + assertEquals(START + "Bad request [[3;33;22mFound 1 problem", commandResult); } } diff --git a/x-pack/plugin/sql/qa/src/main/java/org/elasticsearch/xpack/sql/qa/jdbc/ErrorsTestCase.java b/x-pack/plugin/sql/qa/src/main/java/org/elasticsearch/xpack/sql/qa/jdbc/ErrorsTestCase.java index d4f1cb5b305..87de389e9be 100644 --- a/x-pack/plugin/sql/qa/src/main/java/org/elasticsearch/xpack/sql/qa/jdbc/ErrorsTestCase.java +++ b/x-pack/plugin/sql/qa/src/main/java/org/elasticsearch/xpack/sql/qa/jdbc/ErrorsTestCase.java @@ -20,7 +20,7 @@ public class ErrorsTestCase extends JdbcIntegrationTestCase implements org.elast public void testSelectInvalidSql() throws Exception { try (Connection c = esJdbc()) { SQLException e = expectThrows(SQLException.class, () -> c.prepareStatement("SELECT * FRO").executeQuery()); - assertEquals("Found 1 problem(s)\nline 1:8: Cannot determine columns for [*]", e.getMessage()); + assertEquals("Found 1 problem\nline 1:8: Cannot determine columns for [*]", e.getMessage()); } } @@ -28,7 +28,7 @@ public class ErrorsTestCase extends JdbcIntegrationTestCase implements org.elast public void testSelectFromMissingIndex() throws SQLException { try (Connection c = esJdbc()) { SQLException e = expectThrows(SQLException.class, () -> c.prepareStatement("SELECT * FROM test").executeQuery()); - assertEquals("Found 1 problem(s)\nline 1:15: Unknown index [test]", e.getMessage()); + assertEquals("Found 1 problem\nline 1:15: Unknown index [test]", e.getMessage()); } } @@ -42,8 +42,8 @@ public class ErrorsTestCase extends JdbcIntegrationTestCase implements org.elast try (Connection c = esJdbc()) { SQLException e = expectThrows(SQLException.class, () -> c.prepareStatement("SELECT * FROM test").executeQuery()); // see https://github.com/elastic/elasticsearch/issues/34719 - //assertEquals("Found 1 problem(s)\nline 1:15: [test] doesn't have any types so it is incompatible with sql", e.getMessage()); - assertEquals("Found 1 problem(s)\nline 1:15: Unknown index [test]", e.getMessage()); + //assertEquals("Found 1 problem\nline 1:15: [test] doesn't have any types so it is incompatible with sql", e.getMessage()); + assertEquals("Found 1 problem\nline 1:15: Unknown index [test]", e.getMessage()); } } @@ -52,7 +52,7 @@ public class ErrorsTestCase extends JdbcIntegrationTestCase implements org.elast index("test", body -> body.field("test", "test")); try (Connection c = esJdbc()) { SQLException e = expectThrows(SQLException.class, () -> c.prepareStatement("SELECT missing FROM test").executeQuery()); - assertEquals("Found 1 problem(s)\nline 1:8: Unknown column [missing]", e.getMessage()); + assertEquals("Found 1 problem\nline 1:8: Unknown column [missing]", e.getMessage()); } } @@ -61,7 +61,7 @@ public class ErrorsTestCase extends JdbcIntegrationTestCase implements org.elast index("test", body -> body.field("foo", 1)); try (Connection c = esJdbc()) { SQLException e = expectThrows(SQLException.class, () -> c.prepareStatement("SELECT missing(foo) FROM test").executeQuery()); - assertEquals("Found 1 problem(s)\nline 1:8: Unknown function [missing]", e.getMessage()); + assertEquals("Found 1 problem\nline 1:8: Unknown function [missing]", e.getMessage()); } } @@ -71,7 +71,7 @@ public class ErrorsTestCase extends JdbcIntegrationTestCase implements org.elast try (Connection c = esJdbc()) { SQLException e = expectThrows(SQLException.class, () -> c.prepareStatement("SELECT foo, SCORE(), COUNT(*) FROM test GROUP BY foo").executeQuery()); - assertEquals("Found 1 problem(s)\nline 1:13: Cannot use non-grouped column [SCORE()], expected [foo]", e.getMessage()); + assertEquals("Found 1 problem\nline 1:13: Cannot use non-grouped column [SCORE()], expected [foo]", e.getMessage()); } } @@ -82,7 +82,7 @@ public class ErrorsTestCase extends JdbcIntegrationTestCase implements org.elast SQLException e = expectThrows(SQLException.class, () -> c.prepareStatement("SELECT foo, COUNT(*) FROM test GROUP BY foo ORDER BY SCORE()").executeQuery()); assertEquals( - "Found 1 problem(s)\nline 1:54: Cannot order by non-grouped column [SCORE()], expected [foo] or an aggregate function", + "Found 1 problem\nline 1:54: Cannot order by non-grouped column [SCORE()], expected [foo] or an aggregate function", e.getMessage()); } } @@ -93,7 +93,7 @@ public class ErrorsTestCase extends JdbcIntegrationTestCase implements org.elast try (Connection c = esJdbc()) { SQLException e = expectThrows(SQLException.class, () -> c.prepareStatement("SELECT COUNT(*) FROM test GROUP BY SCORE()").executeQuery()); - assertEquals("Found 1 problem(s)\nline 1:36: Cannot use [SCORE()] for grouping", e.getMessage()); + assertEquals("Found 1 problem\nline 1:36: Cannot use [SCORE()] for grouping", e.getMessage()); } } @@ -113,7 +113,7 @@ public class ErrorsTestCase extends JdbcIntegrationTestCase implements org.elast try (Connection c = esJdbc()) { SQLException e = expectThrows(SQLException.class, () -> c.prepareStatement("SELECT SIN(SCORE()) FROM test").executeQuery()); - assertThat(e.getMessage(), startsWith("Found 1 problem(s)\nline 1:12: [SCORE()] cannot be an argument to a function")); + assertThat(e.getMessage(), startsWith("Found 1 problem\nline 1:12: [SCORE()] cannot be an argument to a function")); } } diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/AnalysisException.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/AnalysisException.java deleted file mode 100644 index 4d3a799467e..00000000000 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/AnalysisException.java +++ /dev/null @@ -1,59 +0,0 @@ -/* - * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one - * or more contributor license agreements. Licensed under the Elastic License; - * you may not use this file except in compliance with the Elastic License. - */ -package org.elasticsearch.xpack.sql.analysis; - -import org.elasticsearch.rest.RestStatus; -import org.elasticsearch.xpack.ql.tree.Location; -import org.elasticsearch.xpack.ql.tree.Node; -import org.elasticsearch.xpack.sql.SqlClientException; - -import static org.elasticsearch.common.logging.LoggerMessageFormat.format; - -public class AnalysisException extends SqlClientException { - - private final int line; - private final int column; - - public AnalysisException(Node source, String message, Object... args) { - super(message, args); - - Location loc = Location.EMPTY; - if (source != null && source.source() != null) { - loc = source.source().source(); - } - this.line = loc.getLineNumber(); - this.column = loc.getColumnNumber(); - } - - public AnalysisException(Node source, String message, Throwable cause) { - super(message, cause); - - Location loc = Location.EMPTY; - if (source != null && source.source() != null) { - loc = source.source().source(); - } - this.line = loc.getLineNumber(); - this.column = loc.getColumnNumber(); - } - - public int getLineNumber() { - return line; - } - - public int getColumnNumber() { - return column; - } - - @Override - public RestStatus status() { - return RestStatus.BAD_REQUEST; - } - - @Override - public String getMessage() { - return format("line {}:{}: {}", getLineNumber(), getColumnNumber(), super.getMessage()); - } -} diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Analyzer.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Analyzer.java index 64c5c6d61b2..bc6ac7eaf90 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Analyzer.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Analyzer.java @@ -7,6 +7,7 @@ package org.elasticsearch.xpack.sql.analysis.analyzer; import org.elasticsearch.common.logging.LoggerMessageFormat; import org.elasticsearch.xpack.ql.capabilities.Resolvables; +import org.elasticsearch.xpack.ql.common.Failure; import org.elasticsearch.xpack.ql.expression.Alias; import org.elasticsearch.xpack.ql.expression.Attribute; import org.elasticsearch.xpack.ql.expression.AttributeMap; @@ -45,7 +46,6 @@ import org.elasticsearch.xpack.ql.type.InvalidMappedField; import org.elasticsearch.xpack.ql.type.UnsupportedEsField; import org.elasticsearch.xpack.ql.util.CollectionUtils; import org.elasticsearch.xpack.ql.util.Holder; -import org.elasticsearch.xpack.sql.analysis.analyzer.Verifier.Failure; import org.elasticsearch.xpack.sql.expression.Foldables; import org.elasticsearch.xpack.sql.expression.SubQueryExpression; import org.elasticsearch.xpack.sql.expression.function.scalar.Cast; diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/VerificationException.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/VerificationException.java index 0eba4126ba8..7a754065f3b 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/VerificationException.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/VerificationException.java @@ -5,31 +5,20 @@ */ package org.elasticsearch.xpack.sql.analysis.analyzer; -import org.elasticsearch.xpack.ql.tree.Location; -import org.elasticsearch.xpack.ql.util.StringUtils; -import org.elasticsearch.xpack.sql.analysis.AnalysisException; -import org.elasticsearch.xpack.sql.analysis.analyzer.Verifier.Failure; +import org.elasticsearch.rest.RestStatus; +import org.elasticsearch.xpack.ql.common.Failure; +import org.elasticsearch.xpack.sql.SqlClientException; import java.util.Collection; -import java.util.stream.Collectors; - -public class VerificationException extends AnalysisException { - - private final Collection failures; +public class VerificationException extends SqlClientException { protected VerificationException(Collection sources) { - super(null, StringUtils.EMPTY); - failures = sources; + super(Failure.failMessage(sources)); } @Override - public String getMessage() { - return failures.stream() - .map(f -> { - Location l = f.node().source().source(); - return "line " + l.getLineNumber() + ":" + l.getColumnNumber() + ": " + f.message(); - }) - .collect(Collectors.joining(StringUtils.NEW_LINE, "Found " + failures.size() + " problem(s)\n", StringUtils.EMPTY)); + public RestStatus status() { + return RestStatus.BAD_REQUEST; } } diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Verifier.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Verifier.java index 6c1df11f070..1dc1d84ba6d 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Verifier.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Verifier.java @@ -6,6 +6,7 @@ package org.elasticsearch.xpack.sql.analysis.analyzer; import org.elasticsearch.xpack.ql.capabilities.Unresolvable; +import org.elasticsearch.xpack.ql.common.Failure; import org.elasticsearch.xpack.ql.expression.Alias; import org.elasticsearch.xpack.ql.expression.Attribute; import org.elasticsearch.xpack.ql.expression.AttributeMap; @@ -61,12 +62,11 @@ import java.util.LinkedHashSet; import java.util.List; import java.util.Locale; import java.util.Map; -import java.util.Objects; import java.util.Set; import java.util.function.Consumer; import static java.util.stream.Collectors.toMap; -import static org.elasticsearch.common.logging.LoggerMessageFormat.format; +import static org.elasticsearch.xpack.ql.common.Failure.fail; import static org.elasticsearch.xpack.ql.util.CollectionUtils.combine; import static org.elasticsearch.xpack.sql.stats.FeatureMetric.COMMAND; import static org.elasticsearch.xpack.sql.stats.FeatureMetric.GROUPBY; @@ -89,52 +89,6 @@ public final class Verifier { this.metrics = metrics; } - static class Failure { - private final Node node; - private final String message; - - Failure(Node node, String message) { - this.node = node; - this.message = message; - } - - Node node() { - return node; - } - - String message() { - return message; - } - - @Override - public int hashCode() { - return Objects.hash(node); - } - - @Override - public boolean equals(Object obj) { - if (this == obj) { - return true; - } - - if (obj == null || getClass() != obj.getClass()) { - return false; - } - - Verifier.Failure other = (Verifier.Failure) obj; - return Objects.equals(node, other.node); - } - - @Override - public String toString() { - return message; - } - } - - private static Failure fail(Node source, String message, Object... args) { - return new Failure(source, format(message, args)); - } - public Map, String> verifyFailures(LogicalPlan plan) { Collection failures = verify(plan); return failures.stream().collect(toMap(Failure::node, Failure::message)); diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/planner/Planner.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/planner/Planner.java index ac293fd3384..e9ea5ae1bf0 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/planner/Planner.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/planner/Planner.java @@ -5,10 +5,10 @@ */ package org.elasticsearch.xpack.sql.planner; +import org.elasticsearch.xpack.ql.common.Failure; import org.elasticsearch.xpack.ql.plan.logical.LogicalPlan; import org.elasticsearch.xpack.ql.tree.Node; import org.elasticsearch.xpack.sql.plan.physical.PhysicalPlan; -import org.elasticsearch.xpack.sql.planner.Verifier.Failure; import java.util.List; import java.util.Map; @@ -49,7 +49,7 @@ public class Planner { public Map, String> verifyMappingPlanFailures(PhysicalPlan plan) { List failures = Verifier.verifyMappingPlan(plan); - return failures.stream().collect(toMap(Failure::source, Failure::message)); + return failures.stream().collect(toMap(Failure::node, Failure::message)); } public PhysicalPlan verifyExecutingPlan(PhysicalPlan plan) { @@ -62,6 +62,6 @@ public class Planner { public Map, String> verifyExecutingPlanFailures(PhysicalPlan plan) { List failures = Verifier.verifyExecutingPlan(plan); - return failures.stream().collect(toMap(Failure::source, Failure::message)); + return failures.stream().collect(toMap(Failure::node, Failure::message)); } } diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/planner/PlanningException.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/planner/PlanningException.java index 8a013fee2f9..3552057b9a2 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/planner/PlanningException.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/planner/PlanningException.java @@ -6,12 +6,10 @@ package org.elasticsearch.xpack.sql.planner; import org.elasticsearch.rest.RestStatus; -import org.elasticsearch.xpack.ql.tree.Location; +import org.elasticsearch.xpack.ql.common.Failure; import org.elasticsearch.xpack.sql.SqlClientException; -import org.elasticsearch.xpack.sql.planner.Verifier.Failure; import java.util.Collection; -import java.util.stream.Collectors; public class PlanningException extends SqlClientException { public PlanningException(String message, Object... args) { @@ -19,20 +17,11 @@ public class PlanningException extends SqlClientException { } public PlanningException(Collection sources) { - super(extractMessage(sources)); + super(Failure.failMessage(sources)); } @Override public RestStatus status() { return RestStatus.BAD_REQUEST; } - - private static String extractMessage(Collection failures) { - return failures.stream() - .map(f -> { - Location l = f.source().source().source(); - return "line " + l.getLineNumber() + ":" + l.getColumnNumber() + ": " + f.message(); - }) - .collect(Collectors.joining("\n", "Found " + failures.size() + " problem(s)\n", "")); - } } diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/planner/Verifier.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/planner/Verifier.java index 6b9683d439d..fcd2d03b8c8 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/planner/Verifier.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/planner/Verifier.java @@ -5,8 +5,8 @@ */ package org.elasticsearch.xpack.sql.planner; +import org.elasticsearch.xpack.ql.common.Failure; import org.elasticsearch.xpack.ql.expression.function.aggregate.InnerAggregate; -import org.elasticsearch.xpack.ql.tree.Node; import org.elasticsearch.xpack.sql.plan.physical.PhysicalPlan; import org.elasticsearch.xpack.sql.plan.physical.PivotExec; import org.elasticsearch.xpack.sql.plan.physical.Unexecutable; @@ -14,53 +14,11 @@ import org.elasticsearch.xpack.sql.plan.physical.UnplannedExec; import java.util.ArrayList; import java.util.List; -import java.util.Objects; -import static org.elasticsearch.common.logging.LoggerMessageFormat.format; +import static org.elasticsearch.xpack.ql.common.Failure.fail; abstract class Verifier { - static class Failure { - private final Node source; - private final String message; - - Failure(Node source, String message) { - this.source = source; - this.message = message; - } - - Node source() { - return source; - } - - String message() { - return message; - } - - @Override - public int hashCode() { - return source.hashCode(); - } - - @Override - public boolean equals(Object obj) { - if (this == obj) { - return true; - } - - if (obj == null || getClass() != obj.getClass()) { - return false; - } - - Verifier.Failure other = (Verifier.Failure) obj; - return Objects.equals(source, other.source); - } - } - - private static Failure fail(Node source, String message, Object... args) { - return new Failure(source, format(null, message, args)); - } - static List verifyMappingPlan(PhysicalPlan plan) { List failures = new ArrayList<>(); diff --git a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/analysis/analyzer/FieldAttributeTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/analysis/analyzer/FieldAttributeTests.java index 14b1da83825..9c5023ff60c 100644 --- a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/analysis/analyzer/FieldAttributeTests.java +++ b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/analysis/analyzer/FieldAttributeTests.java @@ -144,17 +144,17 @@ public class FieldAttributeTests extends ESTestCase { } public void testDottedFieldPath() { - assertThat(error("some"), is("Found 1 problem(s)\nline 1:8: Cannot use field [some] type [object] only its subfields")); + assertThat(error("some"), is("Found 1 problem\nline 1:8: Cannot use field [some] type [object] only its subfields")); } public void testDottedFieldPathDeeper() { assertThat(error("some.dotted"), - is("Found 1 problem(s)\nline 1:8: Cannot use field [some.dotted] type [object] only its subfields")); + is("Found 1 problem\nline 1:8: Cannot use field [some.dotted] type [object] only its subfields")); } public void testDottedFieldPathTypo() { assertThat(error("some.dotted.fild"), - is("Found 1 problem(s)\nline 1:8: Unknown column [some.dotted.fild], did you mean [some.dotted.field]?")); + is("Found 1 problem\nline 1:8: Unknown column [some.dotted.fild], did you mean [some.dotted.field]?")); } public void testStarExpansionExcludesObjectAndUnsupportedTypes() { @@ -177,13 +177,13 @@ public class FieldAttributeTests extends ESTestCase { VerificationException ex = expectThrows(VerificationException.class, () -> plan("SELECT test.bar FROM test")); assertEquals( - "Found 1 problem(s)\nline 1:8: Reference [test.bar] is ambiguous (to disambiguate use quotes or qualifiers); " + "Found 1 problem\nline 1:8: Reference [test.bar] is ambiguous (to disambiguate use quotes or qualifiers); " + "matches any of [\"test\".\"bar\", \"test\".\"test.bar\"]", ex.getMessage()); ex = expectThrows(VerificationException.class, () -> plan("SELECT test.test FROM test")); assertEquals( - "Found 1 problem(s)\nline 1:8: Reference [test.test] is ambiguous (to disambiguate use quotes or qualifiers); " + "Found 1 problem\nline 1:8: Reference [test.test] is ambiguous (to disambiguate use quotes or qualifiers); " + "matches any of [\"test\".\"test\", \"test\".\"test.test\"]", ex.getMessage()); diff --git a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/analysis/analyzer/VerifierErrorMessagesTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/analysis/analyzer/VerifierErrorMessagesTests.java index a3d55a19ec1..5f49d3a9935 100644 --- a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/analysis/analyzer/VerifierErrorMessagesTests.java +++ b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/analysis/analyzer/VerifierErrorMessagesTests.java @@ -50,9 +50,11 @@ public class VerifierErrorMessagesTests extends ESTestCase { private String error(IndexResolution getIndexResult, String sql) { Analyzer analyzer = new Analyzer(SqlTestUtils.TEST_CFG, new SqlFunctionRegistry(), getIndexResult, new Verifier(new Metrics())); VerificationException e = expectThrows(VerificationException.class, () -> analyzer.analyze(parser.createStatement(sql), true)); - assertTrue(e.getMessage().startsWith("Found ")); - String header = "Found 1 problem(s)\nline "; - return e.getMessage().substring(header.length()); + String message = e.getMessage(); + assertTrue(message.startsWith("Found ")); + String pattern = "\nline "; + int index = message.indexOf(pattern); + return message.substring(index + pattern.length()); } private LogicalPlan accept(String sql) { diff --git a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/execution/search/QuerierTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/execution/search/QuerierTests.java index 2269aca7d8a..64153a92f12 100644 --- a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/execution/search/QuerierTests.java +++ b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/execution/search/QuerierTests.java @@ -87,7 +87,7 @@ public class QuerierTests extends ESTestCase { @SuppressWarnings({"rawtypes", "unchecked"}) public void testAggSorting_FourFields() { - List comparators = Arrays.asList( + List comparators = Arrays. asList( Comparator.naturalOrder(), Comparator.naturalOrder(), Comparator.reverseOrder(), diff --git a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/PostOptimizerVerifierTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/PostOptimizerVerifierTests.java index dae2efb865b..0f1072f462f 100644 --- a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/PostOptimizerVerifierTests.java +++ b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/PostOptimizerVerifierTests.java @@ -61,7 +61,7 @@ public class PostOptimizerVerifierTests extends ESTestCase { private String error(IndexResolution getIndexResult, String sql) { PlanningException e = expectThrows(PlanningException.class, () -> plan(sql)); assertTrue(e.getMessage().startsWith("Found ")); - String header = "Found 1 problem(s)\nline "; + String header = "Found 1 problem\nline "; return e.getMessage().substring(header.length()); }