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

[infra] Fail Clippy on rust build warnings #1029

Merged
merged 4 commits into from
Feb 20, 2025
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
2 changes: 1 addition & 1 deletion .github/workflows/test.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ jobs:

- name: Run Clippy
if: ${{ matrix.python-version == '3.10' && matrix.toolchain == 'stable' }}
run: cargo clippy --all-targets --all-features -- -D clippy::all -A clippy::redundant_closure
run: cargo clippy --all-targets --all-features -- -D clippy::all -D warnings -A clippy::redundant_closure

- name: Install dependencies and build
uses: astral-sh/setup-uv@v5
Expand Down
2 changes: 1 addition & 1 deletion .pre-commit-config.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ repos:
- id: rust-clippy
name: Rust clippy
description: Run cargo clippy on files included in the commit. clippy should be installed before-hand.
entry: cargo clippy --all-targets --all-features -- -Dclippy::all -Aclippy::redundant_closure
entry: cargo clippy --all-targets --all-features -- -Dclippy::all -D warnings -Aclippy::redundant_closure
pass_filenames: false
types: [file, rust]
language: system
Expand Down
10 changes: 5 additions & 5 deletions src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -47,14 +47,14 @@ impl PyConfig {
}

/// Get a configuration option
pub fn get(&mut self, key: &str, py: Python) -> PyResult<PyObject> {
pub fn get<'py>(&mut self, key: &str, py: Python<'py>) -> PyResult<Bound<'py, PyAny>> {
let options = self.config.to_owned();
for entry in options.entries() {
if entry.key == key {
return Ok(entry.value.into_py(py));
return Ok(entry.value.into_pyobject(py)?);
}
}
Ok(None::<String>.into_py(py))
Ok(None::<String>.into_pyobject(py)?)
}

/// Set a configuration option
Expand All @@ -66,10 +66,10 @@ impl PyConfig {

/// Get all configuration options
pub fn get_all(&mut self, py: Python) -> PyResult<PyObject> {
let dict = PyDict::new_bound(py);
let dict = PyDict::new(py);
let options = self.config.to_owned();
for entry in options.entries() {
dict.set_item(entry.key, entry.value.clone().into_py(py))?;
dict.set_item(entry.key, entry.value.clone().into_pyobject(py)?)?;
}
Ok(dict.into())
}
Expand Down
12 changes: 6 additions & 6 deletions src/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -458,8 +458,8 @@ impl PySessionContext {
let py = data.py();

// Instantiate pyarrow Table object & convert to Arrow Table
let table_class = py.import_bound("pyarrow")?.getattr("Table")?;
let args = PyTuple::new_bound(py, &[data]);
let table_class = py.import("pyarrow")?.getattr("Table")?;
let args = PyTuple::new(py, &[data])?;
let table = table_class.call_method1("from_pylist", args)?;

// Convert Arrow Table to datafusion DataFrame
Expand All @@ -478,8 +478,8 @@ impl PySessionContext {
let py = data.py();

// Instantiate pyarrow Table object & convert to Arrow Table
let table_class = py.import_bound("pyarrow")?.getattr("Table")?;
let args = PyTuple::new_bound(py, &[data]);
let table_class = py.import("pyarrow")?.getattr("Table")?;
let args = PyTuple::new(py, &[data])?;
let table = table_class.call_method1("from_pydict", args)?;

// Convert Arrow Table to datafusion DataFrame
Expand Down Expand Up @@ -533,8 +533,8 @@ impl PySessionContext {
let py = data.py();

// Instantiate pyarrow Table object & convert to Arrow Table
let table_class = py.import_bound("pyarrow")?.getattr("Table")?;
let args = PyTuple::new_bound(py, &[data]);
let table_class = py.import("pyarrow")?.getattr("Table")?;
let args = PyTuple::new(py, &[data])?;
let table = table_class.call_method1("from_pandas", args)?;

// Convert Arrow Table to datafusion DataFrame
Expand Down
17 changes: 8 additions & 9 deletions src/dataframe.rs
Original file line number Diff line number Diff line change
Expand Up @@ -545,12 +545,12 @@ impl PyDataFrame {
/// Convert to Arrow Table
/// Collect the batches and pass to Arrow Table
fn to_arrow_table(&self, py: Python<'_>) -> PyResult<PyObject> {
let batches = self.collect(py)?.to_object(py);
let schema: PyObject = self.schema().into_pyobject(py)?.to_object(py);
let batches = self.collect(py)?.into_pyobject(py)?;
let schema = self.schema().into_pyobject(py)?;

// Instantiate pyarrow Table object and use its from_batches method
let table_class = py.import_bound("pyarrow")?.getattr("Table")?;
let args = PyTuple::new_bound(py, &[batches, schema]);
let table_class = py.import("pyarrow")?.getattr("Table")?;
let args = PyTuple::new(py, &[batches, schema])?;
let table: PyObject = table_class.call_method1("from_batches", args)?.into();
Ok(table)
}
Expand Down Expand Up @@ -585,8 +585,7 @@ impl PyDataFrame {

let ffi_stream = FFI_ArrowArrayStream::new(reader);
let stream_capsule_name = CString::new("arrow_array_stream").unwrap();
PyCapsule::new_bound(py, ffi_stream, Some(stream_capsule_name))
.map_err(PyDataFusionError::from)
PyCapsule::new(py, ffi_stream, Some(stream_capsule_name)).map_err(PyDataFusionError::from)
}

fn execute_stream(&self, py: Python) -> PyDataFusionResult<PyRecordBatchStream> {
Expand Down Expand Up @@ -649,8 +648,8 @@ impl PyDataFrame {
/// Collect the batches, pass to Arrow Table & then convert to polars DataFrame
fn to_polars(&self, py: Python<'_>) -> PyResult<PyObject> {
let table = self.to_arrow_table(py)?;
let dataframe = py.import_bound("polars")?.getattr("DataFrame")?;
let args = PyTuple::new_bound(py, &[table]);
let dataframe = py.import("polars")?.getattr("DataFrame")?;
let args = PyTuple::new(py, &[table])?;
let result: PyObject = dataframe.call1(args)?.into();
Ok(result)
}
Expand All @@ -673,7 +672,7 @@ fn print_dataframe(py: Python, df: DataFrame) -> PyDataFusionResult<()> {

// Import the Python 'builtins' module to access the print function
// Note that println! does not print to the Python debug console and is not visible in notebooks for instance
let print = py.import_bound("builtins")?.getattr("print")?;
let print = py.import("builtins")?.getattr("print")?;
print.call1((result,))?;
Ok(())
}
Expand Down
2 changes: 1 addition & 1 deletion src/dataset.rs
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ impl Dataset {
// Creates a Python PyArrow.Dataset
pub fn new(dataset: &Bound<'_, PyAny>, py: Python) -> PyResult<Self> {
// Ensure that we were passed an instance of pyarrow.dataset.Dataset
let ds = PyModule::import_bound(py, "pyarrow.dataset")?;
let ds = PyModule::import(py, "pyarrow.dataset")?;
let ds_attr = ds.getattr("Dataset")?;
let ds_type = ds_attr.downcast::<PyType>()?;
if dataset.is_instance(ds_type)? {
Expand Down
8 changes: 4 additions & 4 deletions src/dataset_exec.rs
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ impl DatasetExec {
})
.transpose()?;

let kwargs = PyDict::new_bound(py);
let kwargs = PyDict::new(py);

kwargs.set_item("columns", columns.clone())?;
kwargs.set_item(
Expand All @@ -121,7 +121,7 @@ impl DatasetExec {
.0,
);

let builtins = Python::import_bound(py, "builtins")?;
let builtins = Python::import(py, "builtins")?;
let pylist = builtins.getattr("list")?;

// Get the fragments or partitions of the dataset
Expand Down Expand Up @@ -198,7 +198,7 @@ impl ExecutionPlan for DatasetExec {
let dataset_schema = dataset
.getattr("schema")
.map_err(|err| InnerDataFusionError::External(Box::new(err)))?;
let kwargs = PyDict::new_bound(py);
let kwargs = PyDict::new(py);
kwargs
.set_item("columns", self.columns.clone())
.map_err(|err| InnerDataFusionError::External(Box::new(err)))?;
Expand All @@ -223,7 +223,7 @@ impl ExecutionPlan for DatasetExec {
let record_batches: Bound<'_, PyIterator> = scanner
.call_method0("to_batches")
.map_err(|err| InnerDataFusionError::External(Box::new(err)))?
.iter()
.try_iter()
.map_err(|err| InnerDataFusionError::External(Box::new(err)))?;

let record_batches = PyArrowBatchesAdapter {
Expand Down
4 changes: 4 additions & 0 deletions src/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -91,3 +91,7 @@ pub fn py_datafusion_err(e: impl Debug) -> PyErr {
pub fn py_unsupported_variant_err(e: impl Debug) -> PyErr {
PyErr::new::<pyo3::exceptions::PyValueError, _>(format!("{e:?}"))
}

pub fn to_datafusion_err(e: impl Debug) -> InnerDataFusionError {
InnerDataFusionError::Execution(format!("{e:?}"))
}
61 changes: 31 additions & 30 deletions src/expr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ use datafusion::logical_expr::utils::exprlist_to_fields;
use datafusion::logical_expr::{
ExprFuncBuilder, ExprFunctionExt, LogicalPlan, WindowFunctionDefinition,
};
use pyo3::IntoPyObjectExt;
use pyo3::{basic::CompareOp, prelude::*};
use std::convert::{From, Into};
use std::sync::Arc;
Expand Down Expand Up @@ -126,35 +127,35 @@ pub fn py_expr_list(expr: &[Expr]) -> PyResult<Vec<PyExpr>> {
#[pymethods]
impl PyExpr {
/// Return the specific expression
fn to_variant(&self, py: Python) -> PyResult<PyObject> {
fn to_variant<'py>(&self, py: Python<'py>) -> PyResult<Bound<'py, PyAny>> {
Python::with_gil(|_| {
match &self.expr {
Expr::Alias(alias) => Ok(PyAlias::from(alias.clone()).into_py(py)),
Expr::Column(col) => Ok(PyColumn::from(col.clone()).into_py(py)),
Expr::Alias(alias) => Ok(PyAlias::from(alias.clone()).into_bound_py_any(py)?),
Expr::Column(col) => Ok(PyColumn::from(col.clone()).into_bound_py_any(py)?),
Expr::ScalarVariable(data_type, variables) => {
Ok(PyScalarVariable::new(data_type, variables).into_py(py))
Ok(PyScalarVariable::new(data_type, variables).into_bound_py_any(py)?)
}
Expr::Like(value) => Ok(PyLike::from(value.clone()).into_py(py)),
Expr::Literal(value) => Ok(PyLiteral::from(value.clone()).into_py(py)),
Expr::BinaryExpr(expr) => Ok(PyBinaryExpr::from(expr.clone()).into_py(py)),
Expr::Not(expr) => Ok(PyNot::new(*expr.clone()).into_py(py)),
Expr::IsNotNull(expr) => Ok(PyIsNotNull::new(*expr.clone()).into_py(py)),
Expr::IsNull(expr) => Ok(PyIsNull::new(*expr.clone()).into_py(py)),
Expr::IsTrue(expr) => Ok(PyIsTrue::new(*expr.clone()).into_py(py)),
Expr::IsFalse(expr) => Ok(PyIsFalse::new(*expr.clone()).into_py(py)),
Expr::IsUnknown(expr) => Ok(PyIsUnknown::new(*expr.clone()).into_py(py)),
Expr::IsNotTrue(expr) => Ok(PyIsNotTrue::new(*expr.clone()).into_py(py)),
Expr::IsNotFalse(expr) => Ok(PyIsNotFalse::new(*expr.clone()).into_py(py)),
Expr::IsNotUnknown(expr) => Ok(PyIsNotUnknown::new(*expr.clone()).into_py(py)),
Expr::Negative(expr) => Ok(PyNegative::new(*expr.clone()).into_py(py)),
Expr::Like(value) => Ok(PyLike::from(value.clone()).into_bound_py_any(py)?),
Expr::Literal(value) => Ok(PyLiteral::from(value.clone()).into_bound_py_any(py)?),
Expr::BinaryExpr(expr) => Ok(PyBinaryExpr::from(expr.clone()).into_bound_py_any(py)?),
Expr::Not(expr) => Ok(PyNot::new(*expr.clone()).into_bound_py_any(py)?),
Expr::IsNotNull(expr) => Ok(PyIsNotNull::new(*expr.clone()).into_bound_py_any(py)?),
Expr::IsNull(expr) => Ok(PyIsNull::new(*expr.clone()).into_bound_py_any(py)?),
Expr::IsTrue(expr) => Ok(PyIsTrue::new(*expr.clone()).into_bound_py_any(py)?),
Expr::IsFalse(expr) => Ok(PyIsFalse::new(*expr.clone()).into_bound_py_any(py)?),
Expr::IsUnknown(expr) => Ok(PyIsUnknown::new(*expr.clone()).into_bound_py_any(py)?),
Expr::IsNotTrue(expr) => Ok(PyIsNotTrue::new(*expr.clone()).into_bound_py_any(py)?),
Expr::IsNotFalse(expr) => Ok(PyIsNotFalse::new(*expr.clone()).into_bound_py_any(py)?),
Expr::IsNotUnknown(expr) => Ok(PyIsNotUnknown::new(*expr.clone()).into_bound_py_any(py)?),
Expr::Negative(expr) => Ok(PyNegative::new(*expr.clone()).into_bound_py_any(py)?),
Expr::AggregateFunction(expr) => {
Ok(PyAggregateFunction::from(expr.clone()).into_py(py))
Ok(PyAggregateFunction::from(expr.clone()).into_bound_py_any(py)?)
}
Expr::SimilarTo(value) => Ok(PySimilarTo::from(value.clone()).into_py(py)),
Expr::Between(value) => Ok(between::PyBetween::from(value.clone()).into_py(py)),
Expr::Case(value) => Ok(case::PyCase::from(value.clone()).into_py(py)),
Expr::Cast(value) => Ok(cast::PyCast::from(value.clone()).into_py(py)),
Expr::TryCast(value) => Ok(cast::PyTryCast::from(value.clone()).into_py(py)),
Expr::SimilarTo(value) => Ok(PySimilarTo::from(value.clone()).into_bound_py_any(py)?),
Expr::Between(value) => Ok(between::PyBetween::from(value.clone()).into_bound_py_any(py)?),
Expr::Case(value) => Ok(case::PyCase::from(value.clone()).into_bound_py_any(py)?),
Expr::Cast(value) => Ok(cast::PyCast::from(value.clone()).into_bound_py_any(py)?),
Expr::TryCast(value) => Ok(cast::PyTryCast::from(value.clone()).into_bound_py_any(py)?),
Expr::ScalarFunction(value) => Err(py_unsupported_variant_err(format!(
"Converting Expr::ScalarFunction to a Python object is not implemented: {:?}",
value
Expand All @@ -163,29 +164,29 @@ impl PyExpr {
"Converting Expr::WindowFunction to a Python object is not implemented: {:?}",
value
))),
Expr::InList(value) => Ok(in_list::PyInList::from(value.clone()).into_py(py)),
Expr::Exists(value) => Ok(exists::PyExists::from(value.clone()).into_py(py)),
Expr::InList(value) => Ok(in_list::PyInList::from(value.clone()).into_bound_py_any(py)?),
Expr::Exists(value) => Ok(exists::PyExists::from(value.clone()).into_bound_py_any(py)?),
Expr::InSubquery(value) => {
Ok(in_subquery::PyInSubquery::from(value.clone()).into_py(py))
Ok(in_subquery::PyInSubquery::from(value.clone()).into_bound_py_any(py)?)
}
Expr::ScalarSubquery(value) => {
Ok(scalar_subquery::PyScalarSubquery::from(value.clone()).into_py(py))
Ok(scalar_subquery::PyScalarSubquery::from(value.clone()).into_bound_py_any(py)?)
}
Expr::Wildcard { qualifier, options } => Err(py_unsupported_variant_err(format!(
"Converting Expr::Wildcard to a Python object is not implemented : {:?} {:?}",
qualifier, options
))),
Expr::GroupingSet(value) => {
Ok(grouping_set::PyGroupingSet::from(value.clone()).into_py(py))
Ok(grouping_set::PyGroupingSet::from(value.clone()).into_bound_py_any(py)?)
}
Expr::Placeholder(value) => {
Ok(placeholder::PyPlaceholder::from(value.clone()).into_py(py))
Ok(placeholder::PyPlaceholder::from(value.clone()).into_bound_py_any(py)?)
}
Expr::OuterReferenceColumn(data_type, column) => Err(py_unsupported_variant_err(format!(
"Converting Expr::OuterReferenceColumn to a Python object is not implemented: {:?} - {:?}",
data_type, column
))),
Expr::Unnest(value) => Ok(unnest_expr::PyUnnestExpr::from(value.clone()).into_py(py)),
Expr::Unnest(value) => Ok(unnest_expr::PyUnnestExpr::from(value.clone()).into_bound_py_any(py)?),
}
})
}
Expand Down
6 changes: 3 additions & 3 deletions src/expr/aggregate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ use datafusion::common::DataFusionError;
use datafusion::logical_expr::expr::{AggregateFunction, Alias};
use datafusion::logical_expr::logical_plan::Aggregate;
use datafusion::logical_expr::Expr;
use pyo3::prelude::*;
use pyo3::{prelude::*, IntoPyObjectExt};
use std::fmt::{self, Display, Formatter};

use super::logical_node::LogicalNode;
Expand Down Expand Up @@ -151,7 +151,7 @@ impl LogicalNode for PyAggregate {
vec![PyLogicalPlan::from((*self.aggregate.input).clone())]
}

fn to_variant(&self, py: Python) -> PyResult<PyObject> {
Ok(self.clone().into_py(py))
fn to_variant<'py>(&self, py: Python<'py>) -> PyResult<Bound<'py, PyAny>> {
self.clone().into_bound_py_any(py)
}
}
6 changes: 3 additions & 3 deletions src/expr/analyze.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
// under the License.

use datafusion::logical_expr::logical_plan::Analyze;
use pyo3::prelude::*;
use pyo3::{prelude::*, IntoPyObjectExt};
use std::fmt::{self, Display, Formatter};

use super::logical_node::LogicalNode;
Expand Down Expand Up @@ -78,7 +78,7 @@ impl LogicalNode for PyAnalyze {
vec![PyLogicalPlan::from((*self.analyze.input).clone())]
}

fn to_variant(&self, py: Python) -> PyResult<PyObject> {
Ok(self.clone().into_py(py))
fn to_variant<'py>(&self, py: Python<'py>) -> PyResult<Bound<'py, PyAny>> {
self.clone().into_bound_py_any(py)
}
}
6 changes: 3 additions & 3 deletions src/expr/create_memory_table.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
use std::fmt::{self, Display, Formatter};

use datafusion::logical_expr::CreateMemoryTable;
use pyo3::prelude::*;
use pyo3::{prelude::*, IntoPyObjectExt};

use crate::sql::logical::PyLogicalPlan;

Expand Down Expand Up @@ -91,7 +91,7 @@ impl LogicalNode for PyCreateMemoryTable {
vec![PyLogicalPlan::from((*self.create.input).clone())]
}

fn to_variant(&self, py: Python) -> PyResult<PyObject> {
Ok(self.clone().into_py(py))
fn to_variant<'py>(&self, py: Python<'py>) -> PyResult<Bound<'py, PyAny>> {
self.clone().into_bound_py_any(py)
}
}
6 changes: 3 additions & 3 deletions src/expr/create_view.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
use std::fmt::{self, Display, Formatter};

use datafusion::logical_expr::{CreateView, DdlStatement, LogicalPlan};
use pyo3::prelude::*;
use pyo3::{prelude::*, IntoPyObjectExt};

use crate::{errors::py_type_err, sql::logical::PyLogicalPlan};

Expand Down Expand Up @@ -88,8 +88,8 @@ impl LogicalNode for PyCreateView {
vec![PyLogicalPlan::from((*self.create.input).clone())]
}

fn to_variant(&self, py: Python) -> PyResult<PyObject> {
Ok(self.clone().into_py(py))
fn to_variant<'py>(&self, py: Python<'py>) -> PyResult<Bound<'py, PyAny>> {
self.clone().into_bound_py_any(py)
}
}

Expand Down
6 changes: 3 additions & 3 deletions src/expr/distinct.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
use std::fmt::{self, Display, Formatter};

use datafusion::logical_expr::Distinct;
use pyo3::prelude::*;
use pyo3::{prelude::*, IntoPyObjectExt};

use crate::sql::logical::PyLogicalPlan;

Expand Down Expand Up @@ -89,7 +89,7 @@ impl LogicalNode for PyDistinct {
}
}

fn to_variant(&self, py: Python) -> PyResult<PyObject> {
Ok(self.clone().into_py(py))
fn to_variant<'py>(&self, py: Python<'py>) -> PyResult<Bound<'py, PyAny>> {
self.clone().into_bound_py_any(py)
}
}
Loading