From 86efb6343e952c90600fd202e6b623e41cd2e050 Mon Sep 17 00:00:00 2001
From: Wei He <weihe@meta.com>
Date: Tue, 16 Jul 2024 20:44:04 -0700
Subject: [PATCH] Fix try_cast to throw VeloxRuntimeError (#10483)

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

Before the fix, try_cast suppress all errors. But VeloxRuntimeError should not
be suppressed. This diff makes try_cast to still throw VeloxRuntimeError.

Reviewed By: kgpai

Differential Revision: D59838302

fbshipit-source-id: 84ab35d0e27c3c67877c5c206cac8a84732ec046
---
 velox/expression/CastExpr-inl.h         |  7 ++++++-
 velox/expression/tests/CastExprTest.cpp | 27 +++++++++++++++++++++++++
 2 files changed, 33 insertions(+), 1 deletion(-)

diff --git a/velox/expression/CastExpr-inl.h b/velox/expression/CastExpr-inl.h
index 0bb12fece092..d920918f3a2b 100644
--- a/velox/expression/CastExpr-inl.h
+++ b/velox/expression/CastExpr-inl.h
@@ -268,7 +268,12 @@ void CastExpr::applyToSelectedNoThrowLocal(
     rows.template applyToSelected([&](auto row) INLINE_LAMBDA {
       try {
         func(row);
-      } catch (...) {
+      } catch (const VeloxException& e) {
+        if (!e.isUserError()) {
+          throw;
+        }
+        result->setNull(row, true);
+      } catch (const std::exception&) {
         result->setNull(row, true);
       }
     });
diff --git a/velox/expression/tests/CastExprTest.cpp b/velox/expression/tests/CastExprTest.cpp
index bd83ce550f66..4f35490847c1 100644
--- a/velox/expression/tests/CastExprTest.cpp
+++ b/velox/expression/tests/CastExprTest.cpp
@@ -707,6 +707,15 @@ TEST_F(CastExprTest, timestampToString) {
   };
   VELOX_ASSERT_THROW(
       mustThrow(), "Unable to convert timezone 'America/Los_Angeles' past");
+
+  // try_cast should also throw since it's runtime error.
+  auto tryCastMustThrow = [&]() {
+    return testTryCast<Timestamp, std::string>(
+        "string", {Timestamp(253405036800, 0)}, {"10000-02-01 08:00:00.000"});
+  };
+  VELOX_ASSERT_THROW(
+      tryCastMustThrow(),
+      "Unable to convert timezone 'America/Los_Angeles' past");
 }
 
 TEST_F(CastExprTest, dateToTimestamp) {
@@ -760,6 +769,24 @@ TEST_F(CastExprTest, timestampToDate) {
       },
       TIMESTAMP(),
       DATE());
+
+  // Ensure external/date throws since it doesn't know how to convert large
+  // timestamps.
+  auto mustThrow = [&]() {
+    return testCast<Timestamp, int32_t>(
+        "date", {Timestamp(253405036800, 0)}, {0});
+  };
+  VELOX_ASSERT_THROW(
+      mustThrow(), "Unable to convert timezone 'America/Los_Angeles' past");
+
+  // try_cast should also throw since it's runtime error.
+  auto tryCastMustThrow = [&]() {
+    return testTryCast<Timestamp, int32_t>(
+        "date", {Timestamp(253405036800, 0)}, {0});
+  };
+  VELOX_ASSERT_THROW(
+      tryCastMustThrow(),
+      "Unable to convert timezone 'America/Los_Angeles' past");
 }
 
 TEST_F(CastExprTest, timestampInvalid) {