Skip to content
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

Consistent error messages when validating function arguments #5888

Open
alex-krash opened this issue Jul 5, 2019 · 3 comments
Open

Consistent error messages when validating function arguments #5888

alex-krash opened this issue Jul 5, 2019 · 3 comments
Assignees
Labels

Comments

@alex-krash
Copy link
Contributor

Current situation
Currently, SQL functions performs validation of given arguments inside their body. During validation, there are common error situations (illegal types of arguments, number of arguments does not match, etc.). Currently, error messages for such cases are formed in each function separately, and it has following circumstances:

  1. In each function implementation file, we are adding section ErrorCodes with code constants
  2. Copy-pasted creation of exception messages (toString(xxx), etc)
  3. Messages sometimes drops useful context (position of argument, given type and expected type)

Proposal
I propose to unify messages for arguments validation, in following manner:
Wrong number of variadic arguments
Function {function_name} expects {start} - {end} arguments, {given_number_of_arguments} given
Variadic function called without arguments
Function {function_name} expects at least one argument, but none given
Too much arguments for variadic function
Function {function_name} expects at most {max_arguments}, but {given_count} given

Validating types of arguments:

Illegal type {given_type} of {position} argument for function {function_name}
Illegal type {given_type} of {position} argument for function {function_name} - must be {expected_type}
Illegal type {given_type} of {position} argument for function {function_name} - must be {expected_type1} or {expected_type2} or {expected_type3}

(within implementation, {position} can be human-friendly for first 9 positions:
Illegal type UInt16 of third argument for function multiIf)

Describe the solution you'd like
From my point of view, it would be good to implement several DB::Exception sub-classes with appropriate constructors. Constructor will accept DB::IFunction instance as first argument, and rest arguments, needed to form error-message. On top of that, sub-class will always have it's error-code, so no need to import it explicitly in function implementation file.

Describe alternatives you've considered
If solution with exception sub-classes is not suitable, than we may, at least move creation of error messages into specific static functions in some helper.

@alexey-milovidov alexey-milovidov self-assigned this Jul 5, 2019
@alexey-milovidov
Copy link
Member

Solved in #3775

@alex-krash
Copy link
Contributor Author

@alexey-milovidov , yeah, I remember this PR. Looks like it simplifies checks a lot.
The only thing left here are error codes (as I understand, your implementation will always use ErrorCodes::ILLEGAL_TYPE_OF_ARGUMENT, whereas negative "unit" tests uses --{serverError XXX} for checking particular validations to be passed.

@alexey-milovidov
Copy link
Member

as I understand, your implementation will always use

The purpose is to make function signatures analyzable. It will allow to make better error codes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants