Add between operator for strings (#207)

Summary: Pull Request resolved: #207 Reviewed By: kgpai Differential Revision: D30933071 Pulled By: mbasmanova fbshipit-source-id: 06b17a7098f5a11ec6b2154650bf58aada5b91e3
facebookincubator · Sep 14, 2021 · 577708c · 577708c
1 parent 6d9ca6c
commit 577708c
Show file tree

Hide file tree

Showing 5 changed files with 31 additions and 14 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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"));
+}