diff options
4 files changed, 234 insertions, 40 deletions
diff --git a/config-model/src/main/java/com/yahoo/searchdefinition/MapEvaluationTypeContext.java b/config-model/src/main/java/com/yahoo/searchdefinition/MapEvaluationTypeContext.java index 8c7398b3dde..be49353b1e8 100644 --- a/config-model/src/main/java/com/yahoo/searchdefinition/MapEvaluationTypeContext.java +++ b/config-model/src/main/java/com/yahoo/searchdefinition/MapEvaluationTypeContext.java @@ -13,12 +13,16 @@ import com.yahoo.searchlib.rankingexpression.rule.ReferenceNode; import com.yahoo.tensor.TensorType; import com.yahoo.tensor.evaluation.TypeContext; +import java.util.ArrayDeque; import java.util.Collection; import java.util.Collections; +import java.util.Deque; import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.Optional; +import java.util.Stack; +import java.util.stream.Collectors; /** * A context which only contains type information. @@ -26,21 +30,29 @@ import java.util.Optional; * query, attribute or constant features, as we do not have information about which such * features exist (but we know those that exist are doubles). * + * This is not multithread safe. + * * @author bratseth */ public class MapEvaluationTypeContext extends FunctionReferenceContext implements TypeContext<Reference> { private final Map<Reference, TensorType> featureTypes = new HashMap<>(); - public MapEvaluationTypeContext(Collection<ExpressionFunction> functions) { + /** For invocation loop detection */ + private final Deque<Reference> currentResolutionCallStack; + + MapEvaluationTypeContext(Collection<ExpressionFunction> functions) { super(functions); + this.currentResolutionCallStack = new ArrayDeque<>(); } - public MapEvaluationTypeContext(Map<String, ExpressionFunction> functions, - Map<String, String> bindings, - Map<Reference, TensorType> featureTypes) { + private MapEvaluationTypeContext(Map<String, ExpressionFunction> functions, + Map<String, String> bindings, + Map<Reference, TensorType> featureTypes, + Deque<Reference> currentResolutionCallStack) { super(functions, bindings); this.featureTypes.putAll(featureTypes); + this.currentResolutionCallStack = currentResolutionCallStack; } public void setType(Reference reference, TensorType type) { @@ -55,42 +67,52 @@ public class MapEvaluationTypeContext extends FunctionReferenceContext implement @Override public TensorType getType(Reference reference) { // A reference to a macro argument? - Optional<String> binding = boundIdentifier(reference); - if (binding.isPresent()) { - try { - // This is not pretty, but changing to bind expressions rather - // than their string values requires deeper changes - return new RankingExpression(binding.get()).type(this); + if (currentResolutionCallStack.contains(reference)) + throw new IllegalArgumentException("Invocation loop: " + + currentResolutionCallStack.stream().map(Reference::toString).collect(Collectors.joining(" -> ")) + + " -> " + reference); + currentResolutionCallStack.addLast(reference); + + try { + Optional<String> binding = boundIdentifier(reference); + if (binding.isPresent()) { + try { + // This is not pretty, but changing to bind expressions rather + // than their string values requires deeper changes + return new RankingExpression(binding.get()).type(this); + } catch (ParseException e) { + throw new IllegalArgumentException(e); + } } - catch (ParseException e) { - throw new IllegalArgumentException(e); + + // A reference to an attribute, query or constant feature? + if (FeatureNames.isSimpleFeature(reference)) { + // The argument may be a local identifier bound to the actual value + String argument = reference.simpleArgument().get(); + reference = Reference.simple(reference.name(), bindings.getOrDefault(argument, argument)); + return featureTypes.getOrDefault(reference, defaultTypeOf(reference)); } - } - // A reference to an attribute, query or constant feature? - if (FeatureNames.isSimpleFeature(reference)) { - // The argument may be a local identifier bound to the actual value - String argument = reference.simpleArgument().get(); - reference = Reference.simple(reference.name(), bindings.getOrDefault(argument, argument)); - return featureTypes.getOrDefault(reference, defaultTypeOf(reference)); - } + // A reference to a function? + Optional<ExpressionFunction> function = functionInvocation(reference); + if (function.isPresent()) { + return function.get().getBody().type(this.withBindings(bind(function.get().arguments(), reference.arguments()))); + } - // A reference to a function? - Optional<ExpressionFunction> function = functionInvocation(reference); - if (function.isPresent()) { - return function.get().getBody().type(this.withBindings(bind(function.get().arguments(), reference.arguments()))); - } + // A reference to a feature which returns a tensor? + Optional<TensorType> featureTensorType = tensorFeatureType(reference); + if (featureTensorType.isPresent()) { + return featureTensorType.get(); + } - // A reference to a feature which returns a tensor? - Optional<TensorType> featureTensorType = tensorFeatureType(reference); - if (featureTensorType.isPresent()) { - return featureTensorType.get(); + // We do not know what this is - since we do not have complete knowledge abut the match features + // in Java we must assume this is a match feature and return the double type - which is the type of all + // all match features + return TensorType.empty; + } + finally { + currentResolutionCallStack.removeLast(); } - - // We do not know what this is - since we do not have complete knowledge abut the match features - // in Java we must assume this is a match feature and return the double type - which is the type of all - // all match features - return TensorType.empty; } /** @@ -173,7 +195,7 @@ public class MapEvaluationTypeContext extends FunctionReferenceContext implement @Override public MapEvaluationTypeContext withBindings(Map<String, String> bindings) { if (bindings.isEmpty() && this.bindings.isEmpty()) return this; - return new MapEvaluationTypeContext(functions(), bindings, featureTypes); + return new MapEvaluationTypeContext(functions(), bindings, featureTypes, currentResolutionCallStack); } } diff --git a/config-model/src/test/java/com/yahoo/searchdefinition/FeatureNamesTestCase.java b/config-model/src/test/java/com/yahoo/searchdefinition/FeatureNamesTestCase.java index aa01070d296..056fc27f067 100644 --- a/config-model/src/test/java/com/yahoo/searchdefinition/FeatureNamesTestCase.java +++ b/config-model/src/test/java/com/yahoo/searchdefinition/FeatureNamesTestCase.java @@ -1,8 +1,4 @@ -/* - * // Copyright 2018 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. - * - * - */ +// Copyright 2018 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. package com.yahoo.searchdefinition; import org.junit.Test; diff --git a/config-model/src/test/java/com/yahoo/searchdefinition/RankingExpressionInliningTestCase.java b/config-model/src/test/java/com/yahoo/searchdefinition/RankingExpressionInliningTestCase.java index e1ddd0c02ca..b13ffabda77 100644 --- a/config-model/src/test/java/com/yahoo/searchdefinition/RankingExpressionInliningTestCase.java +++ b/config-model/src/test/java/com/yahoo/searchdefinition/RankingExpressionInliningTestCase.java @@ -7,12 +7,14 @@ import com.yahoo.searchdefinition.derived.AttributeFields; import com.yahoo.searchdefinition.derived.RawRankProfile; import com.yahoo.searchdefinition.parser.ParseException; import com.yahoo.searchlib.rankingexpression.integration.ml.ImportedModels; +import com.yahoo.yolean.Exceptions; import org.junit.Test; import java.util.Optional; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; /** * @author bratseth diff --git a/config-model/src/test/java/com/yahoo/searchdefinition/RankingExpressionLoopDetectionTestCase.java b/config-model/src/test/java/com/yahoo/searchdefinition/RankingExpressionLoopDetectionTestCase.java new file mode 100644 index 00000000000..725f361bd28 --- /dev/null +++ b/config-model/src/test/java/com/yahoo/searchdefinition/RankingExpressionLoopDetectionTestCase.java @@ -0,0 +1,174 @@ +// Copyright 2018 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. +package com.yahoo.searchdefinition; + +import com.yahoo.searchdefinition.parser.ParseException; +import com.yahoo.yolean.Exceptions; +import org.junit.Test; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.fail; + +/** + * @author bratseth + */ +public class RankingExpressionLoopDetectionTestCase { + + @Test + public void testSelfLoop() throws ParseException { + RankProfileRegistry rankProfileRegistry = new RankProfileRegistry(); + SearchBuilder builder = new SearchBuilder(rankProfileRegistry); + builder.importString( + "search test {\n" + + " document test { \n" + + " field a type string { \n" + + " indexing: index \n" + + " }\n" + + " }\n" + + " \n" + + " rank-profile test {\n" + + " first-phase {\n" + + " expression: foo\n" + + " }\n" + + " macro foo() {\n" + + " expression: foo\n" + + " }\n" + + " }\n" + + "\n" + + "}\n"); + try { + builder.build(); + fail("Excepted exception"); + } + catch (IllegalArgumentException e) { + assertEquals("In search definition 'test', rank profile 'test': The first-phase expression is invalid: Invocation loop: foo -> foo", + Exceptions.toMessageString(e)); + } + } + + @Test + public void testNestedLoop() throws ParseException { + RankProfileRegistry rankProfileRegistry = new RankProfileRegistry(); + SearchBuilder builder = new SearchBuilder(rankProfileRegistry); + builder.importString( + "search test {\n" + + " document test { \n" + + " field a type string { \n" + + " indexing: index \n" + + " }\n" + + " }\n" + + " \n" + + " rank-profile test {\n" + + " first-phase {\n" + + " expression: foo\n" + + " }\n" + + " macro foo() {\n" + + " expression: arg(5)\n" + + " }\n" + + " macro arg(a1) {\n" + + " expression: foo + a1*2\n" + + " }\n" + + " }\n" + + "\n" + + "}\n"); + try { + builder.build(); + fail("Excepted exception"); + } + catch (IllegalArgumentException e) { + assertEquals("In search definition 'test', rank profile 'test': The first-phase expression is invalid: Invocation loop: foo -> arg(5) -> foo", + Exceptions.toMessageString(e)); + } + } + + @Test + public void testSelfArgumentLoop() throws ParseException { + RankProfileRegistry rankProfileRegistry = new RankProfileRegistry(); + SearchBuilder builder = new SearchBuilder(rankProfileRegistry); + builder.importString( + "search test {\n" + + " document test { \n" + + " field a type string { \n" + + " indexing: index \n" + + " }\n" + + " }\n" + + " \n" + + " rank-profile test {\n" + + " first-phase {\n" + + " expression: foo\n" + + " }\n" + + " macro foo() {\n" + + " expression: arg(foo)\n" + + " }\n" + + " macro arg(a1) {\n" + + " expression: a1*2\n" + + " }\n" + + " }\n" + + "\n" + + "}\n"); + try { + builder.build(); + fail("Excepted exception"); + } + catch (IllegalArgumentException e) { + assertEquals("In search definition 'test', rank profile 'test': The first-phase expression is invalid: Invocation loop: foo -> arg(foo) -> a1 -> foo", + Exceptions.toMessageString(e)); + } + } + + @Test + public void testNoLoopWithSameLocalArgument() throws ParseException { + RankProfileRegistry rankProfileRegistry = new RankProfileRegistry(); + SearchBuilder builder = new SearchBuilder(rankProfileRegistry); + builder.importString( + "search test {\n" + + " document test { \n" + + " field a type string { \n" + + " indexing: index \n" + + " }\n" + + " }\n" + + " \n" + + " rank-profile test {\n" + + " first-phase {\n" + + " expression: foo(3)\n" + + " }\n" + + " macro foo(a1) {\n" + + " expression: bar(3)\n" + + " }\n" + + " macro bar(a1) {\n" + + " expression: a1*2\n" + + " }\n" + + " }\n" + + "\n" + + "}\n"); + builder.build(); + } + + @Test + public void testNoLoopWithMultipleInvocations() throws ParseException { + RankProfileRegistry rankProfileRegistry = new RankProfileRegistry(); + SearchBuilder builder = new SearchBuilder(rankProfileRegistry); + builder.importString( + "search test {\n" + + " document test { \n" + + " field a type string { \n" + + " indexing: index \n" + + " }\n" + + " }\n" + + " \n" + + " rank-profile test {\n" + + " first-phase {\n" + + " expression: foo(3)\n" + + " }\n" + + " macro foo(a1) {\n" + + " expression: bar(3) + bar(a1)\n" + + " }\n" + + " macro bar(a1) {\n" + + " expression: a1*2\n" + + " }\n" + + " }\n" + + "\n" + + "}\n"); + builder.build(); + } + +} |