feat: Allow propagating constant folding exceptions (facebookincubator#13680)

Summary:
Pull Request resolved: facebookincubator#13680

Enhance velox::exec::tryEvaluateConstantExpression to allow for propagating exceptions occurred during evaluation of a constant expression. Specifically, constant folding 5 / 0 fails with "division by zero". Before this change, the error would be swallowed. After this change the error would be propagated unless optional parameter suppressEvaluationFailures is set to true.

Remove velox::exec::evaluateConstantExpression as it is no longer used / needed.

Reviewed By: bikramSingh91

Differential Revision: D76152526

fbshipit-source-id: 190abf1c75f8034e58c70fa665abaaca4ba5a189

Author: mbasmanova

velox/expression/Expr.cpp

@@ -1683,11 +1683,7 @@ void Expr::appendInputsSql(
 }
 
 bool Expr::isConstant() const {
-  if (!isDeterministic()) {
-    return false;
-  }
-
-  return distinctFields_.empty();
+  return isDeterministic() && distinctFields_.empty();
 }
 
 namespace {
@@ -2041,25 +2037,25 @@ core::ExecCtx* SimpleExpressionEvaluator::ensureExecCtx() {
   return execCtx_.get();
 }
 
-VectorPtr evaluateConstantExpression(
-    const core::TypedExprPtr& expr,
-    memory::MemoryPool* pool) {
-  auto result = tryEvaluateConstantExpression(expr, pool);
-  VELOX_USER_CHECK_NOT_NULL(
-      result, "Expression is not constant-foldable: {}", expr->toString());
-  return result;
-}
-
 VectorPtr tryEvaluateConstantExpression(
     const core::TypedExprPtr& expr,
-    memory::MemoryPool* pool) {
-  auto data = BaseVector::create<RowVector>(ROW({}), 1, pool);
-
+    memory::MemoryPool* pool,
+    bool suppressEvaluationFailures) {
   auto queryCtx = velox::core::QueryCtx::create();
   velox::core::ExecCtx execCtx{pool, queryCtx.get()};
   velox::exec::ExprSet exprSet({expr}, &execCtx);
 
-  if (exprSet.expr(0)->is<ConstantExpr>()) {
+  // The construction of ExprSet involves compiling and constant folding the
+  // expression. If constant folding succeeded, then we get a ConstantExpr.
+  // Constant folding may fail because expression is not constant-foldable or if
+  // an error happened during evaluation (5 / 0 fails with "division by zero").
+  // If constant folding didn't succeed, but suppressEvaluationFailures is
+  // false, we need to re-evaluate the expression to propagate the failure.
+  const bool doEvaluate = exprSet.expr(0)->is<ConstantExpr>() ||
+      (!suppressEvaluationFailures && exprSet.expr(0)->isConstant());
+
+  if (doEvaluate) {
+    auto data = BaseVector::create<RowVector>(ROW({}), 1, pool);
     velox::exec::EvalCtx evalCtx(&execCtx, &exprSet, data.get());
     velox::SelectivityVector singleRow(1);
     std::vector<velox::VectorPtr> results(1);

velox/expression/Expr.h

@@ -805,19 +805,18 @@ std::unique_ptr<ExprSet> makeExprSetFromFlag(
     std::vector<core::TypedExprPtr>&& source,
     core::ExecCtx* execCtx);
 
-/// Evaluates a deterministic expression that doesn't depend on any inputs and
-/// returns the result as single-row vector. Throws if expression is
-/// non-deterministic or has dependencies.
-VectorPtr evaluateConstantExpression(
-    const core::TypedExprPtr& expr,
-    memory::MemoryPool* pool);
-
 /// Evaluates a deterministic expression that doesn't depend on any inputs and
 /// returns the result as single-row vector. Returns nullptr if the expression
 /// is non-deterministic or has dependencies.
+///
+/// By default, propagates failures that occur during evaluation of the
+/// expression. For example, evaluating 5 / 0 throws "division by zero". If
+/// 'suppressEvaluationFailures' is true, these failures are swallowed and the
+/// caller receives a nullptr result.
 VectorPtr tryEvaluateConstantExpression(
     const core::TypedExprPtr& expr,
-    memory::MemoryPool* pool);
+    memory::MemoryPool* pool,
+    bool suppressEvaluationFailures = false);
 
 /// Returns a string representation of the expression trees annotated with
 /// runtime statistics. Expected to be called after calling ExprSet::eval one or

velox/expression/tests/ExprTest.cpp

@@ -4987,7 +4987,13 @@ TEST_F(ExprTest, disabledeferredLazyLoading) {
 TEST_F(ExprTest, evaluateConstantExpression) {
   auto eval = [&](const std::string& sql) {
     auto expr = parseExpression(sql, ROW({"a"}, {BIGINT()}));
-    return exec::evaluateConstantExpression(expr, pool());
+    return exec::tryEvaluateConstantExpression(expr, pool());
+  };
+
+  auto evalNoThrow = [&](const std::string& sql) {
+    auto expr = parseExpression(sql, ROW({"a"}, {BIGINT()}));
+    return exec::tryEvaluateConstantExpression(
+        expr, pool(), true /* supressEvaluationFailures */);
   };
 
   assertEqualVectors(eval("1 + 2"), makeConstant<int64_t>(3, 1));
@@ -5009,28 +5015,23 @@ TEST_F(ExprTest, evaluateConstantExpression) {
           "try(coalesce(array_min_by(array[1, 2, 3], x -> x / 0), 0::INTEGER))"),
       makeNullConstant(TypeKind::INTEGER, 1));
 
-  auto tryEval = [&](const std::string& sql) {
-    auto expr = parseExpression(sql, ROW({"a"}, {BIGINT()}));
-    return exec::tryEvaluateConstantExpression(expr, pool());
-  };
+  EXPECT_TRUE(eval("a + 1") == nullptr);
 
-  VELOX_ASSERT_THROW(eval("a + 1"), "Expression is not constant-foldable");
-  ASSERT_TRUE(tryEval("a + 1") == nullptr);
+  EXPECT_TRUE(eval("rand() + 1.0") == nullptr);
 
-  VELOX_ASSERT_THROW(
-      eval("rand() + 1.0"), "Expression is not constant-foldable");
-  ASSERT_TRUE(tryEval("rand() + 1.0") == nullptr);
+  EXPECT_TRUE(eval("transform(array[1, 2, 3], x -> (x * 2) + a)") == nullptr);
 
-  VELOX_ASSERT_THROW(
-      eval("transform(array[1, 2, 3], x -> (x * 2) + a)"),
-      "Expression is not constant-foldable");
-  ASSERT_TRUE(
-      tryEval("transform(array[1, 2, 3], x -> (x * 2) + a)") == nullptr);
+  EXPECT_TRUE(eval("transform(array[1, 2, 3], x -> x + rand())") == nullptr);
+
+  VELOX_ASSERT_THROW(eval("5 / 0"), "division by zero");
+  EXPECT_TRUE(evalNoThrow("5 / 0") == nullptr);
+
+  VELOX_ASSERT_THROW(eval("1 + 5 / 0"), "division by zero");
+  EXPECT_TRUE(evalNoThrow("1 + 5 / 0") == nullptr);
 
   VELOX_ASSERT_THROW(
-      eval("transform(array[1, 2, 3], x -> x + rand())"),
-      "Expression is not constant-foldable");
-  ASSERT_TRUE(tryEval("transform(array[1, 2, 3], x -> x + rand())") == nullptr);
+      eval("transform(array[1, 2, 3], x -> x / 0)"), "division by zero");
+  EXPECT_TRUE(evalNoThrow("transform(array[1, 2, 3], x -> x / 0)") == nullptr);
 }
 
 TEST_F(ExprTest, isDeterministic) {