-
Notifications
You must be signed in to change notification settings - Fork 6.6k
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
harmonize h3PointDist* error messages #64080
harmonize h3PointDist* error messages #64080
Conversation
This is an automated comment for commit 524f289 with description of existing statuses. It's updated for the latest CI running ❌ Click here to open a full report in a separate page
Successful checks
|
@@ -49,7 +49,7 @@ class FunctionH3PointDist final : public IFunction | |||
throw Exception( | |||
ErrorCodes::ILLEGAL_TYPE_OF_ARGUMENT, | |||
"Illegal type {} of argument {} of function {}. Must be Float64", | |||
arg->getName(), i, getName()); | |||
arg->getName(), i + 1, getName()); |
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.
We could use FunctionArgumentDescriptor
(src/Functions/FunctionHelpers.cpp) which does all the validation based on an descriptor struct. This would also guarantee consistent error messages (across all functions using FunctionArgumentDescriptor
).
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.
That would indeed be a lot better!
Looks like validateArgumentType
is currently used in in geohash functions, and I don't see any usage of validateArgumentsImpl
.
From what I previously saw in the codebase, that could be applied for most functions (there are exceptions like hascolumnintable
or some functions that accepts 1 or 3 parameters).
So I suppose either it's done from time to time (like in this PR), or a single PR updates the different functions.
I never learnt how to code in cpp, so when contributing to the codebase, I'd rather stay in fixing typo and other minor things.
I don't feel comfortable introducing that change in this PR myself.
When error msg in
getReturnTypeImpl
would printError msg in
executeImpl
would printThis PR ensures both messages are the same, and choose to follow the error message in
executeImpl
.Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
...
Documentation entry for user-facing changes
Modify your CI run
NOTE: If your merge the PR with modified CI you MUST KNOW what you are doing
NOTE: Checked options will be applied if set before CI RunConfig/PrepareRunConfig step
Include tests (required builds will be added automatically):
Exclude tests:
Extra options:
Only specified batches in multi-batch jobs: