Skip to content

Change error type of invalid argument to PlanError rather than InternalError, remove misleading comments - #7355

Merged
alamb merged 5 commits into
apache:mainfrom
alamb:alamb/misleading_errors
Aug 23, 2023
Merged

Change error type of invalid argument to PlanError rather than InternalError, remove misleading comments#7355
alamb merged 5 commits into
apache:mainfrom
alamb:alamb/misleading_errors

Conversation

@alamb

@alambalamb commented Aug 21, 2023

Copy link
Copy Markdown
Contributor

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

DataFusion CLI v29.0.0
❯ select encode(1,2);
Internal error: The encode function can only accept utf8 or binary.. This was likely caused by a bug in DataFusion's code and we would welcome that you file an bug report in our issue tracker
❯ select array_concat(1,2);
Internal error: The array_concat function can only accept list as the args.. This was likely caused by a bug in DataFusion's code and we would welcome that you file an bug report in our issue tracker

What changes are included in this PR?

  1. Change to DataFusionError::Plan
  2. Update tests
  3. Update comments

Are these changes tested?

Yes

Are there any user-facing changes?

yes, better error messages

@github-actionsgithub-actionsBot added the logical-expr Logical plan and expressions label Aug 21, 2023
@github-actionsgithub-actionsBot added the sqllogictest SQL Logic Tests (.slt) label Aug 21, 2023
@alambalamb changed the title Fix error type of invalid argument types, remove misleading commentsChange error type of invalid argument to PlanError rather than InternalError, remove misleading commentsAug 21, 2023
_ => {
// this error is internal as `data_types` should have captured this.
return internal_err!(
return plan_err!(

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@alamb
alamb marked this pull request as ready for review August 21, 2023 20:56
@alamb

Copy link
Copy Markdown
ContributorAuthor

Thank you @DDtKey !

@alamb

Copy link
Copy Markdown
ContributorAuthor

@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

Screenshot 2023-08-23 at 6 11 25 AM

What is the reason we want to require an approval from one of the committers prior to merging?

@alamb
alamb merged commit bfe3e42 into apache:mainAug 23, 2023
@alamb

Copy link
Copy Markdown
ContributorAuthor

Thank you @Dandandan

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

logical-exprLogical plan and expressionssqllogictestSQL Logic Tests (.slt)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@alamb@Dandandan@Weijun-H@DDtKey