timsaucer commented on code in PR #1763:
URL:
https://github.com/apache/datafusion-python/pull/1763#discussion_r4124534423
##########
crates/core/src/expr.rs:
##########
@@ -624,44 +624,68 @@ impl PyExpr {
// Expression Function Builder functions
- pub fn order_by(&self, order_by: Vec<PySortExpr>) -> PyExprFuncBuilder {
- self.expr
- .clone()
+ #[pyo3(signature = (order_by, keep_window_frame=false))]
+ pub fn order_by(
+ &self,
+ order_by: Vec<PySortExpr>,
+ keep_window_frame: bool,
+ ) -> PyExprFuncBuilder {
+ builder_from_expr(&self.expr, keep_window_frame)
.order_by(to_sort_expressions(order_by))
.into()
}
- pub fn filter(&self, filter: PyExpr) -> PyExprFuncBuilder {
- self.expr.clone().filter(filter.expr.clone()).into()
+ #[pyo3(signature = (filter, keep_window_frame=false))]
+ pub fn filter(&self, filter: PyExpr, keep_window_frame: bool) ->
PyExprFuncBuilder {
+ builder_from_expr(&self.expr, keep_window_frame)
+ .filter(filter.expr.clone())
+ .into()
}
- pub fn distinct(&self) -> PyExprFuncBuilder {
- self.expr.clone().distinct().into()
+ #[pyo3(signature = (keep_window_frame=false))]
+ pub fn distinct(&self, keep_window_frame: bool) -> PyExprFuncBuilder {
+ builder_from_expr(&self.expr, keep_window_frame)
+ .distinct()
+ .into()
}
- pub fn null_treatment(&self, null_treatment: NullTreatment) ->
PyExprFuncBuilder {
- self.expr
- .clone()
+ #[pyo3(signature = (null_treatment, keep_window_frame=false))]
+ pub fn null_treatment(
+ &self,
+ null_treatment: NullTreatment,
+ keep_window_frame: bool,
+ ) -> PyExprFuncBuilder {
+ builder_from_expr(&self.expr, keep_window_frame)
.null_treatment(Some(null_treatment.into()))
.into()
}
- pub fn partition_by(&self, partition_by: Vec<PyExpr>) -> PyExprFuncBuilder
{
+ #[pyo3(signature = (partition_by, keep_window_frame=false))]
+ pub fn partition_by(
+ &self,
+ partition_by: Vec<PyExpr>,
+ keep_window_frame: bool,
+ ) -> PyExprFuncBuilder {
let partition_by = partition_by.iter().map(|e|
e.expr.clone()).collect();
- self.expr.clone().partition_by(partition_by).into()
+ builder_from_expr(&self.expr, keep_window_frame)
+ .partition_by(partition_by)
+ .into()
}
pub fn window_frame(&self, window_frame: PyWindowFrame) ->
PyExprFuncBuilder {
- self.expr.clone().window_frame(window_frame.into()).into()
+ builder_from_expr(&self.expr, false)
+ .window_frame(window_frame.into())
+ .into()
}
- #[pyo3(signature = (partition_by=None, window_frame=None, order_by=None,
null_treatment=None))]
+ #[pyo3(signature = (partition_by=None, window_frame=None, order_by=None,
null_treatment=None, keep_window_frame=false))]
pub fn over(
&self,
partition_by: Option<Vec<PyExpr>>,
window_frame: Option<PyWindowFrame>,
order_by: Option<Vec<PySortExpr>>,
null_treatment: Option<NullTreatment>,
+ keep_window_frame: bool,
) -> PyDataFusionResult<PyExpr> {
match &self.expr {
Expr::AggregateFunction(agg_fn) => {
Review Comment:
Fixed in 8e21c8ad: `docs: note that over() drops options set on an aggregate`
##########
crates/core/src/expr.rs:
##########
@@ -743,6 +767,63 @@ impl PyExpr {
}
}
+/// Start an [`ExprFuncBuilder`] that keeps the options already set on `expr`.
+///
+/// Upstream's `ExprFunctionExt` methods on an `Expr` start from an empty
+/// builder, so `build()` would reset every option not set again. The Python
+/// function wrappers already apply their keyword options, so chaining another
+/// builder method onto their result must not discard them.
+///
+/// A built window function always stores a concrete frame, so whether the user
+/// chose it is lost. `keep_window_frame` carries that from the Python side;
when
+/// false, a frame equal to the default for the current order-by is treated as
+/// unset.
+fn builder_from_expr(expr: &Expr, keep_window_frame: bool) -> ExprFuncBuilder {
+ match expr {
+ Expr::AggregateFunction(agg) => {
+ let params = &agg.params;
+ let mut builder =
expr.clone().null_treatment(params.null_treatment);
+ if !params.order_by.is_empty() {
+ builder = builder.order_by(params.order_by.clone());
+ }
+ if let Some(filter) = ¶ms.filter {
+ builder = builder.filter(filter.as_ref().clone());
+ }
+ if params.distinct {
+ builder = builder.distinct();
+ }
+ builder
+ }
+ Expr::WindowFunction(window) => {
+ let params = &window.params;
+ let mut builder =
expr.clone().null_treatment(params.null_treatment);
+ if !params.partition_by.is_empty() {
+ builder = builder.partition_by(params.partition_by.clone());
+ }
+ let has_order_by = !params.order_by.is_empty();
+ if has_order_by {
+ builder = builder.order_by(params.order_by.clone());
+ }
+ // A frame equal to the default `build()` derived from the
order-by is
+ // left unset, so it is derived again from the final order-by.
+ if keep_window_frame
+ || params.window_frame
+ !=
datafusion::logical_expr::WindowFrame::new(has_order_by.then_some(true))
Review Comment:
Fixed in b4cad2d2: `fix: treat the frame an empty order_by derives as a
default when chaining`
##########
python/datafusion/expr.py:
##########
@@ -453,6 +453,10 @@ class Expr: # noqa: PLW1641
:ref:`Expressions` in the online documentation for more information.
"""
+ # Set by ``over()`` when the window frame was given explicitly, so
chaining a
+ # builder method keeps it even if it equals the default frame.
+ _explicit_window_frame = False
Review Comment:
Fixed in d7312799: `fix: decide window frame re-derivation from the
expression alone`
##########
python/datafusion/dataframe.py:
##########
@@ -1217,6 +1246,13 @@ def explain(
analyze: If ``True``, the plan will run and metrics reported.
format: Output format for the plan. Defaults to
:py:attr:`ExplainFormat.INDENT`.
+ show_statistics: If ``True``, include each operator's statistics.
Review Comment:
Fixed in 607dde49: `fix: reject show_statistics combined with analyze in
explain`
##########
python/datafusion/functions/spark.py:
##########
@@ -1520,6 +1711,15 @@ def format_string(format: str | Expr, *cols: Expr) ->
Expr:
return Expr(_f.format_string(fmt_expr.expr, *[c.expr for c in cols]))
+def printf(format: str | Expr, *cols: Expr) -> Expr:
Review Comment:
Fixed in 4933fa28: `fix: treat a bare str as a column name in spark.printf`
##########
crates/core/src/udf.rs:
##########
@@ -209,6 +209,16 @@ impl ScalarUDFImpl for PythonFunctionScalarUDF {
}
}
+fn scalar_udf_from_capsule(capsule: &Bound<'_, PyCapsule>) ->
PyDataFusionResult<ScalarUDF> {
Review Comment:
Fixed in 23ed533d: `fix: name the expected and found capsule when importing
a UDF`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
