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

fix: Fix various complaints from the latest nightly clippy #11958

Merged
merged 3 commits into from
Aug 13, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 35 additions & 33 deletions datafusion-cli/Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions datafusion/common/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Copy link
Contributor

Choose a reason for hiding this comment

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

for anyone else reviewing this PR, this is not a new dependency as it was already used transitively by other packages

pyo3 = { version = "0.21.0", optional = true }
sqlparser = { workspace = true }

Expand Down
39 changes: 13 additions & 26 deletions datafusion/common/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -481,13 +481,6 @@ macro_rules! unwrap_or_internal_err {
};
}

macro_rules! with_dollar_sign {
($($body:tt)*) => {
macro_rules! __with_dollar_sign { $($body)* }
__with_dollar_sign!($);
}
}

/// Add a macros for concise DataFusionError::* errors declaration
/// supports placeholders the same way as `format!`
/// Examples:
Expand All @@ -501,17 +494,17 @@ macro_rules! with_dollar_sign {
/// `NAME_DF_ERR` - macro name for wrapping DataFusionError::*. Needed to keep backtrace opportunity
/// in construction where DataFusionError::* used directly, like `map_err`, `ok_or_else`, etc
macro_rules! make_error {
($NAME_ERR:ident, $NAME_DF_ERR: ident, $ERR:ident) => {
with_dollar_sign! {
($d:tt) => {
($NAME_ERR:ident, $NAME_DF_ERR: ident, $ERR:ident) => { make_error!(@inner ($), $NAME_ERR, $NAME_DF_ERR, $ERR); };
(@inner ($d:tt), $NAME_ERR:ident, $NAME_DF_ERR:ident, $ERR:ident) => {
::paste::paste!{
/// Macro wraps `$ERR` to add backtrace feature
#[macro_export]
macro_rules! $NAME_DF_ERR {
($d($d args:expr),*) => {
$crate::DataFusionError::$ERR(
format!(
::std::format!(
"{}{}",
format!($d($d args),*),
::std::format!($d($d args),*),
$crate::DataFusionError::get_back_trace(),
).into()
)
Expand All @@ -522,16 +515,16 @@ macro_rules! make_error {
#[macro_export]
macro_rules! $NAME_ERR {
($d($d args:expr),*) => {
Err($crate::DataFusionError::$ERR(
format!(
"{}{}",
format!($d($d args),*),
$crate::DataFusionError::get_back_trace(),
).into()
))
}
Err($crate::[<_ $NAME_DF_ERR>]!($d($d args),*))
}
}

#[doc(hidden)]
#[allow(unused)]
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does this macro need to be marked as #[allow(unused)]? I thought these macros are used in various places of the codebase

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

This makes sense -- I will add your reasoning as a comment in a follow on PR

pub use $NAME_ERR as [<_ $NAME_ERR>];
#[doc(hidden)]
#[allow(unused)]
pub use $NAME_DF_ERR as [<_ $NAME_DF_ERR>];
}
};
}
Expand Down Expand Up @@ -613,12 +606,6 @@ macro_rules! schema_err {

// To avoid compiler error when using macro in the same crate:
// macros from the current crate cannot be referred to by absolute paths
pub use config_err as _config_err;
pub use internal_datafusion_err as _internal_datafusion_err;
pub use internal_err as _internal_err;
pub use not_impl_err as _not_impl_err;
pub use plan_datafusion_err as _plan_datafusion_err;
pub use plan_err as _plan_err;
pub use schema_err as _schema_err;

/// Create a "field not found" DataFusion::SchemaError
Expand Down
12 changes: 12 additions & 0 deletions datafusion/common/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

Choose a reason for hiding this comment

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

I double checked the docs locally:

Screenshot 2024-08-12 at 6 05 07 PM

// These are just re-exports of macros by the same name, which gets around the 'cannot refer to
// macro-expanded macro_export macros by their full path' error.
// The design to get around this comes from this comment:
// https://github.com/rust-lang/rust/pull/52234#issuecomment-976702997
#[doc(hidden)]
pub use error::{
_config_datafusion_err, _exec_datafusion_err, _internal_datafusion_err,
_not_impl_datafusion_err, _plan_datafusion_err, _resources_datafusion_err,
_substrait_datafusion_err,
};

/// Downcast an Arrow Array to a concrete type, return an `DataFusionError::Internal` if the cast is
/// not possible. In normal usage of DataFusion the downcast should always succeed.
///
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -341,14 +341,9 @@ pub fn build_row_filter(
let mut candidates: Vec<FilterCandidate> = predicates
.into_iter()
.flat_map(|expr| {
if let Ok(candidate) =
FilterCandidateBuilder::new(expr.clone(), file_schema, table_schema)
.build(metadata)
{
candidate
} else {
None
}
.unwrap_or_default()
})
.collect();

Expand Down
Loading