HyukjinKwon commented on PR #57078:
URL: https://github.com/apache/spark/pull/57078#issuecomment-4902215408
## Code review summary
Reviewed the full diff. **No blocking findings** — this is a well-scoped,
low-risk pandas 3 compatibility change. The main-code fixes are correct and
localized, and the doctest/test adjustments are limited to genuine,
semantically-equivalent pandas 3 behavior changes.
### Verified correct
- **`window.py` (`sem`)** — `std(ddof) =
sqrt(var_samp·(count-1)/(count-ddof))`, then `/ sqrt(count)`. Since Spark's
`var_samp = SS/(count-1)`, the `(count-1)` cancels to `SS/(count-ddof)` =
variance with the `ddof` divisor, so any `ddof` is honored. The `count > ddof`
guard (unchanged from the pandas<3 branch) protects the `count-ddof`
denominator.
- **`internal.py` (`restore_index`)** — `is_non_default_range = start != 0
or step != 1` correctly separates a genuine default `RangeIndex` from a
materialized integer index. The `ps.Index([0, 1, 2])` (contiguous-from-zero)
case that's indistinguishable from a default index is acknowledged in the PR
description and handled via `+SKIP`.
- **`types.py` / `typehints.py`** — the two `future.infer_string` gates are
logically consistent (both require pandas≥3 **and** the option enabled), and
each short-circuits so `pd.get_option("future.infer_string")` is never
evaluated on pandas<3 — no `KeyError` on older pandas.
- **`test_pyarrow_arrow_to_pandas_default.py`** — the `overrides=` kwarg is
pre-existing infrastructure on `master` (signature + consumer both present), so
passing it here is safe.
- Doctest-heavy files
(`frame`/`generic`/`series`/`namespace`/`datetimes`/`indexes/*`) contain **no
hidden logic changes** — all additions are `[...]` / `+ELLIPSIS` / `+SKIP`
output edits.
### Minor / optional (non-blocking)
- **`window.py`** — the pandas≥3 and pandas<3 branches now duplicate the
`sem` structure. Fine as-is given the version divergence; if the pandas<3
support window is short, a follow-up could collapse them once the minimum is
bumped (same lifecycle as the `__init__.py` `TODO`).
- **`internal.py`** — for an empty explicit index (`ps.Index([])`),
`pdf.index` is `RangeIndex(0, 0, 1)` → not reconstructed → stays an empty
`RangeIndex` rather than an empty `Index`. Edge case, indistinguishable at
conversion time, and consistent with the documented default-vs-materialized
limitation — noting only for completeness.
### Housekeeping (already tracked by the author)
- `[SPARK-XXXXX]` placeholders in the title and `TODO(SPARK-XXXXX)` need a
real ticket before finalizing.
- The `[TEMP][DO-NOT-MERGE]` fork-only workflow
(`.github/workflows/zz_fork_pandas3_verify.yml`) is to be dropped before
review, per the PR note. It contains no secrets or risky patterns.
LGTM once the draft/DO-NOT-MERGE scaffolding is removed and the JIRA ID is
filled in.
<!-- ai-code-review -->
--
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]
