From 577708c2d05c448b753db107a9582ef474dc9876 Mon Sep 17 00:00:00 2001
From: Masha Basmanova <mbasmanova@fb.com>
Date: Tue, 14 Sep 2021 10:30:45 -0700
Subject: [PATCH] Add between operator for strings (#207)

Summary: Pull Request resolved: https://github.com/facebookincubator/velox/pull/207

Reviewed By: kgpai

Differential Revision: D30933071

Pulled By: mbasmanova

fbshipit-source-id: 06b17a7098f5a11ec6b2154650bf58aada5b91e3
---
 velox/duckdb/conversion/DuckParser.cpp        | 13 ++++--------
 .../conversion/tests/DuckParserTest.cpp       |  6 ++----
 velox/expression/ExprCompiler.cpp             |  4 +++-
 .../prestosql/RegisterComparisons.cpp         |  1 +
 .../prestosql/tests/ComparisonsTest.cpp       | 21 +++++++++++++++++++
 5 files changed, 31 insertions(+), 14 deletions(-)

diff --git a/velox/duckdb/conversion/DuckParser.cpp b/velox/duckdb/conversion/DuckParser.cpp
index 12b5f01ec5c0..771c53c44234 100644
--- a/velox/duckdb/conversion/DuckParser.cpp
+++ b/velox/duckdb/conversion/DuckParser.cpp
@@ -156,15 +156,10 @@ std::shared_ptr<const core::IExpr> parseComparisonExpr(ParsedExpression& expr) {
 std::shared_ptr<const core::IExpr> parseBetweenExpr(ParsedExpression& expr) {
   const auto& betweenExpr = dynamic_cast<BetweenExpression&>(expr);
   return callExpr(
-      "and",
-      {
-          callExpr(
-              "gte",
-              {parseExpr(*betweenExpr.input), parseExpr(*betweenExpr.lower)}),
-          callExpr(
-              "lte",
-              {parseExpr(*betweenExpr.input), parseExpr(*betweenExpr.upper)}),
-      });
+      "between",
+      {parseExpr(*betweenExpr.input),
+       parseExpr(*betweenExpr.lower),
+       parseExpr(*betweenExpr.upper)});
 }
 
 // Parse a conjunction (AND or OR).
diff --git a/velox/duckdb/conversion/tests/DuckParserTest.cpp b/velox/duckdb/conversion/tests/DuckParserTest.cpp
index 645000dae478..381233a7da17 100644
--- a/velox/duckdb/conversion/tests/DuckParserTest.cpp
+++ b/velox/duckdb/conversion/tests/DuckParserTest.cpp
@@ -144,12 +144,10 @@ TEST(DuckParserTest, expressions) {
 }
 
 TEST(DuckParserTest, between) {
-  EXPECT_EQ(
-      "and(gte(\"c0\",0),lte(\"c0\",1))",
-      parseExpr("c0 between 0 and 1")->toString());
+  EXPECT_EQ("between(\"c0\",0,1)", parseExpr("c0 between 0 and 1")->toString());
 
   EXPECT_EQ(
-      "and(and(gte(\"c0\",0),lte(\"c0\",1)),gt(\"c0\",10))",
+      "and(between(\"c0\",0,1),gt(\"c0\",10))",
       parseExpr("c0 between 0 and 1 and c0 > 10")->toString());
 }
 
diff --git a/velox/expression/ExprCompiler.cpp b/velox/expression/ExprCompiler.cpp
index de44bcb622e7..bd54c992da39 100644
--- a/velox/expression/ExprCompiler.cpp
+++ b/velox/expression/ExprCompiler.cpp
@@ -348,7 +348,9 @@ ExprPtr compileExpression(
       result = std::make_shared<Expr>(
           resultType, std::move(compiledInputs), func, call->name());
     } else {
-      VELOX_FAIL("Function not registered: {}", call->name());
+      VELOX_FAIL(
+          "Function not registered: {}",
+          core::FunctionKey(call->name(), inputTypes).toString());
     }
   } else if (
       auto access =
diff --git a/velox/functions/prestosql/RegisterComparisons.cpp b/velox/functions/prestosql/RegisterComparisons.cpp
index e74c0548b20b..1c7b527ab872 100644
--- a/velox/functions/prestosql/RegisterComparisons.cpp
+++ b/velox/functions/prestosql/RegisterComparisons.cpp
@@ -34,6 +34,7 @@ void registerComparisonFunctions() {
   registerFunction<udf_between<int64_t>, bool, int64_t, int64_t, int64_t>();
   registerFunction<udf_between<double>, bool, double, double, double>();
   registerFunction<udf_between<float>, bool, float, float, float>();
+  registerFunction<udf_between<StringView>, bool, Varchar, Varchar, Varchar>();
 }
 
 } // namespace facebook::velox::functions
diff --git a/velox/functions/prestosql/tests/ComparisonsTest.cpp b/velox/functions/prestosql/tests/ComparisonsTest.cpp
index e3b89430dd5e..576c3028eb75 100644
--- a/velox/functions/prestosql/tests/ComparisonsTest.cpp
+++ b/velox/functions/prestosql/tests/ComparisonsTest.cpp
@@ -31,3 +31,24 @@ TEST_F(ComparisonsTest, between) {
     EXPECT_EQ(result->valueAt(i), std::get<1>(testData[i])) << "at " << i;
   }
 }
+
+TEST_F(ComparisonsTest, betweenVarchar) {
+  using S = StringView;
+
+  const auto between = [&](std::optional<std::string> s) {
+    auto expr = "c0 between 'mango' and 'pear'";
+    if (s.has_value()) {
+      return evaluateOnce<bool>(expr, std::optional(S(s.value())));
+    } else {
+      return evaluateOnce<bool>(expr, std::optional<S>());
+    }
+  };
+
+  EXPECT_EQ(std::nullopt, between(std::nullopt));
+  EXPECT_EQ(false, between(""));
+  EXPECT_EQ(false, between("apple"));
+  EXPECT_EQ(false, between("pineapple"));
+  EXPECT_EQ(true, between("mango"));
+  EXPECT_EQ(true, between("orange"));
+  EXPECT_EQ(true, between("pear"));
+}