Merge pull request #15678 from vespa-engine/havardpe/remove-engine-or-factory

remove EngineOrFactory

11 files changed, 12 insertions, 286 deletions

diff --git a/eval/CMakeLists.txt b/eval/CMakeLists.txt
index c4fd211897b..d9dc7e587b0 100644
--- a/eval/CMakeLists.txt
+++ b/eval/CMakeLists.txt
@@ -15,7 +15,6 @@ vespa_define_module(
     src/tests/eval/array_array_map
     src/tests/eval/compile_cache
     src/tests/eval/compiled_function
-    src/tests/eval/engine_or_factory
     src/tests/eval/fast_sparse_map
     src/tests/eval/fast_value
     src/tests/eval/function
diff --git a/eval/src/tests/eval/engine_or_factory/CMakeLists.txt b/eval/src/tests/eval/engine_or_factory/CMakeLists.txt
deleted file mode 100644
index f0bd0f63251..00000000000
--- a/eval/src/tests/eval/engine_or_factory/CMakeLists.txt
+++ /dev/null
@@ -1,17 +0,0 @@
-# Copyright Verizon Media. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
-vespa_add_executable(eval_engine_or_factory_test_app TEST
-    SOURCES
-    engine_or_factory_test.cpp
-    DEPENDS
-    vespaeval
-    GTest::GTest
-)
-vespa_add_test(NAME eval_engine_or_factory_test_app COMMAND eval_engine_or_factory_test_app)
-vespa_add_executable(eval_engine_or_factory_override_test_app TEST
-    SOURCES
-    engine_or_factory_override_test.cpp
-    DEPENDS
-    vespaeval
-    GTest::GTest
-)
-vespa_add_test(NAME eval_engine_or_factory_override_test_app COMMAND eval_engine_or_factory_override_test_app)
diff --git a/eval/src/tests/eval/engine_or_factory/engine_or_factory_override_test.cpp b/eval/src/tests/eval/engine_or_factory/engine_or_factory_override_test.cpp
deleted file mode 100644
index b06e2723d74..00000000000
--- a/eval/src/tests/eval/engine_or_factory/engine_or_factory_override_test.cpp
+++ /dev/null
@@ -1,24 +0,0 @@
-// Copyright Verizon Media. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
-
-#include <vespa/eval/eval/engine_or_factory.h>
-#include <vespa/eval/eval/fast_value.h>
-#include <vespa/eval/eval/simple_value.h>
-#include <vespa/vespalib/util/exceptions.h>
-#include <vespa/vespalib/gtest/gtest.h>
-
-using namespace vespalib::eval;
-
-TEST(EngineOrFactoryOverrideTest, set_can_override_get_result) {
-    EngineOrFactory::set(SimpleValueBuilderFactory::get());
-    EXPECT_EQ(EngineOrFactory::get().to_string(), "SimpleValueBuilderFactory");
-}
-
-TEST(EngineOrFactoryOverrideTest, set_with_same_value_is_allowed) {
-    EngineOrFactory::set(SimpleValueBuilderFactory::get());
-}
-
-TEST(EngineOrFactoryOverrideTest, set_with_another_value_is_not_allowed) {
-    EXPECT_THROW(EngineOrFactory::set(FastValueBuilderFactory::get()), vespalib::IllegalStateException);
-}
-
-GTEST_MAIN_RUN_ALL_TESTS()
diff --git a/eval/src/tests/eval/engine_or_factory/engine_or_factory_test.cpp b/eval/src/tests/eval/engine_or_factory/engine_or_factory_test.cpp
deleted file mode 100644
index 26e8e8e75ec..00000000000
--- a/eval/src/tests/eval/engine_or_factory/engine_or_factory_test.cpp
+++ /dev/null
@@ -1,23 +0,0 @@
-// Copyright Verizon Media. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
-
-#include <vespa/eval/eval/engine_or_factory.h>
-#include <vespa/eval/eval/fast_value.h>
-#include <vespa/eval/eval/simple_value.h>
-#include <vespa/vespalib/util/exceptions.h>
-#include <vespa/vespalib/gtest/gtest.h>
-
-using namespace vespalib::eval;
-
-TEST(EngineOrFactoryTest, default_is_fast_value_builder_factory) {
-    EXPECT_EQ(EngineOrFactory::get().to_string(), "FastValueBuilderFactory");
-}
-
-TEST(EngineOrFactoryTest, set_with_same_value_is_allowed) {
-    EngineOrFactory::set(FastValueBuilderFactory::get());
-}
-
-TEST(EngineOrFactoryTest, set_with_another_value_is_not_allowed) {
-    EXPECT_THROW(EngineOrFactory::set(SimpleValueBuilderFactory::get()), vespalib::IllegalStateException);
-}
-
-GTEST_MAIN_RUN_ALL_TESTS()
diff --git a/eval/src/vespa/eval/eval/CMakeLists.txt b/eval/src/vespa/eval/eval/CMakeLists.txt
index 66c807565cc..d32841520a6 100644
--- a/eval/src/vespa/eval/eval/CMakeLists.txt
+++ b/eval/src/vespa/eval/eval/CMakeLists.txt
@@ -9,7 +9,6 @@ vespa_add_library(eval_eval OBJECT
     compile_tensor_function.cpp
     delete_node.cpp
     double_value_builder.cpp
-    engine_or_factory.cpp
     fast_forest.cpp
     fast_sparse_map.cpp
     fast_value.cpp
diff --git a/eval/src/vespa/eval/eval/engine_or_factory.cpp b/eval/src/vespa/eval/eval/engine_or_factory.cpp
deleted file mode 100644
index 5b46fd61951..00000000000
--- a/eval/src/vespa/eval/eval/engine_or_factory.cpp
+++ /dev/null
@@ -1,116 +0,0 @@
-// Copyright Verizon Media. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
-
-#include "engine_or_factory.h"
-#include "fast_value.h"
-#include "simple_value.h"
-#include "value_codec.h"
-#include <vespa/eval/instruction/generic_concat.h>
-#include <vespa/eval/instruction/generic_join.h>
-#include <vespa/eval/instruction/generic_map.h>
-#include <vespa/eval/instruction/generic_merge.h>
-#include <vespa/eval/instruction/generic_reduce.h>
-#include <vespa/eval/instruction/generic_rename.h>
-#include "tensor_engine.h"
-#include <vespa/vespalib/data/memory.h>
-#include <vespa/vespalib/objects/nbostream.h>
-#include <vespa/vespalib/util/exceptions.h>
-#include <vespa/vespalib/util/stash.h>
-#include <vespa/vespalib/util/stringfmt.h>
-
-using vespalib::make_string_short::fmt;
-
-using namespace vespalib::eval::instruction;
-
-namespace vespalib::eval {
-
-EngineOrFactory EngineOrFactory::_default{FastValueBuilderFactory::get()};
-
-
-EngineOrFactory
-EngineOrFactory::get_shared(EngineOrFactory hint)
-{
-    static EngineOrFactory shared{hint};
-    return shared;
-}
-
-TensorSpec
-EngineOrFactory::to_spec(const Value &value) const
-{
-    if (is_engine()) {
-        return engine().to_spec(value);
-    } else {
-        return factory(), spec_from_value(value);
-    }
-}
-
-std::unique_ptr<Value>
-EngineOrFactory::from_spec(const TensorSpec &spec) const
-{
-    if (is_engine()) {
-        return engine().from_spec(spec);
-    } else {
-        return value_from_spec(spec, factory());
-    }
-}
-
-void
-EngineOrFactory::encode(const Value &value, nbostream &output) const
-{
-    if (is_engine()) {
-        return engine().encode(value, output);
-    } else {
-        return factory(), encode_value(value, output);
-    }
-}
-
-std::unique_ptr<Value>
-EngineOrFactory::decode(nbostream &input) const
-{
-    if (is_engine()) {
-        return engine().decode(input);
-    } else {
-        return decode_value(input, factory());
-    }
-}
-
-std::unique_ptr<Value>
-EngineOrFactory::copy(const Value &value)
-{
-    nbostream stream;
-    encode(value, stream);
-    return decode(stream);
-}
-
-void
-EngineOrFactory::set(EngineOrFactory wanted)
-{
-    assert(wanted.is_factory());
-    auto engine = get_shared(wanted);
-    if (engine._value != wanted._value) {
-        auto msg = fmt("EngineOrFactory: trying to set implementation to [%s] when [%s] is already in use",
-                       wanted.to_string().c_str(), engine.to_string().c_str());
-        throw IllegalStateException(msg);
-    }
-}
-
-EngineOrFactory
-EngineOrFactory::get()
-{
-    return get_shared(_default);
-}
-
-vespalib::string
-EngineOrFactory::to_string() const
-{
-    if (is_factory()) {
-        if (&factory() == &FastValueBuilderFactory::get()) {
-            return "FastValueBuilderFactory";
-        }
-        if (&factory() == &SimpleValueBuilderFactory::get()) {
-            return "SimpleValueBuilderFactory";
-        }
-    }
-    return "???";
-}
-
-}
diff --git a/eval/src/vespa/eval/eval/engine_or_factory.h b/eval/src/vespa/eval/eval/engine_or_factory.h
deleted file mode 100644
index 7269f22159d..00000000000
--- a/eval/src/vespa/eval/eval/engine_or_factory.h
+++ /dev/null
@@ -1,56 +0,0 @@
-// Copyright Verizon Media. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
-
-#pragma once
-
-#include "aggr.h"
-#include "tensor_spec.h"
-#include "operation.h"
-#include <variant>
-
-namespace vespalib {
-class Stash;
-class nbostream;
-}
-
-namespace vespalib::eval {
-
-struct TensorEngine;
-struct ValueBuilderFactory;
-struct TensorFunction;
-
-/**
- * This utility class contains a reference to either a TensorEngine or
- * a ValueBuilderFactory. This is needed during a transition period to
- * support both evaluation models. We want to get rid of the
- * TensorEngine concept since using the Value API directly removes the
- * need to constrain operations to only calculate on tensors belonging
- * to the same tensor engine. The factory is a hint to the preferred
- * Value implementation.
- **/
-class EngineOrFactory {
-private:
-    using engine_t = const TensorEngine *;
-    using factory_t = const ValueBuilderFactory *;
-    std::variant<engine_t,factory_t> _value;
-    static EngineOrFactory _default;
-    static EngineOrFactory get_shared(EngineOrFactory hint);
-public:
-    EngineOrFactory(const ValueBuilderFactory &factory_in) : _value(&factory_in) {}
-    bool is_engine() const { return std::holds_alternative<engine_t>(_value); }
-    bool is_factory() const { return std::holds_alternative<factory_t>(_value); }
-    const TensorEngine &engine() const { return *std::get<engine_t>(_value); }
-    const ValueBuilderFactory &factory() const { return *std::get<factory_t>(_value); }
-    // functions that can be called with either engine or factory
-    TensorSpec to_spec(const Value &value) const;
-    std::unique_ptr<Value> from_spec(const TensorSpec &spec) const;
-    void encode(const Value &value, nbostream &output) const;
-    std::unique_ptr<Value> decode(nbostream &input) const;
-    std::unique_ptr<Value> copy(const Value &value);
-    // global switch with default; call set before get to override the default
-    static void set(EngineOrFactory wanted);
-    static EngineOrFactory get();
-    // try to describe the value held by this object as a human-readable string
-    vespalib::string to_string() const;
-};
-
-}
diff --git a/eval/src/vespa/eval/eval/value.cpp b/eval/src/vespa/eval/eval/value.cpp
index 267d2443112..7abc8d568cb 100644
--- a/eval/src/vespa/eval/eval/value.cpp
+++ b/eval/src/vespa/eval/eval/value.cpp
@@ -1,7 +1,9 @@
 // Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 
 #include "value.h"
+#include "value_codec.h"
 #include <vespa/vespalib/util/typify.h>
+#include <vespa/vespalib/objects/nbostream.h>
 
 namespace vespalib {
 namespace eval {
@@ -62,5 +64,13 @@ ValueType ScalarValue<T>::_type = ValueType::make_type(get_cell_type<T>(), {});
 template class ScalarValue<double>;
 template class ScalarValue<float>;
 
+std::unique_ptr<Value>
+ValueBuilderFactory::copy(const Value &value) const
+{
+    nbostream stream;
+    encode_value(value, stream);
+    return decode_value(stream, *this);
+}
+
 } // namespace vespalib::eval
 } // namespace vespalib
diff --git a/eval/src/vespa/eval/eval/value.h b/eval/src/vespa/eval/eval/value.h
index e876ba7b472..cf4e21029fc 100644
--- a/eval/src/vespa/eval/eval/value.h
+++ b/eval/src/vespa/eval/eval/value.h
@@ -198,6 +198,7 @@ struct ValueBuilderFactory {
     {
         return create_value_builder<T>(type, type.count_mapped_dimensions(), type.dense_subspace_size(), 1);
     }
+    std::unique_ptr<Value> copy(const Value &value) const;
     virtual ~ValueBuilderFactory() {}
 protected:
     virtual std::unique_ptr<ValueBuilderBase> create_value_builder_base(const ValueType &type,
diff --git a/eval/src/vespa/eval/tensor/partial_update.cpp b/eval/src/vespa/eval/tensor/partial_update.cpp
index 82cd5553741..0f3ebe9537b 100644
--- a/eval/src/vespa/eval/tensor/partial_update.cpp
+++ b/eval/src/vespa/eval/tensor/partial_update.cpp
@@ -417,42 +417,4 @@ TensorPartialUpdate::remove(const Value &input, const Value &remove_spec, const
             input, remove_spec, factory);
 }
 
-Value::UP
-TensorPartialUpdate::modify(const Value &input, join_fun_t function,
-                            const Value &modifier, EngineOrFactory engine)
-{
-    if (engine.is_engine()) {
-        abort();
-    }
-    return modify(input, function, modifier, engine.factory());
-
-}
-
-Value::UP
-TensorPartialUpdate::add(const Value &input, const Value &add_cells, EngineOrFactory engine)
-{
-    if (engine.is_engine()) {
-        abort();
-    }
-    return add(input, add_cells, engine.factory());
-}
-
-Value::UP
-TensorPartialUpdate::remove(const Value &input, const Value &remove_spec, EngineOrFactory engine)
-{
-    if (engine.is_engine()) {
-        abort();
-    }
-    return remove(input, remove_spec, engine.factory());
-}
-
-bool
-TensorPartialUpdate::check_suitably_sparse(const Value &modifier, const EngineOrFactory engine)
-{
-    if (engine.is_engine()) {
-        abort();
-    }
-    return modifier.type().is_sparse();
-}
-
 } // namespace
diff --git a/eval/src/vespa/eval/tensor/partial_update.h b/eval/src/vespa/eval/tensor/partial_update.h
index 6bbef6a8189..b3e9d32fca8 100644
--- a/eval/src/vespa/eval/tensor/partial_update.h
+++ b/eval/src/vespa/eval/tensor/partial_update.h
@@ -2,7 +2,7 @@
 
 #pragma once
 
-#include <vespa/eval/eval/engine_or_factory.h>
+#include <vespa/eval/eval/fast_value.h>
 #include <vespa/eval/eval/value.h>
 #include <vespa/eval/eval/operation.h>
 
@@ -39,15 +39,6 @@ struct TensorPartialUpdate {
      *  Returns null pointer if these constraints are violated.
      **/
     static Value::UP remove(const Value &input, const Value &remove_spec, const ValueBuilderFactory &factory);
-
-    /* Backwards compatibility adapters. TODO: remove */
-    using EngineOrFactory = vespalib::eval::EngineOrFactory;
-    static Value::UP modify(const Value &input, join_fun_t function,
-                            const Value &modifier, EngineOrFactory factory);
-    static Value::UP add(const Value &input, const Value &add_cells, EngineOrFactory factory);
-    static Value::UP remove(const Value &input, const Value &remove_spec, EngineOrFactory factory);
-    /** Check if the given value can be used as a modifier or remove_spec */
-    static bool check_suitably_sparse(const Value &modifier,  EngineOrFactory factory);
 };
 
 } // namespace