Introduce velox::Expected #9858

mbasmanova · 2024-05-17T16:44:40Z

Summary: velox::Expected<T> can be used by no-throw APIs to return result or error.

Differential Revision: D57496990

facebook-github-bot · 2024-05-17T16:44:50Z

This pull request was exported from Phabricator. Differential Revision: D57496990

netlify · 2024-05-17T16:44:58Z

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	`d26063f`
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/664798b699c6d5000874e067

Summary: `velox::Expected<T>` can be used by no-throw APIs to return result or error. Differential Revision: D57496990

facebook-github-bot · 2024-05-17T16:46:13Z

This pull request was exported from Phabricator. Differential Revision: D57496990

xiaoxmeng

LGTM. Thanks!

pedroerp · 2024-05-17T17:34:33Z

velox/common/base/Status.h

@@ -489,6 +490,10 @@ inline Status genericToStatus(Status&& st) {
 }

 } // namespace internal
+
+template <typename T>


maybe add a comment block here explaining how we intend this to be used with a few examples?

Updated. Thanks.

pedroerp · 2024-05-17T17:36:06Z

velox/common/base/tests/StatusTest.cpp

@@ -128,5 +128,23 @@ TEST(StatusTest, macros) {
  ASSERT_TRUE(didThrow) << "VELOX_CHECK_OK did not throw";
 }

+Expected<int> modulo(int a, int b) {
+  if (b == 0) {
+    return folly::makeUnexpected(Status::UserError("division by zero"));


the only annoying part with folly::Expected is that it doesn't let you to simply:

return Status::UserError("division by zero");

like you can with arrow::Result. We could try to work around it, but it doesn't let you do it by default (I'm not exactly sure why)

That's a good concern, otherwise the macros won't work with Expected

pedroerp · 2024-05-17T17:38:22Z

velox/common/base/tests/StatusTest.cpp

@@ -128,5 +128,23 @@ TEST(StatusTest, macros) {
  ASSERT_TRUE(didThrow) << "VELOX_CHECK_OK did not throw";
 }

+Expected<int> modulo(int a, int b) {
+  if (b == 0) {
+    return folly::makeUnexpected(Status::UserError("division by zero"));


we could also provide macros like other project do, such as:

VELOX_CHECK(b == 0, "division by zero");

or since we already have VELOX_CHECK, maybe VELOX_RETURN_IF?

Semantically, we would probably need a set of macros, that throw (the VELOX_CHECK ones we have), and ones that just return an status. It would be a pain, but we could potentially rename VELOX_CHECK to VELOX_THROW (to make it more explicitly that it actually throws), and have VELOX_CHECK (and variants) to just return an error status.

Summary: `velox::Expected<T>` can be used by no-throw APIs to return result or error. Reviewed By: xiaoxmeng, Yuhta, pedroerp Differential Revision: D57496990

facebook-github-bot · 2024-05-17T17:48:15Z

This pull request was exported from Phabricator. Differential Revision: D57496990

Summary: `velox::Expected<T>` can be used by no-throw APIs to return result or error. Reviewed By: xiaoxmeng, Yuhta, pedroerp Differential Revision: D57496990

facebook-github-bot · 2024-05-17T17:49:54Z

This pull request was exported from Phabricator. Differential Revision: D57496990

Summary: `velox::Expected<T>` can be used by no-throw APIs to return result or error. Reviewed By: xiaoxmeng, Yuhta, Magoja, pedroerp Differential Revision: D57496990

facebook-github-bot · 2024-05-22T13:09:34Z

This pull request has been merged in fbb3578.

Summary: Pull Request resolved: facebookincubator#9858 `velox::Expected<T>` can be used by no-throw APIs to return result or error. Reviewed By: xiaoxmeng, Yuhta, Magoja, pedroerp Differential Revision: D57496990 fbshipit-source-id: 9f820d293e9b990633183e8daee73f6b12c2cb5e

Summary: We have recently introduced velox::Expected (#9858) to use in non-void no-throw APIs. It's more proper to return that. Pull Request resolved: #10997 Reviewed By: xiaoxmeng Differential Revision: D63035456 Pulled By: Yuhta fbshipit-source-id: 6c8322593f9677e1dfa70f719b2922429f418d57

Summary: We have recently introduced velox::Expected (#9858) to use in non-void no-throw APIs. It's more proper to return that. Pull Request resolved: #11246 Reviewed By: bikramSingh91 Differential Revision: D64335564 Pulled By: kgpai fbshipit-source-id: 92be9d34c739577ece1c1b0a0df497045348692a

…ator#10997) Summary: We have recently introduced velox::Expected (facebookincubator#9858) to use in non-void no-throw APIs. It's more proper to return that. Pull Request resolved: facebookincubator#10997 Reviewed By: xiaoxmeng Differential Revision: D63035456 Pulled By: Yuhta fbshipit-source-id: 6c8322593f9677e1dfa70f719b2922429f418d57

…cubator#11246) Summary: We have recently introduced velox::Expected (facebookincubator#9858) to use in non-void no-throw APIs. It's more proper to return that. Pull Request resolved: facebookincubator#11246 Reviewed By: bikramSingh91 Differential Revision: D64335564 Pulled By: kgpai fbshipit-source-id: 92be9d34c739577ece1c1b0a0df497045348692a

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label May 17, 2024

facebook-github-bot added the fb-exported label May 17, 2024

mbasmanova requested review from pedroerp and Yuhta May 17, 2024 16:45

mbasmanova added a commit to mbasmanova/velox-1 that referenced this pull request May 17, 2024

Introduce velox::Expected (facebookincubator#9858)

9daa666

Summary: `velox::Expected<T>` can be used by no-throw APIs to return result or error. Differential Revision: D57496990

mbasmanova force-pushed the export-D57496990 branch from c38878d to 9daa666 Compare May 17, 2024 16:46

xiaoxmeng approved these changes May 17, 2024

View reviewed changes

Yuhta approved these changes May 17, 2024

View reviewed changes

pedroerp approved these changes May 17, 2024

View reviewed changes

pedroerp reviewed May 17, 2024

View reviewed changes

mbasmanova force-pushed the export-D57496990 branch from 9daa666 to bbfa46a Compare May 17, 2024 17:48

Introduce velox::Expected (facebookincubator#9858)

d26063f

Summary: `velox::Expected<T>` can be used by no-throw APIs to return result or error. Reviewed By: xiaoxmeng, Yuhta, pedroerp Differential Revision: D57496990

mbasmanova force-pushed the export-D57496990 branch from bbfa46a to d26063f Compare May 17, 2024 17:49

facebook-github-bot closed this in fbb3578 May 22, 2024

facebook-github-bot added the Merged label May 22, 2024

mbasmanova mentioned this pull request May 23, 2024

Refactor DateTimeFormatter::parse to return velox::Expected #9915

Closed

NEUpanning mentioned this pull request Sep 14, 2024

Refactor TimestampConversion to return velox::Expected #10997

Closed

NEUpanning mentioned this pull request Oct 12, 2024

Refactor buildDateTimeFormatter to return velox::Expected #11246

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Introduce velox::Expected #9858

Introduce velox::Expected #9858

Uh oh!

mbasmanova commented May 17, 2024

Uh oh!

facebook-github-bot commented May 17, 2024

Uh oh!

netlify bot commented May 17, 2024 •

edited

Loading

Uh oh!

facebook-github-bot commented May 17, 2024

Uh oh!

xiaoxmeng left a comment

Uh oh!

pedroerp May 17, 2024

Uh oh!

mbasmanova May 17, 2024

Uh oh!

pedroerp May 17, 2024

Uh oh!

Yuhta May 17, 2024 •

edited

Loading

Uh oh!

pedroerp May 17, 2024 •

edited

Loading

Uh oh!

facebook-github-bot commented May 17, 2024

Uh oh!

facebook-github-bot commented May 17, 2024

Uh oh!

facebook-github-bot commented May 22, 2024

Uh oh!

Uh oh!

Introduce velox::Expected #9858

Introduce velox::Expected #9858

Uh oh!

Conversation

mbasmanova commented May 17, 2024

Uh oh!

facebook-github-bot commented May 17, 2024

Uh oh!

netlify bot commented May 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for meta-velox canceled.

Uh oh!

facebook-github-bot commented May 17, 2024

Uh oh!

xiaoxmeng left a comment

Choose a reason for hiding this comment

Uh oh!

pedroerp May 17, 2024

Choose a reason for hiding this comment

Uh oh!

mbasmanova May 17, 2024

Choose a reason for hiding this comment

Uh oh!

pedroerp May 17, 2024

Choose a reason for hiding this comment

Uh oh!

Yuhta May 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pedroerp May 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented May 17, 2024

Uh oh!

facebook-github-bot commented May 17, 2024

Uh oh!

facebook-github-bot commented May 22, 2024

Uh oh!

Uh oh!

netlify bot commented May 17, 2024 •

edited

Loading

Yuhta May 17, 2024 •

edited

Loading

pedroerp May 17, 2024 •

edited

Loading