diff --git a/sql/server/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Analyzer.java b/sql/server/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Analyzer.java index b57b3665c25..42c9b75306e 100644 --- a/sql/server/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Analyzer.java +++ b/sql/server/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Analyzer.java @@ -650,6 +650,11 @@ public class Analyzer extends RuleExecutor { return plan.transformExpressionsUp(e -> { if (e instanceof UnresolvedFunction) { UnresolvedFunction uf = (UnresolvedFunction) e; + + if (uf.analyzed()) { + return uf; + } + String name = uf.name(); if (hasStar(uf.arguments())) { @@ -690,12 +695,8 @@ public class Analyzer extends RuleExecutor { } List matches = StringUtils.findSimilar(normalizedName, names); - if (!matches.isEmpty()) { - return new UnresolvedFunction(uf.location(), uf.name(), uf.distinct(), uf.children(), UnresolvedFunction.errorMessage(normalizedName, matches)); - } - else { - return uf; - } + String message = matches.isEmpty() ? uf.unresolvedMessage() : UnresolvedFunction.errorMessage(normalizedName, matches); + return new UnresolvedFunction(uf.location(), uf.name(), uf.distinct(), uf.children(), true, message); } // TODO: look into Generator for significant terms, etc.. Function f = functionRegistry.resolveFunction(uf, SqlSession.CURRENT_SETTINGS.get()); diff --git a/sql/server/src/main/java/org/elasticsearch/xpack/sql/expression/function/UnresolvedFunction.java b/sql/server/src/main/java/org/elasticsearch/xpack/sql/expression/function/UnresolvedFunction.java index 433d5a87564..f717aece370 100644 --- a/sql/server/src/main/java/org/elasticsearch/xpack/sql/expression/function/UnresolvedFunction.java +++ b/sql/server/src/main/java/org/elasticsearch/xpack/sql/expression/function/UnresolvedFunction.java @@ -20,18 +20,22 @@ public class UnresolvedFunction extends Function implements Unresolvable { private final String name; private final boolean distinct; private final String unresolvedMsg; + // flag to indicate analysis has been applied and there's no point in doing it again + // this is an optimization to prevent searching for a better unresolved message over and over again + private final boolean analyzed; public UnresolvedFunction(Location location, String name, boolean distinct, List children) { - this(location, name, distinct, children, null); + this(location, name, distinct, children, false, null); } /** * Constructor used for specifying a more descriptive message (typically 'did you mean') instead of the default one. */ - public UnresolvedFunction(Location location, String name, boolean distinct, List children, String unresolvedMessage) { + public UnresolvedFunction(Location location, String name, boolean distinct, List children, boolean analyzed, String unresolvedMessage) { super(location, children); this.name = name; this.distinct = distinct; + this.analyzed = analyzed; this.unresolvedMsg = unresolvedMessage == null ? errorMessage(name, null) : unresolvedMessage; } @@ -54,6 +58,10 @@ public class UnresolvedFunction extends Function implements Unresolvable { return distinct; } + public boolean analyzed() { + return analyzed; + } + @Override public DataType dataType() { throw new UnresolvedException("dataType", this); @@ -82,7 +90,8 @@ public class UnresolvedFunction extends Function implements Unresolvable { public static String errorMessage(String name, List potentialMatches) { String msg = "Unknown function [" + name + "]"; if (!CollectionUtils.isEmpty(potentialMatches)) { - msg += ", did you mean " + (potentialMatches.size() == 1 ? "[" + potentialMatches.get(0) + "]": "any of " + potentialMatches.toString()) + "?"; + msg += ", did you mean " + + (potentialMatches.size() == 1 ? "[" + potentialMatches.get(0) + "]" : "any of " + potentialMatches.toString()) + "?"; } return msg; } diff --git a/sql/server/src/main/java/org/elasticsearch/xpack/sql/rule/RuleExecutionException.java b/sql/server/src/main/java/org/elasticsearch/xpack/sql/rule/RuleExecutionException.java new file mode 100644 index 00000000000..ab5bf8ec4ce --- /dev/null +++ b/sql/server/src/main/java/org/elasticsearch/xpack/sql/rule/RuleExecutionException.java @@ -0,0 +1,15 @@ +/* + * 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.rule; + +import org.elasticsearch.xpack.sql.ServerSqlException; + +public class RuleExecutionException extends ServerSqlException { + + public RuleExecutionException(String message, Object... args) { + super(message, args); + } +} diff --git a/sql/server/src/main/java/org/elasticsearch/xpack/sql/rule/RuleExecutor.java b/sql/server/src/main/java/org/elasticsearch/xpack/sql/rule/RuleExecutor.java index fb4a38a2148..1643cab3a84 100644 --- a/sql/server/src/main/java/org/elasticsearch/xpack/sql/rule/RuleExecutor.java +++ b/sql/server/src/main/java/org/elasticsearch/xpack/sql/rule/RuleExecutor.java @@ -5,38 +5,42 @@ */ package org.elasticsearch.xpack.sql.rule; -import java.util.ArrayList; -import java.util.LinkedHashMap; -import java.util.List; -import java.util.Map; -import java.util.concurrent.TimeUnit; - import org.apache.logging.log4j.Logger; import org.elasticsearch.common.logging.Loggers; import org.elasticsearch.common.unit.TimeValue; import org.elasticsearch.xpack.sql.tree.Node; import org.elasticsearch.xpack.sql.tree.NodeUtils; +import java.util.ArrayList; +import java.util.LinkedHashMap; +import java.util.List; +import java.util.Map; + public abstract class RuleExecutor> { private final Logger log = Loggers.getLogger(getClass()); public static class Limiter { - public static final Limiter DEFAULT = new Limiter(100, 5, TimeUnit.SECONDS); - public static final Limiter ONCE = new Limiter(1, 5, TimeUnit.SECONDS); + public static final Limiter DEFAULT = new Limiter(100); + public static final Limiter ONCE = new Limiter(1) { + + @Override + boolean reached(int runs) { + return runs >= 1; + } + }; private final int runs; - private final long duration; - public Limiter(int runs, long duration, TimeUnit timeUnit) { - this.runs = runs; - this.duration = timeUnit.toNanos(duration); + public Limiter(int maximumRuns) { + this.runs = maximumRuns; } - private boolean reached(int runs, long duration) { - // NOCOMMIT we should probably be throwing exceptions rather than stopping - // NOCOMMIT including time here seems like it'd cause very unpredictable behavior - return runs >= this.runs || duration >= this.duration; + boolean reached(int runs) { + if (runs >= this.runs) { + throw new RuleExecutionException("Rule execution limit %d reached", runs); + } + return false; } } @@ -160,12 +164,12 @@ public abstract class RuleExecutor> { } else { if (log.isTraceEnabled()) { - log.trace("Rule {} applied w/o changes\n", rule); + log.trace("Rule {} applied w/o changes", rule); } } } batchDuration = System.currentTimeMillis() - batchStart; - } while (hasChanged && !batch.limit.reached(batchRuns, batchDuration)); + } while (hasChanged && !batch.limit.reached(batchRuns)); totalDuration += batchDuration; diff --git a/sql/server/src/test/java/org/elasticsearch/xpack/sql/analysis/analyzer/VerifierErrorMessagesTests.java b/sql/server/src/test/java/org/elasticsearch/xpack/sql/analysis/analyzer/VerifierErrorMessagesTests.java index bae19209e20..08ca84464fc 100644 --- a/sql/server/src/test/java/org/elasticsearch/xpack/sql/analysis/analyzer/VerifierErrorMessagesTests.java +++ b/sql/server/src/test/java/org/elasticsearch/xpack/sql/analysis/analyzer/VerifierErrorMessagesTests.java @@ -71,7 +71,7 @@ public class VerifierErrorMessagesTests extends ESTestCase { assertEquals("1:8: Unknown function [ZAZ]", verify("SELECT ZAZ(bool) FROM test")); } - public void testMispelledFunction() { + public void testMisspelledFunction() { assertEquals("1:8: Unknown function [COONT], did you mean [COUNT]?", verify("SELECT COONT(bool) FROM test")); }