diff options
14 files changed, 220 insertions, 176 deletions
diff --git a/searchlib/src/tests/fef/object_passing/object_passing_test.cpp b/searchlib/src/tests/fef/object_passing/object_passing_test.cpp index ce9585f11cb..793b2d40ed3 100644 --- a/searchlib/src/tests/fef/object_passing/object_passing_test.cpp +++ b/searchlib/src/tests/fef/object_passing/object_passing_test.cpp @@ -59,10 +59,13 @@ struct ProxyBlueprint : Blueprint { } bool setup(const IIndexEnvironment &, const std::vector<vespalib::string> ¶ms) override { ASSERT_EQUAL(1u, params.size()); - object_input = defineInput(params[0], accept_input); - describeOutput("value", "the value", object_output ? FeatureType::object(ValueType::double_type()) : FeatureType::number()); - describeOutput("was_object", "whether input was object", FeatureType::number()); - return true; + if (auto input = defineInput(params[0], accept_input)) { + object_input = input.value().is_object(); + describeOutput("value", "the value", object_output ? FeatureType::object(ValueType::double_type()) : FeatureType::number()); + describeOutput("was_object", "whether input was object", FeatureType::number()); + return true; + } + return false; } FeatureExecutor &createExecutor(const IQueryEnvironment &, vespalib::Stash &stash) const override { return stash.create<ProxyExecutor>(object_input, object_output); diff --git a/searchlib/src/tests/fef/resolver/resolver_test.cpp b/searchlib/src/tests/fef/resolver/resolver_test.cpp index 7591d028620..72076f3999e 100644 --- a/searchlib/src/tests/fef/resolver/resolver_test.cpp +++ b/searchlib/src/tests/fef/resolver/resolver_test.cpp @@ -1,19 +1,15 @@ // Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. -#include <vespa/vespalib/testkit/testapp.h> +#include <vespa/vespalib/testkit/test_kit.h> #include <vespa/searchlib/fef/fef.h> #include <vespa/searchlib/fef/test/indexenvironment.h> #include <vespa/searchlib/features/valuefeature.h> #include <vespa/searchlib/features/rankingexpressionfeature.h> -#include <vespa/log/log.h> -LOG_SETUP("resolver_test"); - +using namespace search; +using namespace search::fef; using search::features::RankingExpressionBlueprint; -namespace search { -namespace fef { - class BaseBlueprint : public Blueprint { public: BaseBlueprint() : Blueprint("base") { } @@ -46,9 +42,9 @@ public: virtual bool setup(const IIndexEnvironment & indexEnv, const ParameterList & params) override { (void) indexEnv; (void) params; - defineInput("base.foo"); - defineInput("base.bar"); - defineInput("base.baz"); + ASSERT_TRUE(defineInput("base.foo")); + ASSERT_TRUE(defineInput("base.bar")); + ASSERT_TRUE(defineInput("base.baz")); describeOutput("out", "out"); return true; } @@ -57,66 +53,37 @@ public: } }; -class Test : public vespalib::TestApp { -private: - BlueprintFactory _factory; - void requireThatWeGetUniqueBlueprints(); - void require_that_bad_input_is_handled(); -public: - Test(); - ~Test(); - int Main() override; +struct Fixture { + BlueprintFactory factory; + Fixture() { + factory.addPrototype(Blueprint::SP(new BaseBlueprint())); + factory.addPrototype(Blueprint::SP(new CombineBlueprint())); + factory.addPrototype(std::make_shared<RankingExpressionBlueprint>()); + } }; -Test::Test() : - _factory() -{ - _factory.addPrototype(Blueprint::SP(new BaseBlueprint())); - _factory.addPrototype(Blueprint::SP(new CombineBlueprint())); - _factory.addPrototype(std::make_shared<RankingExpressionBlueprint>()); -} -Test::~Test() {} - -void -Test::requireThatWeGetUniqueBlueprints() -{ +TEST_F("requireThatWeGetUniqueBlueprints", Fixture()) { test::IndexEnvironment ienv; - BlueprintResolver::SP res(new BlueprintResolver(_factory, ienv)); + BlueprintResolver::SP res(new BlueprintResolver(f.factory, ienv)); res->addSeed("combine"); EXPECT_TRUE(res->compile()); const BlueprintResolver::ExecutorSpecList & spec = res->getExecutorSpecs(); - EXPECT_EQUAL(2u, spec.size()); + ASSERT_EQUAL(2u, spec.size()); EXPECT_TRUE(dynamic_cast<BaseBlueprint *>(spec[0].blueprint.get()) != NULL); EXPECT_TRUE(dynamic_cast<CombineBlueprint *>(spec[1].blueprint.get()) != NULL); } -void -Test::require_that_bad_input_is_handled() -{ +TEST_F("require_that_bad_input_is_handled", Fixture) { test::IndexEnvironment ienv; ienv.getProperties().add(indexproperties::eval::LazyExpressions::NAME, "false"); ienv.getProperties().add("rankingExpression(badinput).rankingScript", "base.foobad + base.bar"); - BlueprintResolver::SP res(new BlueprintResolver(_factory, ienv)); + BlueprintResolver::SP res(new BlueprintResolver(f.factory, ienv)); res->addSeed("rankingExpression(badinput)"); EXPECT_FALSE(res->compile()); const BlueprintResolver::ExecutorSpecList & spec = res->getExecutorSpecs(); - EXPECT_EQUAL(2u, spec.size()); + ASSERT_EQUAL(2u, spec.size()); EXPECT_TRUE(dynamic_cast<BaseBlueprint *>(spec[0].blueprint.get()) != nullptr); EXPECT_TRUE(dynamic_cast<RankingExpressionBlueprint *>(spec[1].blueprint.get()) != nullptr); } -int -Test::Main() -{ - TEST_INIT("resolver_test"); - - requireThatWeGetUniqueBlueprints(); - require_that_bad_input_is_handled(); - - TEST_DONE(); -} - -} -} - -TEST_APPHOOK(search::fef::Test); +TEST_MAIN() { TEST_RUN_ALL(); } diff --git a/searchlib/src/tests/rankingexpression/intrinsic_blueprint_adapter/intrinsic_blueprint_adapter_test.cpp b/searchlib/src/tests/rankingexpression/intrinsic_blueprint_adapter/intrinsic_blueprint_adapter_test.cpp index 861af3527ca..9394cf085fc 100644 --- a/searchlib/src/tests/rankingexpression/intrinsic_blueprint_adapter/intrinsic_blueprint_adapter_test.cpp +++ b/searchlib/src/tests/rankingexpression/intrinsic_blueprint_adapter/intrinsic_blueprint_adapter_test.cpp @@ -39,7 +39,7 @@ struct MyBlueprint : Blueprint { EXPECT_EQUAL(params[0], "foo"); EXPECT_EQUAL(params[1], "bar"); if (is_set(extra_input)) { - defineInput("my_input", AcceptInput::ANY); + EXPECT_TRUE(!defineInput("my_input", AcceptInput::ANY).has_value()); } if (!is_set(no_output)) { if (is_set(error_result)) { diff --git a/searchlib/src/vespa/searchlib/features/constant_feature.cpp b/searchlib/src/vespa/searchlib/features/constant_feature.cpp index 7fc0c5c05fc..5eedb5834bf 100644 --- a/searchlib/src/vespa/searchlib/features/constant_feature.cpp +++ b/searchlib/src/vespa/searchlib/features/constant_feature.cpp @@ -62,9 +62,9 @@ ConstantBlueprint::setup(const IIndexEnvironment &env, _key = params[0].getValue(); _value = env.getConstantValue(_key); if (!_value) { - LOG(error, "Constant '%s' not found", _key.c_str()); + fail("Constant '%s' not found", _key.c_str()); } else if (_value->type().is_error()) { - LOG(error, "Constant '%s' has invalid type", _key.c_str()); + fail("Constant '%s' has invalid type", _key.c_str()); } FeatureType output_type = _value ? FeatureType::object(_value->type()) : diff --git a/searchlib/src/vespa/searchlib/features/firstphasefeature.cpp b/searchlib/src/vespa/searchlib/features/firstphasefeature.cpp index 9d1831c6102..8025b443206 100644 --- a/searchlib/src/vespa/searchlib/features/firstphasefeature.cpp +++ b/searchlib/src/vespa/searchlib/features/firstphasefeature.cpp @@ -42,10 +42,14 @@ bool FirstPhaseBlueprint::setup(const IIndexEnvironment & env, const ParameterList &) { - describeOutput("score", "The ranking score for first phase.", - defineInput(indexproperties::rank::FirstPhase::lookup(env.getProperties()), - AcceptInput::ANY)); - return true; + if (auto maybe_input = defineInput(indexproperties::rank::FirstPhase::lookup(env.getProperties()), + AcceptInput::ANY)) + { + describeOutput("score", "The ranking score for first phase.", maybe_input.value()); + return true; + } else { + return false; + } } FeatureExecutor & diff --git a/searchlib/src/vespa/searchlib/features/rankingexpression/intrinsic_blueprint_adapter.cpp b/searchlib/src/vespa/searchlib/features/rankingexpression/intrinsic_blueprint_adapter.cpp index 4ff9d2f4e30..4b777802985 100644 --- a/searchlib/src/vespa/searchlib/features/rankingexpression/intrinsic_blueprint_adapter.cpp +++ b/searchlib/src/vespa/searchlib/features/rankingexpression/intrinsic_blueprint_adapter.cpp @@ -12,14 +12,14 @@ namespace search::features::rankingexpression { namespace { -bool is_valid(const FeatureType *type) { - if (type == nullptr) { +bool is_valid(const std::optional<FeatureType> &type) { + if (!type.has_value()) { return false; } - if (!type->is_object()) { + if (!type.value().is_object()) { return true; } - return !type->type().is_error(); + return !type.value().type().is_error(); } struct IntrinsicBlueprint : IntrinsicExpression { @@ -38,19 +38,21 @@ struct IntrinsicBlueprint : IntrinsicExpression { }; struct ResultTypeExtractor : Blueprint::DependencyHandler { - std::unique_ptr<FeatureType> result_type; + std::optional<FeatureType> result_type; bool too_much; - ResultTypeExtractor() : result_type(), too_much(false) {} - const FeatureType &resolve_input(const vespalib::string &, Blueprint::AcceptInput) override { + bool failed; + ResultTypeExtractor() : result_type(), too_much(false), failed(false) {} + std::optional<FeatureType> resolve_input(const vespalib::string &, Blueprint::AcceptInput) override { too_much = true; - return FeatureType::number(); + return std::nullopt; } - void define_output(const vespalib::string &, const FeatureType &type) override { - too_much = (too_much || bool(result_type)); - result_type = std::make_unique<FeatureType>(type); + void define_output(const vespalib::string &, FeatureType type) override { + too_much = (too_much || result_type.has_value()); + result_type.emplace(std::move(type)); } - bool valid() const { return (is_valid(result_type.get()) && !too_much); } - const FeatureType &get() const { return *result_type; } + void fail(const vespalib::string &) override { failed = true; } + bool valid() const { return (is_valid(result_type) && !too_much && !failed); } + const FeatureType &get() const { return result_type.value(); } }; } // namespace search::features::rankingexpression::<unnamed> diff --git a/searchlib/src/vespa/searchlib/features/rankingexpressionfeature.cpp b/searchlib/src/vespa/searchlib/features/rankingexpressionfeature.cpp index 62860e04c73..5423e37a755 100644 --- a/searchlib/src/vespa/searchlib/features/rankingexpressionfeature.cpp +++ b/searchlib/src/vespa/searchlib/features/rankingexpressionfeature.cpp @@ -253,13 +253,11 @@ RankingExpressionBlueprint::setup(const fef::IIndexEnvironment &env, script = params[0].getValue(); //LOG(debug, "Script from param: '%s'\n", script.c_str()); } else { - LOG(error, "No expression given."); - return false; + return fail("No expression given."); } auto rank_function = Function::parse(script, rankingexpression::FeatureNameExtractor()); if (rank_function->has_error()) { - LOG(error, "Failed to parse expression '%s': %s", script.c_str(), rank_function->get_error().c_str()); - return false; + return fail("Failed to parse expression '%s': %s", script.c_str(), rank_function->get_error().c_str()); } _intrinsic_expression = _expression_replacer->maybe_replace(*rank_function, env); if (_intrinsic_expression) { @@ -268,25 +266,33 @@ RankingExpressionBlueprint::setup(const fef::IIndexEnvironment &env, return true; } bool do_compile = true; + bool dependency_error = false; std::vector<ValueType> input_types; for (size_t i = 0; i < rank_function->num_params(); ++i) { - const FeatureType &input = defineInput(rank_function->param_name(i), AcceptInput::ANY); - _input_is_object.push_back(char(input.is_object())); - if (input.is_object()) { - do_compile = false; - input_types.push_back(input.type()); + if (auto maybe_input = defineInput(rank_function->param_name(i), AcceptInput::ANY)) { + const FeatureType &input = maybe_input.value(); + _input_is_object.push_back(char(input.is_object())); + if (input.is_object()) { + do_compile = false; + input_types.push_back(input.type()); + } else { + input_types.push_back(ValueType::double_type()); + } } else { - input_types.push_back(ValueType::double_type()); + dependency_error = true; + input_types.push_back(ValueType::error_type()); } } + if (dependency_error) { + return false; + } NodeTypes node_types(*rank_function, input_types); if (!node_types.all_types_are_double()) { do_compile = false; } ValueType root_type = node_types.get_type(rank_function->root()); if (root_type.is_error()) { - LOG(error, "rank expression contains type errors: %s\n", script.c_str()); - return false; + return fail("rank expression contains type errors: %s\n", script.c_str()); } auto compile_issues = CompiledFunction::detect_issues(*rank_function); auto interpret_issues = InterpretedFunction::detect_issues(*rank_function); @@ -297,9 +303,8 @@ RankingExpressionBlueprint::setup(const fef::IIndexEnvironment &env, } const auto &issues = do_compile ? compile_issues : interpret_issues; if (issues) { - LOG(error, "rank expression cannot be evaluated: %s\n%s", - script.c_str(), list_issues(issues.list).c_str()); - return false; + return fail("rank expression cannot be evaluated: %s\n%s", + script.c_str(), list_issues(issues.list).c_str()); } // avoid costly compilation when only verifying setup if (env.getFeatureMotivation() != env.FeatureMotivation::VERIFY_SETUP) { diff --git a/searchlib/src/vespa/searchlib/fef/blueprint.cpp b/searchlib/src/vespa/searchlib/fef/blueprint.cpp index 7073d0c0ccd..fb758058fe0 100644 --- a/searchlib/src/vespa/searchlib/fef/blueprint.cpp +++ b/searchlib/src/vespa/searchlib/fef/blueprint.cpp @@ -3,13 +3,14 @@ #include "blueprint.h" #include "parametervalidator.h" #include <cassert> +#include <vespa/vespalib/util/stringfmt.h> #include <vespa/log/log.h> LOG_SETUP(".fef.blueprint"); namespace search::fef { -const FeatureType & +std::optional<FeatureType> Blueprint::defineInput(vespalib::stringref inName, AcceptInput accept) { assert(_dependency_handler != nullptr); @@ -19,11 +20,23 @@ Blueprint::defineInput(vespalib::stringref inName, AcceptInput accept) void Blueprint::describeOutput(vespalib::stringref outName, vespalib::stringref desc, - const FeatureType &type) + FeatureType type) { (void) desc; assert(_dependency_handler != nullptr); - _dependency_handler->define_output(outName, type); + _dependency_handler->define_output(outName, std::move(type)); +} + +bool +Blueprint::fail(const char *format, ...) +{ + va_list ap; + va_start(ap, format); + vespalib::string msg = vespalib::make_string_va(format, ap); + va_end(ap); + assert(_dependency_handler != nullptr); + _dependency_handler->fail(msg); + return false; } Blueprint::Blueprint(vespalib::stringref baseName) @@ -52,9 +65,8 @@ Blueprint::setup(const IIndexEnvironment &indexEnv, if (result.valid()) { return setup(indexEnv, result.getParameters()); } else { - LOG(error, "The parameter list used for setting up rank feature %s is not valid: %s", - getBaseName().c_str(), result.getError().c_str()); - return false; + return fail("The parameter list used for setting up rank feature %s is not valid: %s", + getBaseName().c_str(), result.getError().c_str()); } } @@ -62,9 +74,8 @@ bool Blueprint::setup(const IIndexEnvironment &indexEnv, const ParameterList ¶ms) { (void) indexEnv; (void) params; - LOG(error, "The setup function using a typed parameter list does not have a default implementation. " - "Make sure the setup function is implemented in the rank feature %s.", getBaseName().c_str()); - return false; + return fail("The setup function using a typed parameter list does not have a default implementation. " + "Make sure the setup function is implemented in the rank feature %s.", getBaseName().c_str()); } void diff --git a/searchlib/src/vespa/searchlib/fef/blueprint.h b/searchlib/src/vespa/searchlib/fef/blueprint.h index 5d7eb6eb2c0..81f37f7224d 100644 --- a/searchlib/src/vespa/searchlib/fef/blueprint.h +++ b/searchlib/src/vespa/searchlib/fef/blueprint.h @@ -9,6 +9,7 @@ #include "parameter.h" #include "parameterdescriptions.h" #include "feature_type.h" +#include <optional> namespace vespalib { class Stash; } @@ -43,8 +44,9 @@ public: * executor setup. **/ struct DependencyHandler { - virtual const FeatureType &resolve_input(const vespalib::string &feature_name, AcceptInput accept_type) = 0; - virtual void define_output(const vespalib::string &output_name, const FeatureType &type) = 0; + virtual std::optional<FeatureType> resolve_input(const vespalib::string &feature_name, AcceptInput accept_type) = 0; + virtual void define_output(const vespalib::string &output_name, FeatureType type) = 0; + virtual void fail(const vespalib::string &msg) = 0; virtual ~DependencyHandler() = default; }; @@ -87,8 +89,8 @@ protected: * @param inName feature name of input * @param type accepted input type **/ - const FeatureType &defineInput(vespalib::stringref inName, - AcceptInput accept = AcceptInput::NUMBER); + std::optional<FeatureType> defineInput(vespalib::stringref inName, + AcceptInput accept = AcceptInput::NUMBER); /** * Describe an output for this blueprint. This method should be @@ -104,7 +106,22 @@ protected: * @param desc output description **/ void describeOutput(vespalib::stringref outName, vespalib::stringref desc, - const FeatureType &type = FeatureType::number()); + FeatureType type = FeatureType::number()); + + /** + * Fail the setup of this blueprint with the given message. This + * function should be called by the @ref setup function when it + * fails. The failure is handled by the dependency handler to make + * sure we only report the first error for each feature. + * + * @return false + * @param format printf-style format string + **/ + bool fail(const char *format, ...) +#ifdef __GNUC__ + __attribute__ ((format (printf,2,3))) +#endif + ; /** * Used to store a reference to the attribute during prepareSharedState diff --git a/searchlib/src/vespa/searchlib/fef/blueprintresolver.cpp b/searchlib/src/vespa/searchlib/fef/blueprintresolver.cpp index 82a69009e82..c0ca8c857f0 100644 --- a/searchlib/src/vespa/searchlib/fef/blueprintresolver.cpp +++ b/searchlib/src/vespa/searchlib/fef/blueprintresolver.cpp @@ -6,6 +6,7 @@ #include <vespa/vespalib/util/stringfmt.h> #include <stack> #include <cassert> +#include <set> #include <vespa/log/log.h> LOG_SETUP(".fef.blueprintresolver"); @@ -14,6 +15,8 @@ namespace search::fef { namespace { +static const size_t MAX_TRACE_SIZE = 16; + using Accept = Blueprint::AcceptInput; bool is_compatible(bool is_object, Accept accept_type) { @@ -51,15 +54,19 @@ struct Compiler : public Blueprint::DependencyHandler { struct FrameGuard { Stack &stack; explicit FrameGuard(Stack &stack_in) : stack(stack_in) {} - ~FrameGuard() { stack.pop_back(); } + ~FrameGuard() { + stack.back().spec.blueprint->detach_dependency_handler(); + stack.pop_back(); + } }; const BlueprintFactory &factory; const IIndexEnvironment &index_env; - bool compile_error; Stack resolve_stack; ExecutorSpecList &spec_list; FeatureMap &feature_map; + std::set<vespalib::string> setup_set; + std::set<vespalib::string> failed_set; Compiler(const BlueprintFactory &factory_in, const IIndexEnvironment &index_env_in, @@ -67,106 +74,116 @@ struct Compiler : public Blueprint::DependencyHandler { FeatureMap &feature_map_out) : factory(factory_in), index_env(index_env_in), - compile_error(false), resolve_stack(), spec_list(spec_list_out), - feature_map(feature_map_out) {} + feature_map(feature_map_out), + setup_set(), + failed_set() {} + ~Compiler(); Frame &self() { return resolve_stack.back(); } + bool failed() const { return !failed_set.empty(); } - FeatureRef failed(const vespalib::string &feature_name, const vespalib::string &reason) { - if (!compile_error) { - LOG(warning, "invalid rank feature: '%s' (%s)", feature_name.c_str(), reason.c_str()); + FeatureRef fail(const vespalib::string &feature_name, const vespalib::string &reason, bool skip_self = false) { + if (failed_set.count(feature_name) == 0) { + failed_set.insert(feature_name); + LOG(warning, "invalid rank feature '%s': %s", feature_name.c_str(), reason.c_str()); + size_t trace_size = 0; for (size_t i = resolve_stack.size(); i > 0; --i) { const auto &frame = resolve_stack[i - 1]; - if (&frame != &self()) { - LOG(warning, " ... needed by rank feature '%s'", frame.parser.featureName().c_str()); + failed_set.insert(frame.parser.featureName()); + if (!skip_self || (&frame != &self())) { + if (++trace_size <= MAX_TRACE_SIZE) { + LOG(warning, " ... needed by rank feature '%s'", frame.parser.featureName().c_str()); + } } } - compile_error = true; + if (trace_size > MAX_TRACE_SIZE) { + LOG(warning, " ... (%zu more)", (trace_size - MAX_TRACE_SIZE)); + } } - fixup_feature_map(); return FeatureRef(); } - void fixup_feature_map() { - auto itr = feature_map.begin(); - while (itr != feature_map.end()) { - if (itr->second.executor >= spec_list.size()) { - itr = feature_map.erase(itr); - } else { - ++itr; - } - } + void fail_self(const vespalib::string &reason) { + fail(self().parser.featureName(), reason, true); } FeatureRef verify_type(const FeatureNameParser &parser, FeatureRef ref, Accept accept_type) { const auto &spec = spec_list[ref.executor]; - bool is_object = spec.output_types[ref.output]; + bool is_object = spec.output_types[ref.output].is_object(); if (!is_compatible(is_object, accept_type)) { - return failed(parser.featureName(), - vespalib::make_string("output '%s' has wrong type: was %s, expected %s", - parser.output().c_str(), type_str(is_object), accept_type_str(accept_type))); + return fail(parser.featureName(), + vespalib::make_string("output '%s' has wrong type: was %s, expected %s", + parser.output().c_str(), type_str(is_object), accept_type_str(accept_type))); } return ref; } - FeatureRef setup_feature(const FeatureNameParser &parser, Accept accept_type) { - Blueprint::SP blueprint = factory.createBlueprint(parser.baseName()); - if ( ! blueprint) { - return failed(parser.featureName(), - vespalib::make_string("unknown basename: '%s'", parser.baseName().c_str())); - } - resolve_stack.emplace_back(std::move(blueprint), parser); - FrameGuard frame_guard(resolve_stack); - self().spec.blueprint->setName(parser.executorName()); - self().spec.blueprint->attach_dependency_handler(*this); - if (!self().spec.blueprint->setup(index_env, parser.parameters())) { - return failed(parser.featureName(), "invalid parameters"); - } - if (parser.output().empty() && self().spec.output_types.empty()) { - return failed(parser.featureName(), "has no output value"); - } - const auto &feature = feature_map.find(parser.featureName()); - if (feature == feature_map.end()) { - return failed(parser.featureName(), - vespalib::make_string("unknown output: '%s'", parser.output().c_str())); + void setup_executor(const FeatureNameParser &parser) { + if (setup_set.count(parser.executorName()) == 0) { + setup_set.insert(parser.executorName()); + if (Blueprint::SP blueprint = factory.createBlueprint(parser.baseName())) { + resolve_stack.emplace_back(std::move(blueprint), parser); + FrameGuard frame_guard(resolve_stack); + self().spec.blueprint->setName(parser.executorName()); + self().spec.blueprint->attach_dependency_handler(*this); + if (!self().spec.blueprint->setup(index_env, parser.parameters())) { + fail_self("invalid parameters"); + } + if (parser.output().empty() && self().spec.output_types.empty()) { + fail_self("has no output value"); + } + spec_list.push_back(self().spec); // keep all feature_map refs valid + } else { + fail(parser.featureName(), + vespalib::make_string("unknown basename: '%s'", parser.baseName().c_str())); + } } - spec_list.push_back(self().spec); - return verify_type(parser, feature->second, accept_type); } FeatureRef resolve_feature(const vespalib::string &feature_name, Accept accept_type) { FeatureNameParser parser(feature_name); if (!parser.valid()) { - return failed(feature_name, "malformed name"); + return fail(feature_name, "malformed name"); + } + if (failed_set.count(parser.featureName()) > 0) { + return FeatureRef(); } - const auto &feature = feature_map.find(parser.featureName()); - if (feature != feature_map.end()) { - return verify_type(parser, feature->second, accept_type); + auto old_feature = feature_map.find(parser.featureName()); + if (old_feature != feature_map.end()) { + return verify_type(parser, old_feature->second, accept_type); } if ((resolve_stack.size() + 1) > BlueprintResolver::MAX_DEP_DEPTH) { - return failed(parser.featureName(), "dependency graph too deep"); + return fail(parser.featureName(), "dependency graph too deep"); } for (const Frame &frame: resolve_stack) { if (frame.parser.executorName() == parser.executorName()) { - return failed(parser.featureName(), "dependency cycle detected"); + return fail(parser.featureName(), "dependency cycle detected"); } } - return setup_feature(parser, accept_type); + setup_executor(parser); + auto new_feature = feature_map.find(parser.featureName()); + if (new_feature != feature_map.end()) { + return verify_type(parser, new_feature->second, accept_type); + } + return fail(parser.featureName(), + vespalib::make_string("unknown output: '%s'", parser.output().c_str())); } - const FeatureType &resolve_input(const vespalib::string &feature_name, Accept accept_type) override { + std::optional<FeatureType> resolve_input(const vespalib::string &feature_name, Accept accept_type) override { assert(self().spec.output_types.empty()); // require: 'resolve inputs' before 'define outputs' auto ref = resolve_feature(feature_name, accept_type); if (!ref.valid()) { - return FeatureType::number(); + // fail silently here to avoid mutiple traces for the same root error + failed_set.insert(self().parser.featureName()); + return std::nullopt; } self().spec.inputs.push_back(ref); return spec_list[ref.executor].output_types[ref.output]; } - void define_output(const vespalib::string &output_name, const FeatureType &type) override { + void define_output(const vespalib::string &output_name, FeatureType type) override { vespalib::string feature_name = self().parser.executorName(); if (!output_name.empty()) { feature_name.push_back('.'); @@ -177,10 +194,16 @@ struct Compiler : public Blueprint::DependencyHandler { feature_map.emplace(self().parser.executorName(), output_ref); } feature_map.emplace(feature_name, output_ref); - self().spec.output_types.push_back(type); + self().spec.output_types.push_back(std::move(type)); + } + + void fail(const vespalib::string &msg) override { + fail_self(msg); } }; +Compiler::~Compiler() = default; + } // namespace search::fef::<unnamed> BlueprintResolver::ExecutorSpec::ExecutorSpec(Blueprint::SP blueprint_in) @@ -216,7 +239,7 @@ BlueprintResolver::compile() Compiler compiler(_factory, _indexEnv, _executorSpecs, _featureMap); for (const auto &seed: _seeds) { auto ref = compiler.resolve_feature(seed, Blueprint::AcceptInput::ANY); - if (compiler.compile_error) { + if (compiler.failed()) { return false; } _seedMap.emplace(FeatureNameParser(seed).featureName(), ref); diff --git a/searchlib/src/vespa/searchlib/fef/feature_type.h b/searchlib/src/vespa/searchlib/fef/feature_type.h index cdcff93c821..5763df1fe81 100644 --- a/searchlib/src/vespa/searchlib/fef/feature_type.h +++ b/searchlib/src/vespa/searchlib/fef/feature_type.h @@ -13,9 +13,9 @@ namespace fef { * the low-level eval library. A feature can either be a simple number * represented by a double or a polymorph value represented with an * object. The ranking framework itself will mostly care about the - * representation (number/object) and not the specific type, hence the - * implicit cast to bool. The type function is used to extract the - * underlying type and is only allowed for features that are objects. + * representation (number/object) and not the specific type. The type + * function is used to extract the underlying type and is only allowed + * for features that are objects. **/ class FeatureType { private: @@ -26,8 +26,8 @@ private: FeatureType(TYPE_UP type_in) : _type(std::move(type_in)) {} public: FeatureType(const FeatureType &rhs); + FeatureType(FeatureType &&rhs) = default; bool is_object() const { return (_type.get() != nullptr); } - operator bool() const { return is_object(); } const TYPE &type() const; static const FeatureType &number() { return _number; } static FeatureType object(const TYPE &type_in); diff --git a/searchlib/src/vespa/searchlib/fef/rank_program.cpp b/searchlib/src/vespa/searchlib/fef/rank_program.cpp index bd8abe41afb..81da34cabd0 100644 --- a/searchlib/src/vespa/searchlib/fef/rank_program.cpp +++ b/searchlib/src/vespa/searchlib/fef/rank_program.cpp @@ -139,7 +139,7 @@ RankProgram::resolve(const BlueprintResolver::FeatureMap &features, bool unbox_s for (const auto &entry: features) { const auto &name = entry.first; auto ref = entry.second; - bool is_object = specs[ref.executor].output_types[ref.output]; + bool is_object = specs[ref.executor].output_types[ref.output].is_object(); FeatureExecutor *executor = _executors[ref.executor]; const NumberOrObject *raw_value = executor->outputs().get_raw(ref.output); LazyValue lazy_value = check_const(raw_value) ? LazyValue(raw_value) : LazyValue(raw_value, executor); @@ -215,7 +215,7 @@ RankProgram::setup(const MatchData &md, } for (const auto &seed_entry: _resolver->getSeedMap()) { auto seed = seed_entry.second; - if (specs[seed.executor].output_types[seed.output]) { + if (specs[seed.executor].output_types[seed.output].is_object()) { unbox(seed, md); } } diff --git a/searchlib/src/vespa/searchlib/fef/test/dummy_dependency_handler.cpp b/searchlib/src/vespa/searchlib/fef/test/dummy_dependency_handler.cpp index 3a6e4c8db2d..4fd36a69c71 100644 --- a/searchlib/src/vespa/searchlib/fef/test/dummy_dependency_handler.cpp +++ b/searchlib/src/vespa/searchlib/fef/test/dummy_dependency_handler.cpp @@ -13,7 +13,8 @@ DummyDependencyHandler::DummyDependencyHandler(Blueprint &blueprint_in) input(), accept_input(), output(), - output_type() + output_type(), + fail_msg() { blueprint.attach_dependency_handler(*this); } @@ -29,7 +30,7 @@ DummyDependencyHandler::define_object_input(const vespalib::string &name, const object_type_map.emplace(name, FeatureType::object(type)); } -const FeatureType & +std::optional<FeatureType> DummyDependencyHandler::resolve_input(const vespalib::string &feature_name, Blueprint::AcceptInput accept_type) { input.push_back(feature_name); @@ -38,19 +39,28 @@ DummyDependencyHandler::resolve_input(const vespalib::string &feature_name, Blue if (pos == object_type_map.end()) { if (accept_type == Blueprint::AcceptInput::OBJECT) { accept_type_mismatch = true; + return std::nullopt; } return FeatureType::number(); } if (accept_type == Blueprint::AcceptInput::NUMBER) { accept_type_mismatch = true; + return std::nullopt; } return pos->second; } -void DummyDependencyHandler::define_output(const vespalib::string &output_name, const FeatureType &type) +void +DummyDependencyHandler::define_output(const vespalib::string &output_name, FeatureType type) { output.push_back(output_name); - output_type.push_back(type); + output_type.push_back(std::move(type)); +} + +void +DummyDependencyHandler::fail(const vespalib::string &msg) +{ + fail_msg = msg; } } // namespace search::fef::test diff --git a/searchlib/src/vespa/searchlib/fef/test/dummy_dependency_handler.h b/searchlib/src/vespa/searchlib/fef/test/dummy_dependency_handler.h index 1c24858b12b..ca70d0943cf 100644 --- a/searchlib/src/vespa/searchlib/fef/test/dummy_dependency_handler.h +++ b/searchlib/src/vespa/searchlib/fef/test/dummy_dependency_handler.h @@ -26,12 +26,14 @@ struct DummyDependencyHandler : public Blueprint::DependencyHandler std::vector<Blueprint::AcceptInput> accept_input; std::vector<vespalib::string> output; std::vector<FeatureType> output_type; + vespalib::string fail_msg; explicit DummyDependencyHandler(Blueprint &blueprint_in); ~DummyDependencyHandler(); void define_object_input(const vespalib::string &name, const vespalib::eval::ValueType &type); - const FeatureType &resolve_input(const vespalib::string &feature_name, Blueprint::AcceptInput accept_type) override; - void define_output(const vespalib::string &output_name, const FeatureType &type) override; + std::optional<FeatureType> resolve_input(const vespalib::string &feature_name, Blueprint::AcceptInput accept_type) override; + void define_output(const vespalib::string &output_name, FeatureType type) override; + void fail(const vespalib::string &msg) override; }; } // namespace search::fef::test |