andygrove commented on code in PR #1763:
URL:
https://github.com/apache/datafusion-python/pull/1763#discussion_r4138317837
##########
crates/core/src/expr.rs:
##########
@@ -678,8 +705,8 @@ impl PyExpr {
null_treatment,
Review Comment:
1b018456 keeps the direction for the aggregate, but this arm still builds
the window from `agg_fn.func` and `agg_fn.params.args` only (lines 695-698), so
the WITHIN GROUP ordering is dropped again and a descending percentile used as
a window silently returns the ascending result. The `windows.md` workaround
(pass `order_by` in the `Window`) can't bring it back, since a window never
hands its ORDER BY to the accumulator:
```python
from datafusion import SessionContext, col, functions as f
from datafusion.expr import Window, WindowFrame
ctx = SessionContext()
df = ctx.from_pydict({"a": [1.0, 2.0, 3.0, 4.0, 5.0]}, name="t")
desc = col("a").sort(ascending=False)
whole = WindowFrame("rows", None, None)
print(df.aggregate([], [f.percentile_cont(desc,
0.25).alias("p")]).to_pydict())
# {'p': [4.0]}
print(df.select(f.percentile_cont(desc,
0.25).over(Window()).alias("p")).to_pydict())
# {'p': [2.0, 2.0, 2.0, 2.0, 2.0]}
print(df.select(
f.percentile_cont(desc, 0.25).over(Window(order_by=[desc],
window_frame=whole)).alias("p")
).to_pydict())
# {'p': [2.0, 2.0, 2.0, 2.0, 2.0]}
ctx.sql("SELECT percentile_cont(0.25) WITHIN GROUP (ORDER BY a DESC) OVER ()
FROM t")
# Error during planning: OVER and WITHIN GROUP clause cannot be used
together. ...
```
`quantile_cont` gives the same, and `approx_percentile_cont` gives 4.25 as
an aggregate vs 1.75 over a window. SQL also rejects a plain aggregate ORDER BY
with OVER ("Aggregate ORDER BY is not implemented for window functions"), so
raising here when `agg_fn.params.order_by` is non-empty would match SQL and
turn this, and the `order_by` part of #1764, into an error instead of a wrong
answer. The upgrade guide's "They now match `WITHIN GROUP (ORDER BY ... DESC)`"
could also say it's for aggregates only.
##########
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:
b4cad2d2 fixed the chain I gave, but this check keys off
`params.order_by.is_empty()`, and decoding (copy, pickle, `from_bytes`) runs
upstream's `regularize_order_bys`, which adds `ORDER BY UInt64(1)` to a RANGE
frame that has no order-by. After a round trip the frame no longer looks like
the default, so it's kept, and the copy chains differently from the original:
```python
import copy
import pickle
from datafusion import Expr, SessionContext, col, functions as f
from datafusion.expr import Window
ctx = SessionContext()
df = ctx.from_pydict({"i": [1, 1, 2, 3], "v": [1, 2, 3, 4]})
def run(e):
r = e.over(Window(order_by="i")).alias("r")
return df.select(col("v"),
r).sort(col("v")).collect_column("r").to_pylist()
base = f.sum(col("v")).over(Window(order_by=[]))
print(run(base)) # [1, 3, 6, 10]
print(run(copy.copy(base))) # [3, 3, 6, 10]
print(run(pickle.loads(pickle.dumps(base)))) # [3, 3, 6, 10]
print(run(Expr.from_bytes(base.to_bytes()))) # [3, 3, 6, 10]
print(copy.copy(base))
# Expr(sum(v) ORDER BY [UInt64(1) ASC NULLS LAST] RANGE BETWEEN UNBOUNDED
PRECEDING AND CURRENT ROW)
```
On main all four give `[1, 3, 6, 10]`. `windows.md` promises this case ("a
copy, a pickled expression sent to a worker, or one parsed from SQL all chain
the same way"), but
`test_window_builder_default_frame_same_after_copy_and_pickle` only covers
`WindowFrame("rows", None, None)`. The check probably needs to treat an
`order_by` holding only that literal sort key the same as an empty one.
##########
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:
607dde49 rejects `show_statistics=True` with `analyze`, but
`show_statistics=False` is still silently ignored (Analyze always uses the
session setting), and `analyze_level`/`analyze_categories` are silently ignored
without `analyze`. SQL rejects all three:
```python
from datafusion import SessionConfig, SessionContext, col, lit
from datafusion.dataframe import ExplainAnalyzeLevel, ExplainMetricCategory
ctx =
SessionContext(SessionConfig().set("datafusion.explain.show_statistics",
"true"))
df = ctx.from_pydict({"a": [1, 2, 3]}, name="t").filter(col("a") > lit(1))
df.explain(analyze=True, show_statistics=False)
# no error; the plan still has statistics=[Rows=Inexact(3), ...]
df.explain(analyze_level=ExplainAnalyzeLevel.DEV)
# no error; plain plan, no metrics
df.explain(analyze_categories=[ExplainMetricCategory.ROWS])
# no error; plain plan, no metrics
ctx.sql("EXPLAIN (ANALYZE, COSTS OFF) SELECT a FROM t WHERE a > 1")
# Error during planning: EXPLAIN option COSTS cannot be combined with ANALYZE
ctx.sql("EXPLAIN (LEVEL dev) SELECT a FROM t WHERE a > 1")
# Error during planning: EXPLAIN option LEVEL requires ANALYZE
ctx.sql("EXPLAIN (METRICS 'rows') SELECT a FROM t WHERE a > 1")
# Error during planning: EXPLAIN option METRICS requires ANALYZE
```
Checking `show_statistics is not None`, and raising when `analyze_level` or
`analyze_categories` is set without `analyze`, would match.
##########
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:
92ace16b restores the error for the first call, but the check only runs
there. `filter()` has no check, and the chained `PyExprFuncBuilder` methods
have none either, so the same option later in a chain is still dropped. The PR
description says "A builder option that does not apply to the function's kind
still raises":
```python
from datafusion import SessionContext, col, lit, functions as f
ctx = SessionContext()
df = ctx.from_pydict({"g": [1, 1, 2, 2], "v": [1, 2, 3, 4]})
f.lead(col("v"), 1, order_by="v").distinct().build()
# ExprFunctionExt can only be used with Expr::AggregateFunction or
Expr::WindowFunction
e = f.lead(col("v"), 1,
order_by="v").partition_by(col("g")).distinct().build()
print(e) # Expr(lead(DISTINCT v, Int64(1), NULL) PARTITION BY [g] ORDER BY
[v ASC NULLS FIRST] ...)
print(df.select(col("v"),
e.alias("r")).sort(col("v")).collect_column("r").to_pylist())
# [2, None, 4, None], DISTINCT ignored
e = f.sum(col("v")).filter(col("v") > lit(1)).partition_by(col("g")).build()
print(df.aggregate([], [e.alias("s")]).to_pydict())
# {'s': [9]}, partition_by ignored
e = f.row_number(order_by=[col("v")]).filter(col("v") > lit(2)).build()
# main: raises here; PR: builds
df.select(e.alias("r"))
# Error during planning: FILTER clause can only be used with aggregate
window functions. ...
```
The two chained cases drop the option on main too, so only `filter()` is a
regression, and it still errors, just later. Recording the function kind on
`PyExprFuncBuilder` and checking there would cover every entry point with one
rule.
##########
python/datafusion/functions/__init__.py:
##########
@@ -7091,14 +7618,29 @@ def string_agg(
... ).alias("s")])
>>> result.collect_column("s")[0].as_py()
'y,z'
+
+ >>> df = ctx.from_pydict({"a": ["y", "x", "y"]})
+ >>> result = df.aggregate(
+ ... [], [dfn.functions.string_agg(
+ ... dfn.col("a"), ",", distinct=True, order_by="a",
+ ... ).alias("s")])
+ >>> result.collect_column("s")[0].as_py()
+ 'x,y'
"""
+ if not isinstance(distinct, bool):
Review Comment:
This also rejects `numpy.bool_`, which every other aggregate's `distinct`
accepts through PyO3's `Option<bool>`, and the message then says "got bool":
```python
import numpy as np
from datafusion import SessionContext, col, functions as f
df = SessionContext().from_pydict({"s": ["x", "y", "x"]})
print(df.aggregate([], [f.count(col("s"),
distinct=np.True_).alias("n")]).to_pydict())
# {'n': [2]}
f.string_agg(col("s"), ",", distinct=np.True_)
# TypeError: distinct must be a bool, got bool; pass filter and order_by by
keyword
```
A shifted positional call can only put `None` or an `Expr` here, so
rejecting just those two would keep the fix from 0ac494e6 and leave everything
else to PyO3.
##########
python/datafusion/user_defined.py:
##########
@@ -388,7 +397,9 @@ def wrapper(*args: Any, **kwargs: Any) -> Callable:
return decorator
- if hasattr(args[0], "__datafusion_scalar_udf__"):
+ if args and (
+ hasattr(args[0], "__datafusion_scalar_udf__") or
_is_pycapsule(args[0])
+ ):
return ScalarUDF.from_pycapsule(args[0])
if args and callable(args[0]):
Review Comment:
Pre-existing, and f8d7b70f fixed the keyword-only decorator form, but the
function form with the callable passed by keyword (declared by the second
`@overload`) still falls through to the decorator factory. It now fails with a
`TypeError` naming an internal helper instead of the old `IndexError`:
```python
import pyarrow as pa
import pyarrow.compute as pc
from datafusion import udf
def double(x: pa.Array) -> pa.Array:
return pc.multiply(x, 2)
udf(func=double, input_fields=[pa.int64()], return_field=pa.int64(),
volatility="immutable")
# main: IndexError: tuple index out of range
# PR: TypeError: ScalarUDF.udf.<locals>._decorator() got an unexpected
keyword argument 'func'
```
`udaf(accum=...)` and `udwf(func=...)` fail the same way. Taking `func`
(`accum` for `udaf`) from `kwargs` when `args` is empty would cover it.
##########
python/datafusion/user_defined.py:
##########
@@ -31,7 +31,12 @@
from datafusion.expr import Expr
if TYPE_CHECKING:
- from _typeshed import CapsuleType as _PyCapsule
+ import sys
+
+ if sys.version_info >= (3, 13):
+ from types import CapsuleType as _PyCapsule
Review Comment:
This fixes the alias here, but `context.py:92` and `extensions.py:52` still
import `CapsuleType` from `_typeshed`, which f6d04788's message notes doesn't
define it, so the capsule half of those unions is still `Unknown`. With pyright
1.1.414:
```text
python/datafusion/context.py:92:27 - error: "CapsuleType" is unknown import
symbol (reportAttributeAccessIssue)
python/datafusion/extensions.py:52:27 - error: "CapsuleType" is unknown
import symbol (reportAttributeAccessIssue)
```
```python
from datafusion import SessionContext
from datafusion.user_defined import ScalarUDF
ctx = SessionContext()
ctx.set_query_planner(1) # pyright: no error
ctx.with_logical_extension_codec(1) # pyright: no error
ScalarUDF.from_pycapsule(1) # pyright: "Literal[1]" is not
assignable to "CapsuleType"
```
Smaller: since `_PyCapsule` only exists under `TYPE_CHECKING`,
`typing.get_type_hints(ScalarUDF.from_pycapsule)` (and `WindowUDF`'s) now
raises `NameError: name '_PyCapsule' is not defined`; on main it resolved.
##########
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:
`not subset` takes the truth value of `subset`, so column lists from numpy
or pandas now raise here (and in `fill_nan` below). The Rust binding accepts
any sequence of strings, so these worked on main:
```python
import numpy as np
import pandas as pd
from datafusion import SessionContext
df = SessionContext().from_pydict({"a": [1, None, 3], "b": [None, 5, 6],
"c": [None, None, 9]})
cols = pd.DataFrame(columns=["a", "b"]).columns
print(df.fill_null(0, subset=cols).to_pydict())
# main: {'a': [1, 0, 3], 'b': [0, 5, 6], 'c': [None, None, 9]}
# PR: ValueError: The truth value of a Index is ambiguous. ...
print(df.fill_null(0, subset=np.array(["a", "b"])).to_pydict())
# main: {'a': [1, 0, 3], 'b': [0, 5, 6], 'c': [None, None, 9]}
# PR: ValueError: The truth value of an array with more than one element
is ambiguous. ...
```
`if subset is not None and len(subset) == 0:` keeps the new empty-subset
behavior without this.
##########
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:
Only the direction of the first `order_by` key is used. The percentile is
still computed over `sort_expression`, while the output name shows the new
column, so `order_by` doesn't replace the ordering the way this reads (same
text at 5447 and 5497):
```python
from datafusion import SessionContext, col, functions as f
ctx = SessionContext()
df = ctx.from_pydict({"a": [1.0, 2.0, 3.0, 4.0, 5.0], "b": [10.0, 20.0,
30.0, 40.0, 50.0]}, name="t")
e = f.percentile_cont(col("a"), 0.25).order_by(col("b")).build()
print(df.aggregate([], [e]).to_pydict())
# {'percentile_cont(Float64(0.25)) WITHIN GROUP [t.b ASC NULLS FIRST]':
[2.0]}
print(ctx.sql("SELECT percentile_cont(0.25) WITHIN GROUP (ORDER BY b) FROM
t").to_pydict())
# {'percentile_cont(Float64(0.25)) WITHIN GROUP [t.b ASC NULLS LAST]':
[20.0]}
```
The approx variants give 1.75 vs 17.5. The behavior isn't new, but the
docstring now invites it. Saying that `order_by` only sets the direction and
should use the same expression would be accurate.
##########
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:
A lone argument is now the upper bound, but it binds to the parameter named
`start`, which can be passed by keyword. So `range(start=5)`, which used to
raise because `stop` was missing, now silently means `stop=5`, while
`range(stop=5)` raises:
```python
from datafusion import SessionContext, col, functions as f
df = SessionContext().from_pydict({"lo": [3]})
print(df.select(f.range(start=5).alias("r")).to_pydict())
# main: TypeError: range() missing 2 required positional arguments: 'stop'
and 'step'
# PR: {'r': [[0, 1, 2, 3, 4]]}
print(df.select(f.gen_series(start=col("lo")).alias("r")).to_pydict())
# main: TypeError: gen_series() missing 1 required positional argument:
'stop'
# PR: {'r': [[0, 1, 2, 3]]}
f.range(stop=5)
# PR: TypeError: range() missing 1 required positional argument: 'start'
```
Python's `range` avoids this by not taking keywords, and `numpy.arange`
treats a lone `stop=` as the bound; either would keep a value named `start`
from becoming the end. Separately, `series_expr` in `functions.rs` chains
`(start, None, step)` into a two-argument call, so
`_internal.functions.range(lit(10).expr, None, lit(3).expr)` builds
`range(Int64(10), Int64(3))` and returns `[]`. Only `_series` guards that today.
##########
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:
Accepting `distinct()` on an aggregate window is what makes the #1764
workaround in `windows.md` work
(`f.avg(col("v")).over(Window()).distinct().build()`). But Python UDAFs ignore
`AccumulatorArgs.is_distinct` (`udaf.rs:305`), so for them the recommended
pattern silently returns the non-distinct result:
```python
import pyarrow as pa
import pyarrow.compute as pc
from datafusion import Accumulator, SessionContext, col, udaf, functions as f
from datafusion.expr import Window
class MySum(Accumulator):
def __init__(self) -> None:
self._sum = 0.0
def update(self, values: pa.Array) -> None:
self._sum += pc.sum(values).as_py() or 0.0
def merge(self, states: list[pa.Array]) -> None:
self._sum += pc.sum(states[0]).as_py() or 0.0
def state(self) -> list[pa.Scalar]:
return [pa.scalar(self._sum)]
def evaluate(self) -> pa.Scalar:
return pa.scalar(self._sum)
my_sum = udaf(MySum, pa.float64(), pa.float64(), [pa.float64()], "immutable")
df = SessionContext().from_pydict({"v": [1.0, 1.0, 1.0, 5.0]})
print(df.select(my_sum(col("v")).over(Window()).distinct().build().alias("r")).to_pydict())
# {'r': [8.0, 8.0, 8.0, 8.0]}
print(df.select(f.sum(col("v")).over(Window()).distinct().build().alias("r")).to_pydict())
# {'r': [6.0, 6.0, 6.0, 6.0]}
```
The UDAF side is pre-existing: SQL `my_sum(DISTINCT v) OVER ()` also gives
8.0, on main too. But this PR adds a new path to it and recommends that path. A
caveat in `windows.md`, or having the Python UDAF's `accumulator()` return an
error when `is_distinct` is set, would keep the workaround from giving wrong
answers.
##########
python/datafusion/functions/__init__.py:
##########
@@ -5065,8 +5493,9 @@ def approx_percentile_cont_with_weight(
This aggregate function is similar to :py:func:`approx_percentile_cont`
except that
it uses the associated associated weights.
- 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 options ``null_treatment`` and ``distinct``, and ``order_by`` replaces
the
Review Comment:
This line was rewritten, but `distinct` isn't ignored here: upstream rejects
it at execution. `count_star` (lines 961-962) makes the same kind of claim, and
there the builder does apply it:
```python
from datafusion import SessionContext, col, functions as f
df = SessionContext().from_pydict({"a": [1.0, 2.0, 2.0, 3.0], "w": [1.0,
1.0, 1.0, 1.0]})
e = f.approx_percentile_cont_with_weight(col("a"), col("w"),
0.5).distinct().build()
df.aggregate([], [e.alias("p")]).collect()
# This feature is not implemented:
approx_percentile_cont_with_weight(DISTINCT) aggregations are not available
e = f.count_star().distinct().build()
print(e) # Expr(count(DISTINCT
Int64(1)))
print(df.aggregate([], [e.alias("n")]).to_pydict()) # {'n': [1]}, not 4
```
`approx_percentile_cont` really does ignore it, so only the `_with_weight`
text is off. The `count_star` wording predates this PR.
##########
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:
+ return self
return DataFrame(self.df.fill_null(value, subset))
+ def fill_nan(self, value: float, subset: list[str] | None = None) ->
DataFrame:
+ """Fill NaN values in floating-point columns with a value.
+
+ Only floating-point columns are changed; others are kept unchanged, as
is
+ any column ``value`` cannot be cast to. NaN is distinct from null,
which
+ :py:meth:`fill_null` handles.
+
+ Args:
+ value: Value to replace NaN with. Will be cast to match column
type.
+ subset: Optional list of column names to fill. If None, fills all
+ floating-point columns; an empty list fills none.
+
+ Returns:
+ DataFrame with NaN values replaced.
+
+ Examples:
+ >>> from datafusion import SessionContext
+ >>> ctx = SessionContext()
+ >>> nan = float("nan")
+ >>> df = ctx.from_pydict({"a": [1.0, nan, None], "b": [nan, 2.0,
3.0]})
+ >>> df.fill_nan(0.0).to_pydict()
+ {'a': [1.0, 0.0, None], 'b': [0.0, 2.0, 3.0]}
+
+ >>> df.fill_nan(0.0, subset=["a"]).collect_column("b")[0].as_py()
+ nan
+
+ >>> df.fill_nan(0.0, subset=[]).collect_column("b")[0].as_py()
+ nan
+ """
+ if subset is not None and not subset:
+ return self
+ return DataFrame(self.df.fill_nan(value, subset))
Review Comment:
On apache/datafusion#25829: leaving the wrapper as-is makes sense, but right
now the limitation is only in the PR description. It hits any DataFrame with
such a column, even when that column isn't in `subset` or isn't a float, so a
note in the guide with a one-line pointer from the `fill_nan`/`fill_null`
docstrings would save people some confusion:
```python
from datafusion import SessionContext
nan = float("nan")
df = SessionContext().from_pydict({"Price": [nan, 2.0], "qty": [nan, 1.0]})
df.fill_nan(0.0, subset=["qty"])
# Schema error: No field named price. Did you mean '..."Price"'?
```
##########
docs/source/user-guide/upgrade-guides.md:
##########
@@ -198,6 +198,113 @@ ctx.execute(plan, partitions=0) # before
ctx.execute(plan, partition=0) # after
```
+### More aggregate functions accept `distinct`
+
+{py:func}`~datafusion.functions.bit_and`,
+{py:func}`~datafusion.functions.bit_or`,
+{py:func}`~datafusion.functions.mean`,
+{py:func}`~datafusion.functions.percentile_cont`,
+{py:func}`~datafusion.functions.quantile_cont`, and
+{py:func}`~datafusion.functions.string_agg` now accept a `distinct` argument.
+As with `sum` and `avg` in 54.0.0, `distinct` is inserted *before* `filter`, so
+code that passed `filter` (or, for `string_agg`, `order_by`) positionally must
+pass it by keyword.
+
+```python
+f.bit_and(column("a"), my_filter) # before
+f.bit_and(column("a"), filter=my_filter) # after
+```
+
+Passing `filter` to `mean` previously raised a `TypeError`, whether passed
Review Comment:
Nit: "whether passed positionally or by keyword; it now works" reads as if
both now work, but a positional `filter` lands in the new `distinct` and still
raises:
```python
from datafusion import SessionContext, col, lit, functions as f
df = SessionContext().from_pydict({"v": [1.0, 2.0, 3.0]})
f.mean(col("v"), col("v") > lit(1.0))
# TypeError: 'Expr' object is not an instance of 'bool'
print(df.aggregate([], [f.mean(col("v"), filter=col("v") >
lit(1.0)).alias("m")]).to_pydict())
# {'m': [2.5]}
```
"it now works when passed by keyword" would match.
--
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]
