-
Notifications
You must be signed in to change notification settings - Fork 5.5k
fix: Throw USER_ERROR instead of FUNCTION_IMPLEMENTATION_MISSING for unsupported parameters #26553
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…unsupported parameters.
Reviewer's guide (collapsed on small PRs)Reviewer's GuideUpdate the error code thrown in scalar function specialization for unsupported type parameters from FUNCTION_IMPLEMENTATION_MISSING to NOT_SUPPORTED to ensure proper user error classification. Class diagram for updated error handling in ParametricScalarclassDiagram
class ParametricScalar {
+specialize(boundVariables)
}
ParametricScalar --> PrestoException
class PrestoException {
+PrestoException(errorCode, message)
}
%% Error codes used
class FUNCTION_IMPLEMENTATION_MISSING
class NOT_SUPPORTED
PrestoException --> FUNCTION_IMPLEMENTATION_MISSING
PrestoException --> NOT_SUPPORTED
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
Can we introduce unit or integration tests to make sure this functionality has been verified? |
swapsmagic
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's add unit test and should be good to merge.
## Description PR #26553 changed the error code from `FUNCTION_IMPLEMENTATION_MISSING` to `NOT_SUPPORTED` where parameter isn't supported to reduce unnecessary retry. This PR fix the relevant test case to eliminate the test failure. ## Motivation and Context - Eliminate test failure ## Impact N/A ## Test Plan N/A ## Contributor checklist - [ ] Please make sure your submission complies with our [contributing guide](https://github.com/prestodb/presto/blob/master/CONTRIBUTING.md), in particular [code style](https://github.com/prestodb/presto/blob/master/CONTRIBUTING.md#code-style) and [commit standards](https://github.com/prestodb/presto/blob/master/CONTRIBUTING.md#commit-standards). - [ ] PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced. - [ ] Documented new properties (with its default value), SQL syntax, functions, or other functionality. - [ ] If release notes are required, they follow the [release notes guidelines](https://github.com/prestodb/presto/wiki/Release-Notes-Guidelines). - [ ] Adequate tests were added if applicable. - [ ] CI passed. - [ ] If adding new dependencies, verified they have an [OpenSSF Scorecard](https://securityscorecards.dev/#the-checks) score of 5.0 or higher (or obtained explicit TSC approval for lower scores). ## Release Notes ``` == NO RELEASE NOTE == ```
Description
Throwing proper error code for better error classification.
Motivation and Context
We see queries failing with
Unsupported type parameters (BoundVariables{typeVariables={T=array(varchar)}, longVariables={}}) for presto.default.approx_distinct<T>(T):bigintwhere parmater isn't supported. This is simply user error (also the error message suggests so). Throwing user error code in this case reudce unnecessary retry, miscategorization and send proper signal to users.Impact
None
Test Plan
Existing