handle errors in remote function server when get_throwOnError() is false #11083

kunigami · 2024-09-24T22:33:30Z

Differential Revision: D63346758

netlify · 2024-09-24T22:33:47Z

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	`187f903`
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/66ff08a54de7770009fa0389

facebook-github-bot · 2024-09-24T22:33:50Z

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

pedroerp · 2024-09-25T18:55:31Z

velox/functions/remote/client/Remote.cpp

+
+    if (remoteResponse.errors()) {
+      auto errors = *remoteResponse.errors();
+      for (auto entry : errors) {


const auto& entry

pedroerp · 2024-09-25T19:54:44Z

velox/functions/remote/server/RemoteFunctionService.cpp


  std::vector<VectorPtr> expressionResult;
  exprSet.eval(rows, evalCtx, expressionResult);

+  auto evalErrors = evalCtx.errors();
+  if (evalErrors != nullptr && evalErrors->hasError()) {


we chatted offline, but it would be nice to find a more efficient way to serialize these strings. Maybe writing them to a StringView vector then just serializing it?

facebook-github-bot · 2024-09-27T16:29:53Z

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

…lse (facebookincubator#11083) Summary: Pull Request resolved: facebookincubator#11083 Differential Revision: D63346758

facebook-github-bot · 2024-09-27T18:24:28Z

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

…lse (facebookincubator#11083) Summary: Pull Request resolved: facebookincubator#11083 Differential Revision: D63346758

facebook-github-bot · 2024-09-27T19:45:57Z

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

pedroerp

@kunigami awesome, thanks for making the changes! A few nits and cosmetic details, but overall looks pretty good

pedroerp · 2024-10-01T00:57:33Z

velox/functions/remote/client/Remote.cpp

+    if (auto errorPayload = remoteResponse.get_result().errorPayload()) {
+      auto errorsRowVector = IOBufToRowVector(
+          *errorPayload,
+          velox::ROW({velox::VARCHAR()}),


nit: in Velox code we usually omit the "velox" namespace, so for consistency here you can just use ROW({VARCHAR()})

pedroerp · 2024-10-01T00:58:56Z

velox/functions/remote/client/Remote.cpp

+          *context.pool(),
+          serde_.get());
+      auto errorsVector =
+          errorsRowVector->childAt(0)->asFlatVector<velox::StringView>();


can you VELOX_CHECK() here that the conversion above did not return nullptr?

pedroerp · 2024-10-01T01:02:33Z

velox/functions/remote/client/tests/RemoteFunctionTest.cpp

+  FOLLY_ALWAYS_INLINE void
+  call(TInput& result, const TInput& a, const TInput& b) {
+    if (a == 0) {
+      throw std::runtime_error("Failure");


In general we try to make these functions not throw but to return an error status instead. Could you also add a test case that if a function returns an error status it will do the right thing?

for reference:

https://github.com/facebookincubator/velox/blob/main/velox/functions/prestosql/Fail.h

pedroerp · 2024-10-01T01:05:36Z

velox/functions/remote/client/tests/RemoteFunctionTest.cpp

+  SelectivityVector defaultRows(data->size());
+
+  exprSet.eval(defaultRows, context, result);
+  // FIXME: why is that context has no errors here?


I'd guess that the "try" special form creates a temporary context to "swallow" the exceptions then discards them?

velox/functions/remote/client/tests/RemoteFunctionTest.cpp

pedroerp · 2024-10-01T01:07:57Z

velox/functions/remote/server/RemoteFunctionService.cpp

+  BufferPtr dataBuffer =
+      AlignedBuffer::allocate<velox::StringView>(numRows, pool_.get());
+
+  auto flatVector = std::make_shared<velox::FlatVector<velox::StringView>>(


ditto; omit the "velox" namespace

pedroerp · 2024-10-01T01:09:20Z

velox/functions/remote/server/RemoteFunctionService.cpp

+    if (evalErrors->hasErrorAt(i)) {
+      auto exceptionPtr = *evalErrors->errorAt(i);
+      try {
+        std::rethrow_exception(*exceptionPtr);


I'm not super fluent in exception_ptr, but isn't there really a way to capture the inner exception message without re-throwing it? :(

doesn't seem possible :/
https://stackoverflow.com/questions/41168218/what-can-i-do-with-an-stdexception-ptr-other-than-rethrowing-it

pedroerp · 2024-10-01T01:11:01Z

velox/functions/remote/server/RemoteFunctionService.h

@@ -35,6 +37,11 @@ class RemoteFunctionServiceHandler
      std::unique_ptr<remote::RemoteFunctionRequest> request) override;

 private:
+  void handleErrors(


please add a quick documentation header to describe what the function does (triple slashes "///")

pedroerp · 2024-10-01T01:11:48Z

velox/functions/remote/server/RemoteFunctionService.h

@@ -18,7 +18,9 @@

 #include <thrift/lib/cpp2/server/ThriftServer.h>
 #include "velox/common/memory/Memory.h"
+#include "velox/expression/EvalCtx.h"


could also forward declare to avoid the header->header dependency.

facebook-github-bot · 2024-10-01T21:16:08Z

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

…lse (facebookincubator#11083) Summary: Pull Request resolved: facebookincubator#11083 Differential Revision: D63346758

kunigami · 2024-10-01T21:19:57Z

I removed the custom FailFunction for throwing exceptions because we can use CheckedDivideFunction for that. Also importing FailFunction from prestosql/Fail.h which returns error code instead.

Also add test for the conjunctive conditional.

…lse (facebookincubator#11083) Summary: Pull Request resolved: facebookincubator#11083 Differential Revision: D63346758

facebook-github-bot · 2024-10-01T22:13:38Z

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

facebook-github-bot · 2024-10-01T22:20:49Z

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

…lse (facebookincubator#11083) Summary: Pull Request resolved: facebookincubator#11083 Differential Revision: D63346758

pedroerp

Awesome, thanks for adding this!

pedroerp · 2024-10-03T18:50:59Z

velox/functions/remote/client/tests/RemoteFunctionTest.cpp

@@ -161,6 +171,53 @@ TEST_P(RemoteFunctionTest, string) {
  assertEqualVectors(expected, results);
 }

+TEST_P(RemoteFunctionTest, try_exception) {


we usually use camel case for test names as well tryException

velox/functions/remote/client/tests/RemoteFunctionTest.cpp

pedroerp · 2024-10-03T18:53:48Z

velox/functions/remote/server/RemoteFunctionService.cpp

+  }
+  std::vector<VectorPtr> errorsVector{flatVector};
+  auto errorRowVector = std::make_shared<RowVector>(
+      pool_.get(), ROW({VARCHAR()}), BufferPtr(), numRows, errorsVector);


nit: you can use directly {errorsVector} to avoid line 99

…lse (facebookincubator#11083) Summary: Pull Request resolved: facebookincubator#11083 Reviewed By: pedroerp Differential Revision: D63346758

facebook-github-bot · 2024-10-03T19:21:17Z

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

…lse (facebookincubator#11083) Summary: Pull Request resolved: facebookincubator#11083 Reviewed By: pedroerp Differential Revision: D63346758

facebook-github-bot · 2024-10-03T21:12:32Z

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

facebook-github-bot · 2024-10-03T23:28:15Z

This pull request has been merged in a061eb1.

conbench-facebook · 2024-10-03T23:56:26Z

Conbench analyzed the 1 benchmark run on commit a061eb1e.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

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 Sep 24, 2024

facebook-github-bot added the fb-exported label Sep 24, 2024

pedroerp reviewed Sep 25, 2024

View reviewed changes

kunigami force-pushed the export-D63346758 branch from f222fe0 to 61fc63c Compare September 27, 2024 16:29

kunigami pushed a commit to kunigami/velox that referenced this pull request Sep 27, 2024

handle errors in remote function server when get_throwOnError() is fa…

61fc63c

…lse (facebookincubator#11083) Summary: Pull Request resolved: facebookincubator#11083 Differential Revision: D63346758

kunigami pushed a commit to kunigami/velox that referenced this pull request Sep 27, 2024

handle errors in remote function server when get_throwOnError() is fa…

2521704

…lse (facebookincubator#11083) Summary: Pull Request resolved: facebookincubator#11083 Differential Revision: D63346758

kunigami force-pushed the export-D63346758 branch from 61fc63c to 2521704 Compare September 27, 2024 18:24

kunigami pushed a commit to kunigami/velox that referenced this pull request Sep 27, 2024

handle errors in remote function server when get_throwOnError() is fa…

264d702

…lse (facebookincubator#11083) Summary: Pull Request resolved: facebookincubator#11083 Differential Revision: D63346758

kunigami force-pushed the export-D63346758 branch from 2521704 to 264d702 Compare September 27, 2024 19:24

pedroerp reviewed Oct 1, 2024

View reviewed changes

kunigami force-pushed the export-D63346758 branch from 264d702 to 4fa4d95 Compare October 1, 2024 21:16

kunigami pushed a commit to kunigami/velox that referenced this pull request Oct 1, 2024

handle errors in remote function server when get_throwOnError() is fa…

4fa4d95

…lse (facebookincubator#11083) Summary: Pull Request resolved: facebookincubator#11083 Differential Revision: D63346758

kunigami pushed a commit to kunigami/velox that referenced this pull request Oct 1, 2024

handle errors in remote function server when get_throwOnError() is fa…

707a97a

…lse (facebookincubator#11083) Summary: Pull Request resolved: facebookincubator#11083 Differential Revision: D63346758

kunigami force-pushed the export-D63346758 branch from 4fa4d95 to 707a97a Compare October 1, 2024 22:13

kunigami pushed a commit to kunigami/velox that referenced this pull request Oct 1, 2024

handle errors in remote function server when get_throwOnError() is fa…

cc63777

…lse (facebookincubator#11083) Summary: Pull Request resolved: facebookincubator#11083 Differential Revision: D63346758

kunigami force-pushed the export-D63346758 branch from 707a97a to cc63777 Compare October 1, 2024 22:20

pedroerp approved these changes Oct 3, 2024

View reviewed changes

kunigami force-pushed the export-D63346758 branch from cc63777 to 3c1c7e3 Compare October 3, 2024 19:20

handle errors in remote function server when get_throwOnError() is fa…

187f903

…lse (facebookincubator#11083) Summary: Pull Request resolved: facebookincubator#11083 Reviewed By: pedroerp Differential Revision: D63346758

kunigami force-pushed the export-D63346758 branch from 3c1c7e3 to 187f903 Compare October 3, 2024 21:12

facebook-github-bot closed this in a061eb1 Oct 3, 2024

facebook-github-bot added the Merged label Oct 3, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

handle errors in remote function server when get_throwOnError() is false #11083

handle errors in remote function server when get_throwOnError() is false #11083

kunigami commented Sep 24, 2024

netlify bot commented Sep 24, 2024 •

edited

Loading

facebook-github-bot commented Sep 24, 2024

pedroerp Sep 25, 2024

pedroerp Sep 25, 2024

facebook-github-bot commented Sep 27, 2024

facebook-github-bot commented Sep 27, 2024

facebook-github-bot commented Sep 27, 2024

pedroerp left a comment

pedroerp Oct 1, 2024

pedroerp Oct 1, 2024

pedroerp Oct 1, 2024

pedroerp Oct 1, 2024

pedroerp Oct 1, 2024

pedroerp Oct 1, 2024

pedroerp Oct 1, 2024

kunigami Oct 1, 2024

pedroerp Oct 1, 2024

pedroerp Oct 1, 2024

facebook-github-bot commented Oct 1, 2024

kunigami commented Oct 1, 2024

facebook-github-bot commented Oct 1, 2024

facebook-github-bot commented Oct 1, 2024

pedroerp left a comment

pedroerp Oct 3, 2024

pedroerp Oct 3, 2024

facebook-github-bot commented Oct 3, 2024

facebook-github-bot commented Oct 3, 2024

facebook-github-bot commented Oct 3, 2024

conbench-facebook bot commented Oct 3, 2024

handle errors in remote function server when get_throwOnError() is false #11083

handle errors in remote function server when get_throwOnError() is false #11083

Conversation

kunigami commented Sep 24, 2024

netlify bot commented Sep 24, 2024 • edited Loading

✅ Deploy Preview for meta-velox canceled.

facebook-github-bot commented Sep 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Sep 27, 2024

facebook-github-bot commented Sep 27, 2024

facebook-github-bot commented Sep 27, 2024

pedroerp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Oct 1, 2024

kunigami commented Oct 1, 2024

facebook-github-bot commented Oct 1, 2024

facebook-github-bot commented Oct 1, 2024

pedroerp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Oct 3, 2024

facebook-github-bot commented Oct 3, 2024

facebook-github-bot commented Oct 3, 2024

conbench-facebook bot commented Oct 3, 2024

netlify bot commented Sep 24, 2024 •

edited

Loading