timsaucer commented on code in PR #1763:
URL:
https://github.com/apache/datafusion-python/pull/1763#discussion_r4148553979
##########
crates/core/src/expr.rs:
##########
@@ -678,8 +705,8 @@ impl PyExpr {
null_treatment,
Review Comment:
Fixed in 2fb7cce4: `fix: keep aggregate options in over() and reject an
aggregate order_by`. `over()` now keeps `filter`, `distinct`, and
`null_treatment`, and an aggregate `order_by` raises, matching SQL. A `WITHIN
GROUP` function accepts an ascending `sort_expression` and raises on a
descending one, so the descending percentile case is now an error instead of a
wrong answer. The upgrade guide says the percentile change applies to
aggregates, and 4d662570 (`docs: show how to move an aggregate's order_by into
the Window`) adds a before/after for the #1764 case.
##########
crates/core/src/expr.rs:
##########
@@ -743,6 +770,69 @@ 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. A frame equal to the default for the current order-by is
+/// treated as unset, which depends only on the expression and so behaves the
+/// same after a copy, pickle, or round trip through protobuf or SQL.
+fn builder_from_expr(expr: &Expr) -> 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. An
+ // absent and an empty order-by derive different frames but are
both
+ // stored as empty, so either frame counts as the default here.
+ let is_default_frame = if has_order_by {
Review Comment:
Fixed in 99f506d0: `fix: treat the sort key added when decoding a RANGE
frame as no order_by`
##########
python/datafusion/dataframe.py:
##########
@@ -1868,13 +1925,51 @@ def fill_null(self, value: Any, subset: list[str] |
None = None) -> DataFrame:
>>> filled.sort(col("a")).collect()[0].column("a").to_pylist()
[0, 1, 3]
+ >>> df.fill_null(0, subset=[]).to_pydict()
+ {'a': [1, None, 3], 'b': [None, 5, 6]}
+
Notes:
- Only fills nulls in columns where the value can be cast to the
column type
- For columns where casting fails, the original column is kept
unchanged
- For columns not in subset, the original column is kept unchanged
"""
+ if subset is not None and not subset:
Review Comment:
Fixed in e8117588: `fix: accept numpy and pandas column lists as fill_null
and fill_nan subsets`
##########
crates/core/src/expr.rs:
##########
@@ -625,34 +625,61 @@ impl PyExpr {
// Expression Function Builder functions
pub fn order_by(&self, order_by: Vec<PySortExpr>) -> PyExprFuncBuilder {
- self.expr
- .clone()
+ builder_from_expr(&self.expr)
.order_by(to_sort_expressions(order_by))
.into()
}
pub fn filter(&self, filter: PyExpr) -> PyExprFuncBuilder {
Review Comment:
Fixed in 7dd2658f: `fix: check the function kind on every builder call, not
just the first`. The function kind is recorded on `PyExprFuncBuilder` and
checked on every call, including `filter()`.
##########
python/datafusion/functions/__init__.py:
##########
@@ -5110,19 +5539,22 @@ def approx_percentile_cont_with_weight(
def percentile_cont(
sort_expression: Expr | SortExpr,
percentile: float,
+ distinct: bool = False,
filter: Expr | None = None,
) -> Expr:
"""Computes the exact percentile of input values using continuous
interpolation.
Unlike :py:func:`approx_percentile_cont`, this function computes the exact
percentile value rather than an approximation.
- If using the builder functions described in ref:`_aggregation` this
function ignores
- the options ``order_by``, ``null_treatment``, and ``distinct``.
+ If using the builder functions described in :ref:`aggregation` this
function ignores
+ the option ``null_treatment``, and ``order_by`` replaces the ordering of
Review Comment:
Fixed in daac7f28: `docs: say a chained order_by sets only the percentile's
sort direction`
##########
python/datafusion/dataframe.py:
##########
@@ -1229,9 +1269,25 @@ def explain(
Show plan with runtime metrics:
>>> df.explain(analyze=True) # doctest: +SKIP
+
+ Show only row-count metrics:
+
+ >>> from datafusion.dataframe import ExplainMetricCategory
+ >>> df.explain(
+ ... analyze=True,
analyze_categories=[ExplainMetricCategory.ROWS]
+ ... ) # doctest: +SKIP
"""
+ if analyze and show_statistics:
Review Comment:
Fixed in 1af877f2: `fix: reject explain options that the chosen plan would
ignore`
##########
python/datafusion/functions/__init__.py:
##########
@@ -2973,18 +3126,57 @@ def array(*args: Expr) -> Expr:
return make_array(*args)
-def range(start: Expr, stop: Expr, step: Expr) -> Expr:
- """Create a list of values in the range between start and stop.
+def _series(
+ fn: Callable[..., Any],
+ name: str,
+ start: Expr | int,
+ stop: Expr | int | None,
+ step: Expr | int | None,
+) -> Expr:
+ if stop is None and step is not None:
+ msg = f"{name}() requires stop when step is given"
+ raise ValueError(msg)
+ stop = coerce_to_expr_or_none(stop)
+ step = coerce_to_expr_or_none(step)
+ return Expr(
+ fn(
+ coerce_to_expr(start).expr,
+ stop.expr if stop is not None else None,
+ step.expr if step is not None else None,
+ )
+ )
+
+
+def range(
Review Comment:
Fixed in 473856ea: `fix: take a lone range or gen_series argument only by
position`. A lone argument is taken only by position, like the built-in
`range`, so `range(start=5)` raises. `series_expr` also rejects a step without
a stop.
##########
crates/core/src/expr.rs:
##########
@@ -625,34 +625,61 @@ impl PyExpr {
// Expression Function Builder functions
pub fn order_by(&self, order_by: Vec<PySortExpr>) -> PyExprFuncBuilder {
- self.expr
- .clone()
+ builder_from_expr(&self.expr)
.order_by(to_sort_expressions(order_by))
.into()
}
pub fn filter(&self, filter: PyExpr) -> PyExprFuncBuilder {
- self.expr.clone().filter(filter.expr.clone()).into()
+ builder_from_expr(&self.expr)
+ .filter(filter.expr.clone())
+ .into()
}
pub fn distinct(&self) -> PyExprFuncBuilder {
- self.expr.clone().distinct().into()
+ // Only aggregates support DISTINCT, including an aggregate run as a
window
+ // function. For anything else, upstream's empty builder makes
`build()`
+ // raise instead of dropping the option.
+ let supports_distinct = match &self.expr {
+ Expr::AggregateFunction(_) => true,
+ Expr::WindowFunction(window) => {
+ matches!(window.fun, WindowFunctionDefinition::AggregateUDF(_))
Review Comment:
Fixed in 74b64348: `fix: reject DISTINCT in Python aggregate UDFs instead of
ignoring it`. The Python UDAF's `accumulator()` now raises when `is_distinct`
is set. A query the optimizer rewrites to group by the distinct values, such as
SQL `my_sum(DISTINCT v)`, still runs and gives the distinct result.
--
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]
