Support pl.Expr.dt.millennium - #23154
Conversation
github-actions
Bot
added
Python
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds millennium support to ChangesMillennium/Century Computation
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
python/cudf_polars/cudf_polars/dsl/expressions/datetime.py (1)
136-140: 🎯 Functional Correctness | 🔵 Trivial | 🏗️ Heavy lift"Century" is named/documented but not actually supported.
The mapping is named
_CENTURY_MILLENNIUM_DIVISORand its comment describes deriving "century/millennium",Name.Centuryexists in the enum, and Polars exposespl.Expr.dt.century()(returningInt32, e.g. year 2000 → 20, 2001 → 21). However onlyName.Millenniumis registered here and in_valid_ops, so.dt.century()on the GPU engine raisesNotImplementedErrorand silently falls back to CPU.Given the arithmetic branch is already generic over the divisor, enabling century is low effort. Have you considered wiring it in now, or otherwise narrowing the naming/comments to millennium only to avoid implying support that isn't there?
♻️ Enable century alongside millennium
_CENTURY_MILLENNIUM_DIVISOR: ClassVar[dict[Name, int]] = { + Name.Century: 100, Name.Millennium: 1_000, }and register it:
Name.Truncate, + Name.Century, Name.Millennium,Confirm
translate.pydoes not requireName.Centuryin itsneeds_castset (it currently omits both Century and Millennium, which is correct only if libcudf returnsINT32matching Polars).Also applies to: 153-153
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/cudf_polars/cudf_polars/dsl/expressions/datetime.py` around lines 136 - 140, The datetime extraction mapping currently advertises century/millennium support but only registers Millennium, so dt.century() still falls back. Update the datetime expression handling in datetime.py by adding Name.Century to the divisor mapping and the valid ops registration, reusing the same arithmetic branch as Millennium; if you choose not to support it, rename the mapping/comment to millennium-only and remove Century references so the API contract matches behavior. Also verify translate.py still does not need a cast entry for Name.Century or Name.Millennium.python/cudf_polars/tests/expressions/test_datetime_basic.py (1)
216-234: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueGood edge coverage; single-value parametrize is a smell.
The tests cover null handling and the century/millennium boundary years (1900/1901, 2000/2001), which is solid. However
@pytest.mark.parametrize("method", ["millennium"])with a single value adds no coverage and suggests"century"was intended to be included. Once century is supported (see the datetime.py comment), extend the list; otherwise drop the parametrize.♻️ Once century is supported
-@pytest.mark.parametrize("method", ["millennium"]) +@pytest.mark.parametrize("method", ["century", "millennium"])Also applies to: 237-252
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/cudf_polars/tests/expressions/test_datetime_basic.py` around lines 216 - 234, The test_century_millennium parametrization is a no-op with only "millennium", so either remove the single-value pytest.mark.parametrize("method", ...) if only millennium should be tested, or expand it to include "century" once supported. Update the test_century_millennium case to cover the intended dt accessor methods consistently and avoid redundant parametrization.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@python/cudf_polars/cudf_polars/dsl/expressions/datetime.py`:
- Around line 136-140: The datetime extraction mapping currently advertises
century/millennium support but only registers Millennium, so dt.century() still
falls back. Update the datetime expression handling in datetime.py by adding
Name.Century to the divisor mapping and the valid ops registration, reusing the
same arithmetic branch as Millennium; if you choose not to support it, rename
the mapping/comment to millennium-only and remove Century references so the API
contract matches behavior. Also verify translate.py still does not need a cast
entry for Name.Century or Name.Millennium.
In `@python/cudf_polars/tests/expressions/test_datetime_basic.py`:
- Around line 216-234: The test_century_millennium parametrization is a no-op
with only "millennium", so either remove the single-value
pytest.mark.parametrize("method", ...) if only millennium should be tested, or
expand it to include "century" once supported. Update the
test_century_millennium case to cover the intended dt accessor methods
consistently and avoid redundant parametrization.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: a9fab8d4-3a29-462f-8411-aa9257a61429
📒 Files selected for processing (2)
python/cudf_polars/cudf_polars/dsl/expressions/datetime.pypython/cudf_polars/tests/expressions/test_datetime_basic.py
|
/merge |
3 participants
Description
xref #23151
Checklist