feat: Add catchableByTry flag to ErrorCode for extensible TRY error handling#26949
Merged
feilong-liu merged 1 commit intoprestodb:masterfrom Jan 16, 2026
Merged
Conversation
Contributor
Reviewer's GuideIntroduces a new catchableByTry flag on ErrorCode (including JSON/Thrift serialization) and wires it through StandardErrorCode, TryFunction, and RestErrorCode so TRY() catch behavior is driven by an error-code property instead of hardcoded checks, while marking the relevant standard and REST errors as TRY‑catchable. Sequence diagram for TRY function using catchableByTry-based error handlingsequenceDiagram
actor User
participant QueryEngine
participant TryFunction
participant Supplier as UserExpressionSupplier
participant RemoteFunction
participant RestNamespaceManager
participant PrestoException
participant ErrorCode
User->>QueryEngine: Submit query with TRY(remote_function_call(...))
QueryEngine->>TryFunction: evaluate(supplier, defaultValue)
TryFunction->>Supplier: get()
Supplier->>RemoteFunction: invoke()
RemoteFunction->>RestNamespaceManager: HTTP request
RestNamespaceManager-->>RemoteFunction: REST_SERVER_ERROR (ErrorCode catchableByTry = true)
RemoteFunction-->>Supplier: throws PrestoException
Supplier-->>TryFunction: throws PrestoException
TryFunction->>PrestoException: catch
TryFunction->>ErrorCode: getErrorCode()
TryFunction->>ErrorCode: isCatchableByTry()
alt error is catchableByTry == true
TryFunction-->>QueryEngine: return defaultValue (TRY swallows error)
QueryEngine-->>User: query continues, TRY result = defaultValue
else error is catchableByTry == false
TryFunction-->>QueryEngine: rethrow PrestoException
QueryEngine-->>User: query fails with error
end
Class diagram for updated error handling with catchableByTry flagclassDiagram
class ErrorCode {
-int code
-String name
-ErrorType type
-boolean retriable
-boolean catchableByTry
+ErrorCode(int code, String name, ErrorType type, boolean retriable, boolean catchableByTry)
+ErrorCode(int code, String name, ErrorType type)
+ErrorCode(int code, String name, ErrorType type, boolean retriable)
+int getCode()
+String getName()
+ErrorType getType()
+boolean isRetriable()
+boolean isCatchableByTry()
+String toString()
}
class StandardErrorCode {
<<enum>>
-ErrorCode errorCode
+StandardErrorCode(int code, ErrorType type)
+StandardErrorCode(int code, ErrorType type, boolean retriable)
+StandardErrorCode(int code, ErrorType type, boolean retriable, boolean catchableByTry)
+ErrorCode toErrorCode()
}
class RestErrorCode {
<<enum>>
-ErrorCode errorCode
+RestErrorCode(int code, ErrorType type)
+ErrorCode toErrorCode()
}
class TryFunction {
+static <T> T evaluate(Supplier~T~ supplier, T defaultValue)
-static void propagateIfUnhandled(PrestoException e)
}
class PrestoException {
+ErrorCode getErrorCode()
}
class ErrorType
StandardErrorCode --> ErrorCode : wraps
RestErrorCode --> ErrorCode : wraps
PrestoException --> ErrorCode : uses
TryFunction --> PrestoException : catches
TryFunction --> ErrorCode : checks isCatchableByTry
%% Highlight specific enum constants that now set catchableByTry
class StandardErrorCode_InvalidFunctionArgument as StandardErrorCode_INVALID_FUNCTION_ARGUMENT {
}
class StandardErrorCode_DivisionByZero as StandardErrorCode_DIVISION_BY_ZERO {
}
class StandardErrorCode_InvalidCastArgument as StandardErrorCode_INVALID_CAST_ARGUMENT {
}
class StandardErrorCode_NumericValueOutOfRange as StandardErrorCode_NUMERIC_VALUE_OUT_OF_RANGE {
}
StandardErrorCode_INVALID_FUNCTION_ARGUMENT ..> ErrorCode : catchableByTry = true
StandardErrorCode_DIVISION_BY_ZERO ..> ErrorCode : catchableByTry = true
StandardErrorCode_INVALID_CAST_ARGUMENT ..> ErrorCode : catchableByTry = true
StandardErrorCode_NUMERIC_VALUE_OUT_OF_RANGE ..> ErrorCode : catchableByTry = true
RestErrorCode ..> ErrorCode : catchableByTry = true
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `presto-main-base/src/main/java/com/facebook/presto/operator/scalar/TryFunction.java:161` </location>
<code_context>
- || errorCode == INVALID_CAST_ARGUMENT.toErrorCode().getCode()
- || errorCode == INVALID_FUNCTION_ARGUMENT.toErrorCode().getCode()
- || errorCode == NUMERIC_VALUE_OUT_OF_RANGE.toErrorCode().getCode()) {
+ if (e.getErrorCode().isCatchableByTry()) {
return;
}
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Re-evaluate whether TRY should rely solely on `isCatchableByTry` without also checking error type.
Previously, TRY only swallowed a few explicitly whitelisted USER_ERROR codes. With this change, any `PrestoException` whose `ErrorCode` is marked `catchableByTry` will be ignored, including INTERNAL/EXTERNAL errors, which could mask serious issues if a code is misclassified.
Consider constraining this to USER errors (e.g., `e.getErrorCode().getType() == USER_ERROR && e.getErrorCode().isCatchableByTry()`) or clearly enforcing/documenting that only safe USER_ERROR codes can ever be marked `catchableByTry`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
presto-main-base/src/main/java/com/facebook/presto/operator/scalar/TryFunction.java
Show resolved
Hide resolved
0d9f1b5 to
b8236b8
Compare
feilong-liu
reviewed
Jan 15, 2026
...mespace-managers/src/main/java/com/facebook/presto/functionNamespace/rest/RestErrorCode.java
Outdated
Show resolved
Hide resolved
…TRY error handling This change adds a `catchableByTry` boolean field to ErrorCode that indicates whether an error can be caught by the TRY() SQL function. This makes TRY error handling extensible without requiring code changes to TryFunction.java. Changes: - Add `catchableByTry` field to ErrorCode.java with @ThriftField(5) and @JsonProperty - Add backward-compatible constructors that default catchableByTry to false - Mark DIVISION_BY_ZERO, INVALID_CAST_ARGUMENT, INVALID_FUNCTION_ARGUMENT, and NUMERIC_VALUE_OUT_OF_RANGE as catchableByTry=true in StandardErrorCode - Refactor TryFunction.propagateIfUnhandled() to use isCatchableByTry() - Add comprehensive unit tests for ErrorCode and StandardErrorCode
b8236b8 to
42d99bd
Compare
feilong-liu
approved these changes
Jan 16, 2026
This was referenced Mar 31, 2026
15 tasks
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Add a
catchableByTryboolean flag to ErrorCode that allows errors to be declaratively marked as catchable by the TRY() function. This replaces hardcoded error code checks in TryFunction with a single flag check, making it extensible for future error types.Changes:
catchableByTryfield to ErrorCode with JSON/Thrift serializationThis allows any future error code to be marked as TRY-catchable at definition time without modifying TryFunction.