From e0b8893645d88296bccb5d6bf7604755d6998cee Mon Sep 17 00:00:00 2001 From: Costin Leau Date: Fri, 27 Apr 2018 09:24:25 +0300 Subject: [PATCH] SQL: Correct error message (#30138) * SQL: Correct error message Error messages had placeholders that were not replaced; this PR fixes that Fix #30016 --- .../xpack/sql/planner/QueryTranslator.java | 20 +++---- .../analyzer/FieldAttributeTests.java | 2 +- .../logical/command/sys/SysColumnsTests.java | 6 +- .../sql/planner/QueryTranslatorTests.java | 55 +++++++++++++++++++ .../mapping-multi-field-variation.json | 1 + 5 files changed, 70 insertions(+), 14 deletions(-) diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/planner/QueryTranslator.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/planner/QueryTranslator.java index 2c90921e28599..dd0456e9aefc8 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/planner/QueryTranslator.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/planner/QueryTranslator.java @@ -10,6 +10,7 @@ import org.elasticsearch.xpack.sql.expression.BinaryExpression; import org.elasticsearch.xpack.sql.expression.Expression; import org.elasticsearch.xpack.sql.expression.ExpressionId; +import org.elasticsearch.xpack.sql.expression.Expressions; import org.elasticsearch.xpack.sql.expression.FieldAttribute; import org.elasticsearch.xpack.sql.expression.Literal; import org.elasticsearch.xpack.sql.expression.NamedExpression; @@ -159,7 +160,7 @@ static QueryTranslation toQuery(Expression e, boolean onAggs) { } } - throw new UnsupportedOperationException(format(Locale.ROOT, "Don't know how to translate %s %s", e.nodeName(), e)); + throw new SqlIllegalArgumentException("Don't know how to translate {} {}", e.nodeName(), e); } static LeafAgg toAgg(String id, Function f) { @@ -171,7 +172,7 @@ static LeafAgg toAgg(String id, Function f) { } } - throw new UnsupportedOperationException(format(Locale.ROOT, "Don't know how to translate %s %s", f.nodeName(), f)); + throw new SqlIllegalArgumentException("Don't know how to translate {} {}", f.nodeName(), f); } static class GroupingContext { @@ -395,8 +396,8 @@ static String field(AggregateFunction af) { if (arg instanceof Literal) { return String.valueOf(((Literal) arg).value()); } - throw new SqlIllegalArgumentException("Does not know how to convert argument " + arg.nodeString() - + " for function " + af.nodeString()); + throw new SqlIllegalArgumentException("Does not know how to convert argument {} for function {}", arg.nodeString(), + af.nodeString()); } // TODO: need to optimize on ngram @@ -505,9 +506,9 @@ static class BinaryComparisons extends ExpressionTranslator { @Override protected QueryTranslation asQuery(BinaryComparison bc, boolean onAggs) { Check.isTrue(bc.right().foldable(), - "Line %d:%d - Comparisons against variables are not (currently) supported; offender %s in %s", + "Line {}:{}: Comparisons against variables are not (currently) supported; offender [{}] in [{}]", bc.right().location().getLineNumber(), bc.right().location().getColumnNumber(), - bc.right().nodeName(), bc.nodeName()); + Expressions.name(bc.right()), bc.symbol()); if (bc.left() instanceof NamedExpression) { NamedExpression ne = (NamedExpression) bc.left(); @@ -605,8 +606,8 @@ private static Query translateQuery(BinaryComparison bc) { return new TermQuery(loc, name, value); } - Check.isTrue(false, "don't know how to translate binary comparison [{}] in [{}]", bc.right().nodeString(), bc); - return null; + throw new SqlIllegalArgumentException("Don't know how to translate binary comparison [{}] in [{}]", bc.right().nodeString(), + bc); } } @@ -700,9 +701,8 @@ else if (onAggs) { return new QueryTranslation(query, aggFilter); } else { - throw new UnsupportedOperationException("No idea how to translate " + e); + throw new SqlIllegalArgumentException("No idea how to translate " + e); } - } } 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 06b96552e5e98..9d05d151359fd 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 @@ -153,7 +153,7 @@ public void testDottedFieldPathTypo() { public void testStarExpansionExcludesObjectAndUnsupportedTypes() { LogicalPlan plan = plan("SELECT * FROM test"); List list = ((Project) plan).projections(); - assertThat(list, hasSize(7)); + assertThat(list, hasSize(8)); List names = Expressions.names(list); assertThat(names, not(hasItem("some"))); assertThat(names, not(hasItem("some.dotted"))); diff --git a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/plan/logical/command/sys/SysColumnsTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/plan/logical/command/sys/SysColumnsTests.java index 2866624b07bb9..bddddc6941cbb 100644 --- a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/plan/logical/command/sys/SysColumnsTests.java +++ b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/plan/logical/command/sys/SysColumnsTests.java @@ -17,7 +17,7 @@ public class SysColumnsTests extends ESTestCase { public void testSysColumns() { List> rows = new ArrayList<>(); SysColumns.fillInRows("test", "index", TypesTests.loadMapping("mapping-multi-field-variation.json", true), null, rows, null); - assertEquals(15, rows.size()); + assertEquals(16, rows.size()); assertEquals(24, rows.get(0).size()); List row = rows.get(0); @@ -38,13 +38,13 @@ public void testSysColumns() { assertEquals(null, radix(row)); assertEquals(Integer.MAX_VALUE, bufferLength(row)); - row = rows.get(6); + row = rows.get(7); assertEquals("some.dotted", name(row)); assertEquals(Types.STRUCT, sqlType(row)); assertEquals(null, radix(row)); assertEquals(-1, bufferLength(row)); - row = rows.get(14); + row = rows.get(15); assertEquals("some.ambiguous.normalized", name(row)); assertEquals(Types.VARCHAR, sqlType(row)); assertEquals(null, radix(row)); diff --git a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/QueryTranslatorTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/QueryTranslatorTests.java index f654d0c701178..2a3d87b65c964 100644 --- a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/QueryTranslatorTests.java +++ b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/QueryTranslatorTests.java @@ -6,6 +6,7 @@ package org.elasticsearch.xpack.sql.planner; import org.elasticsearch.test.ESTestCase; +import org.elasticsearch.xpack.sql.SqlIllegalArgumentException; import org.elasticsearch.xpack.sql.analysis.analyzer.Analyzer; import org.elasticsearch.xpack.sql.analysis.index.EsIndex; import org.elasticsearch.xpack.sql.analysis.index.IndexResolution; @@ -18,9 +19,11 @@ import org.elasticsearch.xpack.sql.plan.logical.Project; import org.elasticsearch.xpack.sql.planner.QueryTranslator.QueryTranslation; import org.elasticsearch.xpack.sql.querydsl.query.Query; +import org.elasticsearch.xpack.sql.querydsl.query.RangeQuery; import org.elasticsearch.xpack.sql.querydsl.query.TermQuery; import org.elasticsearch.xpack.sql.type.EsField; import org.elasticsearch.xpack.sql.type.TypesTests; +import org.joda.time.DateTime; import java.util.Map; import java.util.TimeZone; @@ -84,4 +87,56 @@ public void testTermEqualityNotAnalyzed() { assertEquals("int", tq.term()); assertEquals(5, tq.value()); } + + public void testComparisonAgainstColumns() { + LogicalPlan p = plan("SELECT some.string FROM test WHERE date > int"); + assertTrue(p instanceof Project); + p = ((Project) p).child(); + assertTrue(p instanceof Filter); + Expression condition = ((Filter) p).condition(); + SqlIllegalArgumentException ex = expectThrows(SqlIllegalArgumentException.class, () -> QueryTranslator.toQuery(condition, false)); + assertEquals("Line 1:43: Comparisons against variables are not (currently) supported; offender [int] in [>]", ex.getMessage()); + } + + public void testDateRange() { + LogicalPlan p = plan("SELECT some.string FROM test WHERE date > 1969-05-13"); + assertTrue(p instanceof Project); + p = ((Project) p).child(); + assertTrue(p instanceof Filter); + Expression condition = ((Filter) p).condition(); + QueryTranslation translation = QueryTranslator.toQuery(condition, false); + Query query = translation.query; + assertTrue(query instanceof RangeQuery); + RangeQuery rq = (RangeQuery) query; + assertEquals("date", rq.field()); + assertEquals(1951, rq.lower()); + } + + public void testDateRangeLiteral() { + LogicalPlan p = plan("SELECT some.string FROM test WHERE date > '1969-05-13'"); + assertTrue(p instanceof Project); + p = ((Project) p).child(); + assertTrue(p instanceof Filter); + Expression condition = ((Filter) p).condition(); + QueryTranslation translation = QueryTranslator.toQuery(condition, false); + Query query = translation.query; + assertTrue(query instanceof RangeQuery); + RangeQuery rq = (RangeQuery) query; + assertEquals("date", rq.field()); + assertEquals("1969-05-13", rq.lower()); + } + + public void testDateRangeCast() { + LogicalPlan p = plan("SELECT some.string FROM test WHERE date > CAST('1969-05-13T12:34:56Z' AS DATE)"); + assertTrue(p instanceof Project); + p = ((Project) p).child(); + assertTrue(p instanceof Filter); + Expression condition = ((Filter) p).condition(); + QueryTranslation translation = QueryTranslator.toQuery(condition, false); + Query query = translation.query; + assertTrue(query instanceof RangeQuery); + RangeQuery rq = (RangeQuery) query; + assertEquals("date", rq.field()); + assertEquals(DateTime.parse("1969-05-13T12:34:56Z"), rq.lower()); + } } \ No newline at end of file diff --git a/x-pack/plugin/sql/src/test/resources/mapping-multi-field-variation.json b/x-pack/plugin/sql/src/test/resources/mapping-multi-field-variation.json index 4e6ff625a5cc1..13c9f62b2136e 100644 --- a/x-pack/plugin/sql/src/test/resources/mapping-multi-field-variation.json +++ b/x-pack/plugin/sql/src/test/resources/mapping-multi-field-variation.json @@ -4,6 +4,7 @@ "int" : { "type" : "integer" }, "text" : { "type" : "text" }, "keyword" : { "type" : "keyword" }, + "date" : { "type" : "date" }, "unsupported" : { "type" : "ip_range" }, "some" : { "properties" : {