refactor: Replace throw std::invalid_argument with VELOX_USER_FAIL in presto-native-execution#27308
Merged
tanjialiang merged 1 commit intoprestodb:masterfrom Mar 11, 2026
Merged
Conversation
Contributor
Reviewer's guide (collapsed on small PRs)Reviewer's GuideReplaces raw std::invalid_argument throws in Presto native execution with Velox error macros so errors are properly classified and translated by Velox/Presto infrastructure, and refines the choice of macro based on whether the error is an unsupported feature or a generic failure. Sequence diagram for updated error translation using Velox macrossequenceDiagram
actor User
participant PrestoClient
participant PrestoCoordinator
participant PrestoWorker
participant VeloxOperator
participant VeloxToPrestoExceptionTranslator
User->>PrestoClient: Submit query
PrestoClient->>PrestoCoordinator: Send SQL query
PrestoCoordinator->>PrestoWorker: Schedule tasks with splits
PrestoWorker->>VeloxOperator: Process split / expression
alt Invalid remote split URL (PrestoExchangeSource_extractTaskId)
VeloxOperator->>VeloxOperator: VELOX_FAIL Cannot extract task ID
else Unexpected Block type (VeloxExprConverter_getConstantValue)
VeloxOperator->>VeloxOperator: VELOX_UNSUPPORTED Unexpected Block type
else Unsupported RowExpression type (VeloxExprConverter_toVeloxExpr)
VeloxOperator->>VeloxOperator: VELOX_UNSUPPORTED Unsupported RowExpression type
end
VeloxOperator-->>VeloxToPrestoExceptionTranslator: Propagate VeloxException
VeloxToPrestoExceptionTranslator-->>PrestoWorker: Map to Presto user error code
PrestoWorker-->>PrestoCoordinator: Return classified user error
PrestoCoordinator-->>PrestoClient: Return Presto error response
PrestoClient-->>User: Display user-facing error with proper code
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 left some high level feedback:
- The commit message and summary mention replacing
std::invalid_argumentwithVELOX_USER_FAIL, but the code usesVELOX_FAILandVELOX_UNSUPPORTED; consider aligning the implementation and description and ensuring the chosen macros actually route these conditions to the intended user-facing error classification. - For
extractTaskId, if this condition is considered a user input or configuration error rather than an internal failure, consider using the explicit user-error macro (e.g.,VELOX_USER_FAIL) instead ofVELOX_FAILto avoid misclassifying the error. - For the unexpected Block type and unsupported RowExpression type cases, verify whether they should be treated as user errors or unsupported features, and choose between
VELOX_USER_FAILandVELOX_UNSUPPORTEDaccordingly so they map to the appropriate Presto error codes.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The commit message and summary mention replacing `std::invalid_argument` with `VELOX_USER_FAIL`, but the code uses `VELOX_FAIL` and `VELOX_UNSUPPORTED`; consider aligning the implementation and description and ensuring the chosen macros actually route these conditions to the intended user-facing error classification.
- For `extractTaskId`, if this condition is considered a user input or configuration error rather than an internal failure, consider using the explicit user-error macro (e.g., `VELOX_USER_FAIL`) instead of `VELOX_FAIL` to avoid misclassifying the error.
- For the unexpected Block type and unsupported RowExpression type cases, verify whether they should be treated as user errors or unsupported features, and choose between `VELOX_USER_FAIL` and `VELOX_UNSUPPORTED` accordingly so they map to the appropriate Presto error codes.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
gggrace14
previously approved these changes
Mar 10, 2026
…tive-execution (prestodb#27308) Summary: 3 production sites throw raw `std::invalid_argument`, which bypasses `VeloxToPrestoExceptionTranslator` and misclassifies user errors as `GENERIC_INTERNAL_ERROR`. Replace them with `VELOX_USER_FAIL` so they are properly translated to user-facing Presto error codes. Sites fixed: - `PrestoExchangeSource.cpp` - invalid task ID in remote split URL - `PrestoToVeloxExpr.cpp:131` - unexpected Block type - `PrestoToVeloxExpr.cpp:905` - unsupported RowExpression type Reviewed By: kewang1024 Differential Revision: D96025465
df27d4a to
46f2c8c
Compare
tanjialiang
added a commit
to tanjialiang/presto
that referenced
this pull request
Mar 11, 2026
…tive-execution (prestodb#27308) Summary: 3 production sites throw raw `std::invalid_argument`, which bypasses `VeloxToPrestoExceptionTranslator` and misclassifies user errors as `GENERIC_INTERNAL_ERROR`. Replace them with `VELOX_USER_FAIL` so they are properly translated to user-facing Presto error codes. Sites fixed: - `PrestoExchangeSource.cpp` - invalid task ID in remote split URL - `PrestoToVeloxExpr.cpp:131` - unexpected Block type - `PrestoToVeloxExpr.cpp:905` - unsupported RowExpression type Reviewed By: kewang1024 Differential Revision: D96025465
gggrace14
approved these changes
Mar 11, 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.
Summary:
3 production sites throw raw
std::invalid_argument, which bypassesVeloxToPrestoExceptionTranslatorand misclassifies user errors asGENERIC_INTERNAL_ERROR. Replace them withVELOX_USER_FAILso theyare properly translated to user-facing Presto error codes.
Sites fixed:
PrestoExchangeSource.cpp- invalid task ID in remote split URLPrestoToVeloxExpr.cpp:131- unexpected Block typePrestoToVeloxExpr.cpp:905- unsupported RowExpression typeDifferential Revision: D96025465
Summary by Sourcery
Normalize error handling in Presto native execution by replacing raw std::invalid_argument throws with Velox error macros for proper Presto error translation.
Bug Fixes: