Merge pull request #20956 from vespa-engine/havardpe/auto-unboxing-of-scalar-results-from-interpreted-ranking-expressions

auto-unbox scalar results from interpreted ranking expressions

7 files changed, 95 insertions, 26 deletions

diff --git a/searchlib/src/tests/features/ranking_expression/ranking_expression_test.cpp b/searchlib/src/tests/features/ranking_expression/ranking_expression_test.cpp
index b65f7d08e58..65e067ae7b1 100644
--- a/searchlib/src/tests/features/ranking_expression/ranking_expression_test.cpp
+++ b/searchlib/src/tests/features/ranking_expression/ranking_expression_test.cpp
@@ -128,7 +128,11 @@ TEST("require that expression with only number inputs produce number output (com
 }
 
 TEST("require that expression with object input produces object output (interpreted)") {
-    TEST_DO(verify_output_type({{"b", "double"}}, "a*b", FeatureType::object(ValueType::double_type())));
+    TEST_DO(verify_output_type({{"b", "tensor(x{})"}}, "a*b", FeatureType::object(ValueType::from_spec("tensor(x{})"))));
+}
+
+TEST("require that scalar expressions are auto-unboxed (interpreted)") {
+    TEST_DO(verify_output_type({{"b", "tensor(x{})"}}, "reduce(a*b,sum)", FeatureType::number()));
 }
 
 TEST("require that ranking expression can resolve to concrete complex type") {
@@ -138,7 +142,7 @@ TEST("require that ranking expression can resolve to concrete complex type") {
 
 TEST("require that ranking expression can be external") {
     TEST_DO(verify_output_type({}, "a*b", FeatureType::number(), "my_expr"));
-    TEST_DO(verify_output_type({{"b", "double"}}, "a*b", FeatureType::object(ValueType::double_type()), "my_expr"));
+    TEST_DO(verify_output_type({{"b", "double"}}, "a*b", FeatureType::number(), "my_expr"));
     TEST_DO(verify_output_type({{"a", "tensor(x{},y{})"}, {"b", "tensor(y{},z{})"}}, "a*b",
                                FeatureType::object(ValueType::from_spec("tensor(x{},y{},z{})")), "my_expr"));
 }
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 46aaf7369e3..3639da05b9e 100644
--- a/searchlib/src/tests/fef/object_passing/object_passing_test.cpp
+++ b/searchlib/src/tests/fef/object_passing/object_passing_test.cpp
@@ -79,7 +79,8 @@ struct Fixture {
     explicit Fixture() {
         factory.addPrototype(std::make_shared<ValueBlueprint>());
         factory.addPrototype(std::make_shared<UnboxBlueprint>());
-        factory.addPrototype(std::make_shared<ProxyBlueprint>("box",         Blueprint::AcceptInput::NUMBER, true));
+        factory.addPrototype(std::make_shared<ProxyBlueprint>("do_box",      Blueprint::AcceptInput::NUMBER, true));
+        factory.addPrototype(std::make_shared<ProxyBlueprint>("do_unbox",    Blueprint::AcceptInput::OBJECT, false));
         factory.addPrototype(std::make_shared<ProxyBlueprint>("maybe_box",   Blueprint::AcceptInput::ANY,    true));
         factory.addPrototype(std::make_shared<ProxyBlueprint>("maybe_unbox", Blueprint::AcceptInput::ANY,    false));
     }
@@ -106,26 +107,33 @@ struct Fixture {
 };
 
 TEST_F("require that values can be boxed and unboxed", Fixture()) {
-    EXPECT_EQUAL(3.0, f1.eval("box(value(3))"));
-    EXPECT_EQUAL(0.0, f1.eval("box(value(3)).was_object"));
-    EXPECT_EQUAL(3.0, f1.eval("unbox(box(value(3)))"));
-    EXPECT_EQUAL(1.0, f1.eval("maybe_unbox(box(value(3))).was_object"));
-    EXPECT_EQUAL(3.0, f1.eval("box(unbox(box(value(3))))"));
-    EXPECT_EQUAL(0.0, f1.eval("box(unbox(box(value(3)))).was_object"));
+    EXPECT_EQUAL(3.0, f1.eval("do_box(value(3))"));
+    EXPECT_EQUAL(0.0, f1.eval("do_box(value(3)).was_object"));
+    EXPECT_EQUAL(3.0, f1.eval("do_unbox(do_box(value(3)))"));
+    EXPECT_EQUAL(1.0, f1.eval("maybe_unbox(do_box(value(3))).was_object"));
+    EXPECT_EQUAL(3.0, f1.eval("do_box(do_unbox(do_box(value(3))))"));
+    EXPECT_EQUAL(0.0, f1.eval("do_box(do_unbox(do_box(value(3)))).was_object"));
 }
 
 TEST_F("require that output features may be either objects or numbers", Fixture()) {
     EXPECT_TRUE(f1.verify("value(3)"));
-    EXPECT_TRUE(f1.verify("box(value(3))"));
+    EXPECT_TRUE(f1.verify("do_box(value(3))"));
 }
 
 TEST_F("require that feature input/output types must be compatible", Fixture()) {
-    EXPECT_TRUE(!f1.verify("unbox(value(3))"));
+    EXPECT_TRUE(!f1.verify("do_unbox(value(3))"));
     EXPECT_TRUE(f1.verify("maybe_unbox(value(3))"));
-    EXPECT_TRUE(f1.verify("unbox(box(value(3)))"));
-    EXPECT_TRUE(!f1.verify("unbox(box(box(value(3))))"));
-    EXPECT_TRUE(f1.verify("unbox(maybe_box(box(value(3))))"));
-    EXPECT_TRUE(f1.verify("unbox(box(unbox(box(value(3)))))"));
+    EXPECT_TRUE(f1.verify("do_unbox(do_box(value(3)))"));
+    EXPECT_TRUE(!f1.verify("do_unbox(do_box(do_box(value(3))))"));
+    EXPECT_TRUE(f1.verify("do_unbox(maybe_box(do_box(value(3))))"));
+    EXPECT_TRUE(f1.verify("do_unbox(do_box(do_unbox(do_box(value(3)))))"));
+}
+
+TEST_F("require that 'unbox' feature works for both numbers and objects", Fixture()) {
+    EXPECT_EQUAL(3.0, f1.eval("unbox(value(3))"));
+    EXPECT_EQUAL(3.0, f1.eval("unbox(do_box(value(3)))"));
+    EXPECT_EQUAL(0.0, f1.eval("maybe_unbox(unbox(do_box(value(3)))).was_object"));
+    EXPECT_EQUAL(0.0, f1.eval("maybe_unbox(unbox(value(3))).was_object"));
 }
 
 TEST_MAIN() { TEST_RUN_ALL(); }
diff --git a/searchlib/src/tests/fef/rank_program/rank_program_test.cpp b/searchlib/src/tests/fef/rank_program/rank_program_test.cpp
index 11f4a01888b..40e1cee3629 100644
--- a/searchlib/src/tests/fef/rank_program/rank_program_test.cpp
+++ b/searchlib/src/tests/fef/rank_program/rank_program_test.cpp
@@ -339,7 +339,7 @@ TEST_F("require that interpreted ranking expressions are always lazy", Fixture()
     f1.add_expr("rank", "if(docid<10,box(track(ivalue(1))),track(ivalue(2)))");
     f1.compile();
     EXPECT_EQUAL(7u, f1.program.num_executors());
-    EXPECT_EQUAL(8u, count_features(f1.program));
+    EXPECT_EQUAL(7u, count_features(f1.program));
     EXPECT_EQUAL(0u, count_const_features(f1.program));
     EXPECT_EQUAL(f1.track_cnt, 0u);
     EXPECT_EQUAL(f1.get(expr_feature("rank"),  5), 1.0);
@@ -364,8 +364,8 @@ TEST_F("require that lazy compiled ranking expressions are pure", Fixture()) {
 
 TEST_F("require that interpreted ranking expressions are pure", Fixture()) {
     f1.add_expr("rank", "box(value(7))").compile();
-    EXPECT_EQUAL(4u, count_features(f1.program));
-    EXPECT_EQUAL(4u, count_const_features(f1.program));
+    EXPECT_EQUAL(3u, count_features(f1.program));
+    EXPECT_EQUAL(3u, count_const_features(f1.program));
     EXPECT_EQUAL(f1.get(), 7.0);
 }
 
diff --git a/searchlib/src/vespa/searchlib/features/rankingexpressionfeature.cpp b/searchlib/src/vespa/searchlib/features/rankingexpressionfeature.cpp
index 069eab190ec..434040c7dad 100644
--- a/searchlib/src/vespa/searchlib/features/rankingexpressionfeature.cpp
+++ b/searchlib/src/vespa/searchlib/features/rankingexpressionfeature.cpp
@@ -127,6 +127,24 @@ public:
     void execute(uint32_t docId) override;
 };
 
+/**
+ * Implements the executor for interpreted ranking expressions (with
+ * tensor support) that will unbox the result.
+ **/
+class UnboxingInterpretedRankingExpressionExecutor : public fef::FeatureExecutor
+{
+private:
+    const InterpretedFunction   &_function;
+    InterpretedFunction::Context _context;
+    MyLazyParams                 _params;
+
+public:
+    UnboxingInterpretedRankingExpressionExecutor(const InterpretedFunction &function,
+                                                 ConstArrayRef<char> input_is_object);
+    bool isPure() override { return true; }
+    void execute(uint32_t docId) override;
+};
+
 //-----------------------------------------------------------------------------
 
 FastForestExecutor::FastForestExecutor(ArrayRef<float> param_space, const FastForest &forest)
@@ -215,6 +233,22 @@ InterpretedRankingExpressionExecutor::execute(uint32_t)
 
 //-----------------------------------------------------------------------------
 
+UnboxingInterpretedRankingExpressionExecutor::UnboxingInterpretedRankingExpressionExecutor(const InterpretedFunction &function,
+                                                                                           ConstArrayRef<char> input_is_object)
+    : _function(function),
+      _context(function),
+      _params(inputs(), input_is_object)
+{
+}
+
+void
+UnboxingInterpretedRankingExpressionExecutor::execute(uint32_t)
+{
+    outputs().set_number(0, _function.eval(_context, _params).as_double());
+}
+
+//-----------------------------------------------------------------------------
+
 RankingExpressionBlueprint::RankingExpressionBlueprint()
     : RankingExpressionBlueprint(std::make_shared<rankingexpression::NullExpressionReplacer>()) {}
 
@@ -225,7 +259,8 @@ RankingExpressionBlueprint::RankingExpressionBlueprint(rankingexpression::Expres
       _fast_forest(),
       _interpreted_function(),
       _compile_token(),
-      _input_is_object()
+      _input_is_object(),
+      _should_unbox(false)
 {
 }
 
@@ -328,9 +363,10 @@ RankingExpressionBlueprint::setup(const fef::IIndexEnvironment &env,
         } else {
             _interpreted_function.reset(new InterpretedFunction(FastValueBuilderFactory::get(),
                                                                 *rank_function, node_types));
+            _should_unbox = root_type.is_double();
         }
     }
-    FeatureType output_type = do_compile
+    FeatureType output_type = (do_compile || _should_unbox)
                               ? FeatureType::number()
                               : FeatureType::object(root_type);
     describeOutput("out", "The result of running the contained ranking expression.", output_type);
@@ -359,7 +395,11 @@ RankingExpressionBlueprint::createExecutor(const fef::IQueryEnvironment &env, ve
     }
     if (_interpreted_function) {
         ConstArrayRef<char> input_is_object = stash.copy_array<char>(_input_is_object);
-        return stash.create<InterpretedRankingExpressionExecutor>(*_interpreted_function, input_is_object);
+        if (_should_unbox) {
+            return stash.create<UnboxingInterpretedRankingExpressionExecutor>(*_interpreted_function, input_is_object);
+        } else {
+            return stash.create<InterpretedRankingExpressionExecutor>(*_interpreted_function, input_is_object);
+        }
     }
     if (_fast_forest) {
         ArrayRef<float> param_space = stash.create_array<float>(_input_is_object.size(), 0.0);
diff --git a/searchlib/src/vespa/searchlib/features/rankingexpressionfeature.h b/searchlib/src/vespa/searchlib/features/rankingexpressionfeature.h
index 43aafe67da1..a759c7855d6 100644
--- a/searchlib/src/vespa/searchlib/features/rankingexpressionfeature.h
+++ b/searchlib/src/vespa/searchlib/features/rankingexpressionfeature.h
@@ -24,6 +24,7 @@ private:
     vespalib::eval::InterpretedFunction::UP    _interpreted_function;
     vespalib::eval::CompileCache::Token::UP    _compile_token;
     std::vector<char>                          _input_is_object;
+    bool                                       _should_unbox;
 
 public:
     RankingExpressionBlueprint();
diff --git a/searchlib/src/vespa/searchlib/fef/test/plugin/unbox.cpp b/searchlib/src/vespa/searchlib/fef/test/plugin/unbox.cpp
index a82f4287d00..83df24ac543 100644
--- a/searchlib/src/vespa/searchlib/fef/test/plugin/unbox.cpp
+++ b/searchlib/src/vespa/searchlib/fef/test/plugin/unbox.cpp
@@ -14,10 +14,18 @@ struct UnboxExecutor : FeatureExecutor {
     }
 };
 
+struct ForwardExecutor : FeatureExecutor {
+    bool isPure() override { return true; }
+    void execute(uint32_t) override {
+        outputs().set_number(0, inputs().get_number(0));
+    }
+};
+
 } // namespace search::fef::test::<unnamed>
 
 UnboxBlueprint::UnboxBlueprint()
-    : Blueprint("unbox")
+  : Blueprint("unbox"),
+    _was_object(false)
 {
 }
 
@@ -41,15 +49,22 @@ UnboxBlueprint::getDescriptions() const
 bool
 UnboxBlueprint::setup(const IIndexEnvironment &, const ParameterList &params)
 {
-    defineInput(params[0].getValue(), AcceptInput::OBJECT);
-    describeOutput("value", "unboxed value", FeatureType::number());
-    return true;
+    if (auto input = defineInput(params[0].getValue(), AcceptInput::ANY)) {
+        _was_object = input.value().is_object();
+        describeOutput("value", "unboxed value", FeatureType::number());
+        return true;
+    }
+    return false; // dependency error
 }
 
 FeatureExecutor &
 UnboxBlueprint::createExecutor(const IQueryEnvironment &, vespalib::Stash &stash) const
 {
-    return stash.create<UnboxExecutor>();
+    if (_was_object) {
+        return stash.create<UnboxExecutor>();
+    } else {
+        return stash.create<ForwardExecutor>();
+    }
 }
 
 } // namespace search::fef::test
diff --git a/searchlib/src/vespa/searchlib/fef/test/plugin/unbox.h b/searchlib/src/vespa/searchlib/fef/test/plugin/unbox.h
index 8e311d0a173..0ce6b07d830 100644
--- a/searchlib/src/vespa/searchlib/fef/test/plugin/unbox.h
+++ b/searchlib/src/vespa/searchlib/fef/test/plugin/unbox.h
@@ -8,6 +8,7 @@
 namespace search::fef::test {
 
 struct UnboxBlueprint : Blueprint {
+    bool _was_object;
     UnboxBlueprint();
     void visitDumpFeatures(const IIndexEnvironment &, IDumpFeatureVisitor &) const override;
     Blueprint::UP createInstance() const override;