Change error type of invalid argument to PlanError rather than InternalError, remove misleading comments #7355
Conversation
This file contains 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
alamb
changed the title
alamb
commented
Aug 21, 2023
| @@ -737,8 +732,7 @@ impl BuiltinScalarFunction { | |||
| Utf8 => List(Arc::new(Field::new("item", Utf8, true))), | |||
| Null => Null, | |||
| _ => { | |||
| // this error is internal as `data_types` should have captured this. | |||
| return internal_err!( | |||
| return plan_err!( | |||
There was a problem hiding this comment.
it is true that this is currently caught by higher up type checks, but I think it is better to throw a plan error here to tell users about the problem if something changes rather than throw a confusing internal error
Also given that this code is often copy/pasted it probably should default to returning a nice error message rather than an opaque one
DDtKey
approved these changes
Aug 22, 2023
|
Thank you @DDtKey ! |
Weijun-H
approved these changes
Aug 23, 2023
|
@jackwener / @liukun4515 / @Dandandan -- here is an example of a PR that has been reviewed by contributors but that I can't merge due to the restriction put in place by #7226 
What is the reason we want to require an approval from one of the committers prior to merging? |
Dandandan
approved these changes
Aug 23, 2023
|
Thank you @Dandandan |
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.
Which issue does this PR close?
related to #6108 and #7326
related to #7339
Rationale for this change
Inspired by @Weijun-H 's change in #7339 I was trying to clean up a comment I saw left over from #7339 I realized that not only are the comments in the rest of this file misleading the same bug also applied to several other functions.
Specifically, despite the comment claims that "this should have been covered earlier" it is clearly not and some of these error paths are directly user visible due to invalid queries (and thus should be plan errors, not internal errors). For example
What changes are included in this PR?
Are these changes tested?
Yes
Are there any user-facing changes?
yes, better error messages