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.

Sign up for GitHub

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

Minor: some cosmetics in filter.rs, fix clippy due to logical conflict #11368

Merged
merged 2 commits into from
Jul 9, 2024
Merged

Minor: some cosmetics in filter.rs, fix clippy due to logical conflict #11368

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
980d1a5
Minor: some cosmetics in `filter.rs`
comphead Jul 9, 2024
22bcf35
Minor: some cosmetics in `filter.rs`
comphead Jul 9, 2024
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
21 changes: 10 additions & 11 deletions datafusion/physical-plan/src/filter.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,6 @@
// specific language governing permissions and limitations
// under the License.

//! FilterExec evaluates a boolean predicate against all input batches to determine which rows to
//! include in its output batches.
use std::any::Any;
use std::pin::Pin;
use std::sync::Arc;
Expand Down Expand Up @@ -60,7 +57,7 @@ pub struct FilterExec {
input: Arc<dyn ExecutionPlan>,
/// Execution metrics
metrics: ExecutionPlanMetricsSet,
/// Selectivity for statistics. 0 = no rows, 100 all rows
/// Selectivity for statistics. 0 = no rows, 100 = all rows
default_selectivity: u8,
cache: PlanProperties,
}
Expand Down Expand Up @@ -91,14 +88,14 @@ impl FilterExec {

Ok(Self {
predicate,
input: input.clone(),
input: Arc::clone(&input),
Copy link
Contributor

Choose a reason for hiding this comment

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

This was causing clippy failures on main https://github.com/apache/datafusion/actions/runs/9857745125/job/27217695529

I think due to a logical conflict

metrics: ExecutionPlanMetricsSet::new(),
default_selectivity,
cache,
})
}
other => {
plan_err!("Filter predicate must return boolean values, not {other:?}")
plan_err!("Filter predicate must return BOOLEAN values, got {other:?}")
}
}
}
Expand All @@ -108,7 +105,9 @@ impl FilterExec {
default_selectivity: u8,
) -> Result<Self, DataFusionError> {
if default_selectivity > 100 {
return plan_err!("Default filter selectivity needs to be less than 100");
return plan_err!(
"Default filter selectivity value needs to be less than or equal to 100"
);
}
self.default_selectivity = default_selectivity;
Ok(self)
Expand Down Expand Up @@ -369,12 +368,12 @@ pub(crate) fn batch_filter(
.and_then(|v| v.into_array(batch.num_rows()))
.and_then(|array| {
let filter_array = match as_boolean_array(&array) {
Ok(boolean_array) => {
Ok(boolean_array.to_owned())
},
Ok(boolean_array) => Ok(boolean_array.to_owned()),
Err(_) => {
let Ok(null_array) = as_null_array(&array) else {
return internal_err!("Cannot create filter_array from non-boolean predicates, unable to continute");
return internal_err!(
Copy link
Contributor

Choose a reason for hiding this comment

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

❤️

"Cannot create filter_array from non-boolean predicates"
);
};

// if the predicate is null, then the result is also null
Expand Down