chrikrah opened a new issue, #55372:
URL: https://github.com/apache/arrow/issues/55372
### Describe the bug, including details regarding any error messages,
version, and platform.
`pyarrow.compute.tdigest` and `hash_tdigest` accept a `q` outside [0, 1] and
return `NaN`, while `quantile` rejects the same value with `ArrowInvalid`.
`TDigestOptions` documents the range as a requirement.
```python
import pyarrow as pa
import pyarrow.compute as pc
print(pa.__version__)
a = pa.array([1.0, 2.0, 3.0])
for fn in (pc.quantile, pc.tdigest):
try:
print(fn.__name__, fn(a, q=[0.5, 1.5]).to_pylist())
except pa.ArrowInvalid as e:
print(fn.__name__, type(e).__name__, e)
t = pa.table({"k": [1, 1, 1], "v": [1.0, 2.0, 3.0]})
agg = t.group_by("k").aggregate([("v", "tdigest",
pc.TDigestOptions(q=[7.0]))])
print(agg.to_pylist())
```
```
$ python -I td.py # pyarrow 26.0.0 wheel, Python 3.12, Linux x86_64
26.0.0
quantile ArrowInvalid Quantile must be between 0 and 1
tdigest [2.0, nan]
[{'k': 1, 'v_tdigest': [nan]}]
```
contract: `TDigestOptions.q` in `python/pyarrow/_compute.pyx:2388` ("All
values must be in [0, 1]"), and `cpp/src/arrow/compute/api_aggregate.h`
("probability level of quantile must be between 0 and 1 inclusive").
`CheckQuantileOptions`
(`cpp/src/arrow/compute/kernels/aggregate_quantile.cc:482`, `a99464f` on main)
returns `Status::Invalid("Quantile must be between 0 and 1")` for any `q`
outside the range. Neither `aggregate_tdigest.cc` nor `GroupedTDigestImpl` in
`hash_aggregate_numeric.cc` (the separate `hash_tdigest` kernel) checks `q`,
and `TDigest::Quantile` in `cpp/src/arrow/util/tdigest.cc` returns `NAN` for `q
< 0 || q > 1`.
Expected: `ArrowInvalid` for any `q` outside [0, 1], from a shared check
called by both the `tdigest` and `hash_tdigest` inits, with the message
`quantile` uses.
I read the source at `a99464f` and ran the 26.0.0 wheel, not a source build.
I did not check how `q=NaN` should behave: `quantile` also lets it through.
I can open a PR with the check and tests in `test_compute.py`,
`kernels/aggregate_test.cc` and `acero/hash_aggregate_test.cc`. Would you take
it?
### Component(s)
C++, Python
--
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]
