refactor(native): Refactor exception translation to be extensible #26563
+251
−130
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
Reviewer's GuideThis PR replaces the static global translation map with an instance‐based, singleton translator that is populated via registerError in its constructor, introduces folly::Singleton and inline helper APIs for extensible exception translation, converts translate methods to non‐static instance methods, and updates all call sites and tests to use the new translator. Class diagram for extensible exception translationclassDiagram
class VeloxToPrestoExceptionTranslator {
+VeloxToPrestoExceptionTranslator()
+~VeloxToPrestoExceptionTranslator()
+translate(const velox::VeloxException& e) const : protocol::ExecutionFailureInfo
+translate(const std::exception& e) const : protocol::ExecutionFailureInfo
+testingErrorMap() const : ErrorCodeMap
#registerError(const std::string& errorSource, const std::string& errorCode, const protocol::ErrorCode& prestoErrorCode)
#errorMap_ : ErrorCodeMap
}
class protocol_ExecutionFailureInfo
class velox_VeloxException
class std_exception
VeloxToPrestoExceptionTranslator --> protocol_ExecutionFailureInfo : returns
VeloxToPrestoExceptionTranslator --> velox_VeloxException : uses
VeloxToPrestoExceptionTranslator --> std_exception : uses
class folly_Singleton_VeloxToPrestoExceptionTranslator {
}
folly_Singleton_VeloxToPrestoExceptionTranslator --> VeloxToPrestoExceptionTranslator : manages singleton instance
class translateToPrestoException {
+translateToPrestoException(const velox::VeloxException& e) : protocol::ExecutionFailureInfo
+translateToPrestoException(const std::exception& e) : protocol::ExecutionFailureInfo
}
translateToPrestoException --> folly_Singleton_VeloxToPrestoExceptionTranslator : gets instance
translateToPrestoException --> VeloxToPrestoExceptionTranslator : calls translate()
Flow diagram for exception translation API usageflowchart TD
A["Exception thrown (VeloxException or std::exception)"] --> B["translateToPrestoException(e)"]
B --> C["folly::Singleton<VeloxToPrestoExceptionTranslator>::try_get_fast()"]
C --> D["VeloxToPrestoExceptionTranslator::translate(e)"]
D --> E["protocol::ExecutionFailureInfo"]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `presto-native-execution/presto_cpp/main/common/tests/CommonTest.cpp:29-27` </location>
<code_context>
+ []() { return new facebook::presto::VeloxToPrestoExceptionTranslator(); });
+}
+
TEST(VeloxToPrestoExceptionTranslatorTest, exceptionTranslation) {
FLAGS_velox_exception_user_stacktrace_enabled = true;
for (const bool withContext : {false, true}) {
</code_context>
<issue_to_address>
**suggestion (testing):** Missing test for duplicate error registration in VeloxToPrestoExceptionTranslator.
Please add a test that registers a duplicate error code and checks that the expected exception or assertion is raised, confirming the singleton's registration logic works correctly.
Suggested implementation:
```cpp
TEST(VeloxToPrestoExceptionTranslatorTest, exceptionTranslation) {
FLAGS_velox_exception_user_stacktrace_enabled = true;
for (const bool withContext : {false, true}) {
EXPECT_EQ(e.errorSource(), error_source::kErrorSourceUser);
EXPECT_EQ(e.errorCode(), velox::error_code::kArithmeticError);
auto translator = folly::Singleton<
VeloxToPrestoExceptionTranslator>::try_get();
ASSERT_NE(translator, nullptr);
auto failureInfo = translateToPrestoException(e);
EXPECT_EQ(failureInfo.type, e.exceptionName());
EXPECT_EQ(failureInfo.errorLocation.lineNumber, e.line());
}
TEST(VeloxToPrestoExceptionTranslatorTest, duplicateErrorRegistration) {
auto* translator = folly::Singleton<VeloxToPrestoExceptionTranslator>::try_get();
ASSERT_NE(translator, nullptr);
// Register a custom error code.
const std::string errorCode = "DUPLICATE_ERROR";
const std::string exceptionName = "DuplicateException";
translator->registerErrorCode(errorCode, exceptionName);
// Attempt to register the same error code again and expect an exception/assertion.
EXPECT_THROW(
translator->registerErrorCode(errorCode, exceptionName),
std::runtime_error
);
}
```
- Ensure that `VeloxToPrestoExceptionTranslator::registerErrorCode` throws a `std::runtime_error` (or the appropriate exception type) when a duplicate error code is registered. If it currently asserts or throws a different exception, adjust the test and/or implementation accordingly.
- If the singleton is reset between tests, you may need to clear or reinitialize the error code registry before/after this test.
</issue_to_address>
### Comment 2
<location> `presto-native-execution/presto_cpp/main/common/tests/CommonTest.cpp:125-131` </location>
<code_context>
}
std::runtime_error stdRuntimeError("Test error message");
- auto failureInfo =
- VeloxToPrestoExceptionTranslator::translate((stdRuntimeError));
+ auto translator = folly::Singleton<
+ VeloxToPrestoExceptionTranslator>::try_get();
+ ASSERT_NE(translator, nullptr);
+ auto failureInfo = translateToPrestoException((stdRuntimeError));
EXPECT_EQ(failureInfo.type, "std::exception");
EXPECT_EQ(failureInfo.errorLocation.lineNumber, 1);
EXPECT_EQ(failureInfo.errorCode.name, "GENERIC_INTERNAL_ERROR");
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding tests for exception translation when stack trace is not available.
Please add a test where VeloxException lacks a stack trace and confirm the translated error either omits or properly handles the stack field.
```suggestion
auto translator = folly::Singleton<
VeloxToPrestoExceptionTranslator>::try_get();
ASSERT_NE(translator, nullptr);
auto failureInfo = translateToPrestoException(e);
EXPECT_EQ(failureInfo.type, e.exceptionName());
EXPECT_EQ(failureInfo.errorLocation.lineNumber, e.line());
EXPECT_EQ(failureInfo.errorCode.name, "GENERIC_USER_ERROR");
}
// Test translation of VeloxException without stack trace.
TEST(CommonTest, VeloxExceptionWithoutStackTrace) {
// Construct a VeloxException without a stack trace.
velox::VeloxException e(
"No stack trace error",
velox::error_code::kInvalidArgument,
error_source::kErrorSourceUser,
__FILE__,
123, // example line number
nullptr // No stack trace
);
auto translator = folly::Singleton<
VeloxToPrestoExceptionTranslator>::try_get();
ASSERT_NE(translator, nullptr);
auto failureInfo = translateToPrestoException(e);
EXPECT_EQ(failureInfo.type, e.exceptionName());
EXPECT_EQ(failureInfo.errorLocation.lineNumber, 123);
EXPECT_EQ(failureInfo.errorCode.name, "GENERIC_USER_ERROR");
// Check that stack field is either omitted, empty, or handled gracefully.
// This depends on the implementation of translateToPrestoException.
// For example, if stack is a string:
EXPECT_TRUE(failureInfo.stack.empty() || failureInfo.stack == "" || failureInfo.stack == std::nullopt);
}
```
</issue_to_address>
### Comment 3
<location> `presto-native-execution/presto_cpp/main/common/tests/CommonTest.cpp:31-27` </location>
<code_context>
+
TEST(VeloxToPrestoExceptionTranslatorTest, exceptionTranslation) {
FLAGS_velox_exception_user_stacktrace_enabled = true;
for (const bool withContext : {false, true}) {
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding tests for unknown error sources and codes.
Please add a test for unknown error sources or codes to confirm the fallback logic returns the expected generic error.
Suggested implementation:
```cpp
TEST(VeloxToPrestoExceptionTranslatorTest, exceptionTranslation) {
FLAGS_velox_exception_user_stacktrace_enabled = true;
for (const bool withContext : {false, true}) {
EXPECT_EQ(e.errorSource(), error_source::kErrorSourceUser);
EXPECT_EQ(e.errorCode(), velox::error_code::kArithmeticError);
auto translator = folly::Singleton<
VeloxToPrestoExceptionTranslator>::try_get();
ASSERT_NE(translator, nullptr);
auto failureInfo = translateToPrestoException(e);
EXPECT_EQ(failureInfo.type, e.exceptionName());
EXPECT_EQ(failureInfo.errorLocation.lineNumber, e.line());
}
}
// Test for unknown error source and code to confirm fallback logic.
TEST(VeloxToPrestoExceptionTranslatorTest, unknownErrorSourceAndCode) {
// Create a Velox exception with unknown error source and code.
velox::VeloxException unknownException(
"Unknown error",
velox::error_source::kErrorSourceUnknown, // Unknown error source
velox::error_code::kUnknown, // Unknown error code
__FILE__,
__LINE__);
auto translator = folly::Singleton<
VeloxToPrestoExceptionTranslator>::try_get();
ASSERT_NE(translator, nullptr);
auto failureInfo = translateToPrestoException(unknownException);
// Check that the fallback logic returns the expected generic error type/code.
EXPECT_EQ(failureInfo.type, "UNKNOWN"); // Or whatever the fallback type is
EXPECT_EQ(failureInfo.errorCode, "GENERIC_INTERNAL_ERROR"); // Or the expected fallback code
EXPECT_EQ(failureInfo.message, "Unknown error");
}
```
- You may need to adjust the expected values for `failureInfo.type` and `failureInfo.errorCode` to match your actual fallback logic. Replace "UNKNOWN" and "GENERIC_INTERNAL_ERROR" with the correct values if your implementation uses different strings.
- If your `translateToPrestoException` function or `VeloxException` constructor requires additional parameters, please add them accordingly.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| folly::Singleton<facebook::presto::VeloxToPrestoExceptionTranslator> | ||
| exceptionTranslatorSingleton( | ||
| []() { return new facebook::presto::VeloxToPrestoExceptionTranslator(); }); | ||
| } |
Contributor
There was a problem hiding this comment.
suggestion (testing): Missing test for duplicate error registration in VeloxToPrestoExceptionTranslator.
Please add a test that registers a duplicate error code and checks that the expected exception or assertion is raised, confirming the singleton's registration logic works correctly.
Suggested implementation:
TEST(VeloxToPrestoExceptionTranslatorTest, exceptionTranslation) {
FLAGS_velox_exception_user_stacktrace_enabled = true;
for (const bool withContext : {false, true}) {
EXPECT_EQ(e.errorSource(), error_source::kErrorSourceUser);
EXPECT_EQ(e.errorCode(), velox::error_code::kArithmeticError);
auto translator = folly::Singleton<
VeloxToPrestoExceptionTranslator>::try_get();
ASSERT_NE(translator, nullptr);
auto failureInfo = translateToPrestoException(e);
EXPECT_EQ(failureInfo.type, e.exceptionName());
EXPECT_EQ(failureInfo.errorLocation.lineNumber, e.line());
}
TEST(VeloxToPrestoExceptionTranslatorTest, duplicateErrorRegistration) {
auto* translator = folly::Singleton<VeloxToPrestoExceptionTranslator>::try_get();
ASSERT_NE(translator, nullptr);
// Register a custom error code.
const std::string errorCode = "DUPLICATE_ERROR";
const std::string exceptionName = "DuplicateException";
translator->registerErrorCode(errorCode, exceptionName);
// Attempt to register the same error code again and expect an exception/assertion.
EXPECT_THROW(
translator->registerErrorCode(errorCode, exceptionName),
std::runtime_error
);
}
- Ensure that
VeloxToPrestoExceptionTranslator::registerErrorCodethrows astd::runtime_error(or the appropriate exception type) when a duplicate error code is registered. If it currently asserts or throws a different exception, adjust the test and/or implementation accordingly. - If the singleton is reset between tests, you may need to clear or reinitialize the error code registry before/after this test.
xiaoxmeng
previously approved these changes
Nov 7, 2025
Contributor
Author
|
@majetideepak I'm not sure if this change might affect your use case. Could you take a look to make sure nothing breaks from your side? Thanks! |
tanjialiang
changed the title
…6563) Summary: Currently the velox exception translation is through global static methods, not extensible. Use cases arises when we want additional error types exposed from not within presto/velox stack. This change makes the translation layer a global singleton, similar to BaseStatsReporter in velox, creating flexibility to extend and customize. Reviewed By: xiaoxmeng Differential Revision: D86562978
tanjialiang
force-pushed
the
export-D86562978
branch
from
a8e1136 to
b14aa36
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary: Currently the velox exception translation is through global static methods, not extensible. Use cases arises when we want additional error types exposed from not within presto/velox stack. This change makes the translation layer a global singleton, similar to BaseStatsReporter in velox, creating flexibility to extend and customize.
Differential Revision: D86562978