fix: Fix various complaints from the latest nightly clippy #11958
Conversation
github-actions
bot
added
sql
There was a problem hiding this comment.
Thank you @itsjunetime 🙏
This will also avoid the mad scramble we often do when a new rust release comes out and we have to make this sort of change to get CI running
I had a few questions on changes in error handling that i think should be addressed, but otherwise this is looking almost ready to ship to me
Note for other reviewers: I found using whitespace blind diff for this change made it easier to see what changed: https://github.com/apache/datafusion/pull/11958/files?w=1
| @@ -60,6 +60,7 @@ libc = "0.2.140" | |||
| num_cpus = { workspace = true } | |||
| object_store = { workspace = true, optional = true } | |||
| parquet = { workspace = true, optional = true, default-features = true } | |||
| paste = "1.0.15" | |||
There was a problem hiding this comment.
for anyone else reviewing this PR, this is not a new dependency as it was already used transitively by other packages
| } | ||
| } | ||
|
|
||
| #[doc(hidden)] | ||
| #[allow(unused)] |
There was a problem hiding this comment.
Why does this macro need to be marked as #[allow(unused)]? I thought these macros are used in various places of the codebase
There was a problem hiding this comment.
rust-analyzer still analyzes macro output and will complain if they generate unused typealiases. Previously we got around that by explicitly adding something like pub use err as _err only where needed, but this makes those identifiers for all errors, (to make it easier to use in the future), but some of them aren't currently used in the codebase. So those typealiases are technically unused, but that's fine because they're just there for if we need them inside the crate.
There was a problem hiding this comment.
This makes sense -- I will add your reasoning as a comment in a follow on PR
| @@ -73,6 +73,18 @@ pub use table_reference::{ResolvedTableReference, TableReference}; | |||
| pub use unnest::UnnestOptions; | |||
| pub use utils::project_schema; | |||
|
|
|||
| // These are hidden from docs purely to avoid polluting the public view of what this crate exports. | |||
There was a problem hiding this comment.
I double checked the docs locally:
| let mut val: Option<Result<ColumnarValue>> = None; | ||
| let mut err: Option<DataFusionError> = None; | ||
| let a = a.as_ref(); | ||
| // ASK: Why do we trust `a` to be non-null at this point? |
There was a problem hiding this comment.
I don't think we do trust a to be non-null -- the current code on main has an explicitly match that handles None doesn't it? Maybe I am misreading the diff
None => (),🤔
However, clearly we don't have test coverage for this
There was a problem hiding this comment.
Previously, in this code path, if a was None, then the err that we unwrap later down in that same block would also be None, causing a more confusing error. So since we're comfortable unwrapping the err later on, we must be confident a is Some(_).
| if let Some(v) = val { | ||
| v | ||
| } else { | ||
| Err(err.unwrap()) |
There was a problem hiding this comment.
that is pretty strange -- I like your formulation unwrap_or_internal_err much better
There was a problem hiding this comment.
Yeah, I feel like this is explicitly the thing that enums allow us to avoid
| Err(e) => parser_err!(format!( | ||
| "Unexpected error: could not parse '{n}' as number: {e}" | ||
| )), | ||
| Err(e) => match e {}, |
There was a problem hiding this comment.
I don't understand this change -- doesn't match e{} just ignore e where as before this would have returned an error?
There was a problem hiding this comment.
e is Infallible, so it can't be constructed. Since it can't be constructed, this code path is unreachable, and that's what this proves. a match with no arms has a return type of ! since it can't ever be executed, so it can be implicitly converted to any type, and can prove that a Result cannot be its Err variant.
There was a problem hiding this comment.
I think we can avoid this error handling entirely by not trying to parse a String in the first place. I'll make a follow on PR
| "Sort operator only valid in a statement context." | ||
| ); | ||
| } | ||
| if let Some(sort_expr) = &on.sort_expr { |
There was a problem hiding this comment.
Another way I have seen this written is
| if let Some(sort_expr) = &on.sort_expr { | |
| if let Some(sort_expr) = on.sort_expr.as_ref() { |
There was a problem hiding this comment.
Ah, yeah - this change was made by cargo clippy --fix so I just went with it. I don't really have any preference for which it should be.
There was a problem hiding this comment.
Thank you @itsjunetime -- I think this is better than main 🙏
I have some ideas on how to simplify the code further, and I'll make another PR with some proposals
| Err(e) => parser_err!(format!( | ||
| "Unexpected error: could not parse '{n}' as number: {e}" | ||
| )), | ||
| Err(e) => match e {}, |
There was a problem hiding this comment.
I think we can avoid this error handling entirely by not trying to parse a String in the first place. I'll make a follow on PR
|
Follow on PR in #11965 |
Which issue does this PR close?
Closes #11957
Rationale for this change
It's nice to keep ahead of future rust changes and make sure people can comfortably use this library on any toolchain.
What changes are included in this PR?
make_errormacro that's used to make all the errors + variants to prevent code duplication and get rid of unnecessarywith_dollar_signmacroAre these changes tested?
Yes, it passes all tests that main currently passes on my machine.
Are there any user-facing changes?
This adds items to the public API, under
#[doc(hidden)]. This is not a breaking change afaik, but would cause a breaking change if they were removed.