Support cudf-polars total_xxx datetime extraction methods - #18171
Support cudf-polars total_xxx datetime extraction methods#18171rapids-bot[bot] merged 28 commits into
cudf-polars total_xxx datetime extraction methods#18171Conversation
github-actions
Bot
added
Python
brandon-b-miller
added
feature request
| millis = plc.datetime.extract_datetime_component( | ||
| column.obj, plc.datetime.DatetimeComponent.MILLISECOND | ||
| ) | ||
| micros = plc.datetime.extract_datetime_component( | ||
| column.obj, plc.datetime.DatetimeComponent.MICROSECOND | ||
| ) | ||
| nanos = plc.datetime.extract_datetime_component( | ||
| column.obj, plc.datetime.DatetimeComponent.NANOSECOND | ||
| ) | ||
| millis_as_nanos = plc.binaryop.binary_operation( | ||
| millis, | ||
| plc.interop.from_arrow(pa.scalar(1_000_000, type=pa.int32())), | ||
| plc.binaryop.BinaryOperator.MUL, | ||
| plc.types.DataType(plc.types.TypeId.INT32), | ||
| ) | ||
| micros_as_nanos = plc.binaryop.binary_operation( | ||
| micros, | ||
| plc.interop.from_arrow(pa.scalar(1_000, type=pa.int32())), | ||
| plc.binaryop.BinaryOperator.MUL, | ||
| plc.types.DataType(plc.types.TypeId.INT32), | ||
| ) | ||
| total_nanos = plc.binaryop.binary_operation( | ||
| nanos, | ||
| millis_as_nanos, | ||
| plc.binaryop.BinaryOperator.ADD, | ||
| plc.types.DataType(plc.types.TypeId.INT32), | ||
| ) | ||
| total_nanos = plc.binaryop.binary_operation( | ||
| total_nanos, | ||
| micros_as_nanos, | ||
| plc.binaryop.BinaryOperator.ADD, | ||
| plc.types.DataType(plc.types.TypeId.INT32), | ||
| ) | ||
| return Column(total_nanos) |
There was a problem hiding this comment.
Yeah, I definitely think we need kernels for this and similar things.
Suppose the datetime column is length N. This reads the input column three times, for 3 * N * 8 bytes, writes millis/micros/nanos for 3 * N * 4 bytes, then converts millis to nanos (N * 4 bytes read, N * 4 bytes written), and similarly micros to nanos. Then we add nanos and millis (2 * N * 4 bytes read, N * 4 bytes written) and then add that to micros (another 2 * N * 4 bytes read, N * 4 bytes written).
So we read (6 + 1 + 1 + 2 + 2) * N * 4 = 48 * N bytes and write (3 + 1 + 1 + 1 + 1) * N * 4 = 28 * N bytes.
In contrast, an optimal algorithm reads 8 N bytes and writes 4 N bytes. So we move approximately six times more data than necessary. Given we have perhaps 5-10x memory bandwidth difference compared to a good CPU implementation we kind of can't afford this cost.
I've also ignored the overhead due to chained dependent kernel launch latency.
There was a problem hiding this comment.
This code isn't technically part of this PR but I'm wondering if perhaps some kernels might be fusable here using the libcudf ast machinery, looking into this.
|
What is the plan on this PR? I think that we decided to go ahead and get this merged and then put up an issue requesting the libcudf kernel right? |
This one is WIP still, pending some efficiency improvements |
|
@vyasr the major issue with the libcudf kernels in this case is that they would be single consumer - polars. Both polars and pandas support the singular |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthrough
Changestotal_* Duration Component Extraction
Integral-to-Duration Type Casting
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 (1)
python/cudf_polars/tests/expressions/test_datetime_basic.py (1)
184-213: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winConsider adding null value coverage.
The test has good numeric coverage including edge cases, but doesn't test null handling. Duration columns can contain nulls, and verifying correct null propagation through
total_*operations would strengthen coverage.pl.Series( [ 0, None, # add null values 1500, None, -134234534, ], dtype=dtype, ),🤖 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 184 - 213, The test_duration_total_component_extract function currently lacks null value coverage in the durations Series test data. To improve test coverage and verify correct null propagation through the duration total component extraction operations, add null values (None) interspersed throughout the list of values in the pl.Series construction for the "durations" field. This will ensure the test validates both numeric edge cases and null handling behavior.
🤖 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/tests/expressions/test_datetime_basic.py`:
- Around line 184-213: The test_duration_total_component_extract function
currently lacks null value coverage in the durations Series test data. To
improve test coverage and verify correct null propagation through the duration
total component extraction operations, add null values (None) interspersed
throughout the list of values in the pl.Series construction for the "durations"
field. This will ensure the test validates both numeric edge cases and null
handling behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 449c1992-f3f5-4ce7-b9ac-9efc950c60da
📒 Files selected for processing (2)
python/cudf_polars/cudf_polars/dsl/expressions/datetime.pypython/cudf_polars/tests/expressions/test_datetime_basic.py
| result = plc.binaryop.binary_operation( | ||
| casted, | ||
| plc.Scalar.from_py( | ||
| factor, plc.DataType(plc.TypeId.INT64), stream=df.stream |
There was a problem hiding this comment.
suggestion: if factor is ever 1, we could avoid this plc.binaryop.binary_operation call all together and return Column(casted, dtype=self.dtype)
Matt711
left a comment
Matt711
left a comment
There was a problem hiding this comment.
Thanks @brandon-b-miller nothing else blocking from me. If you don't mind can you do a refresh of the table in #16481?
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@python/cudf_polars/tests/containers/test_column.py`:
- Around line 318-333: The test_astype_duration_to_narrower_integer function
only validates the dtype and type-id of the result but does not verify the
actual converted values, which means data corruption could go undetected. Add
assertions that check the actual values in result.obj match the expected
converted output from the original input [1, 2, -3]. Additionally, enhance the
test with edge case coverage by adding separate test cases or parameterization
for empty columns, null/missing values, and single-element columns to ensure
robustness per the coding guidelines. Consider also adding a complementary test
that covers the integral-to-duration conversion direction to provide complete
bidirectional coverage for the astype functionality.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 7e27609d-7ee7-4776-ae4a-4cd577a3f764
📒 Files selected for processing (1)
python/cudf_polars/tests/containers/test_column.py
| def test_astype_duration_to_narrower_integer(): | ||
| stream = get_cuda_stream() | ||
| col = Column( | ||
| plc.unary.cast( | ||
| plc.Column.from_iterable_of_py( | ||
| [1, 2, -3], plc.DataType(plc.TypeId.INT64), stream=stream | ||
| ), | ||
| plc.DataType(plc.TypeId.DURATION_MICROSECONDS), | ||
| stream=stream, | ||
| ), | ||
| dtype=DataType(pl.Duration(time_unit="us")), | ||
| ) | ||
| target_dtype = DataType(pl.Int32()) | ||
| result = col.astype(target_dtype, stream=stream) | ||
| assert result.dtype == target_dtype | ||
| assert result.obj.type().id() == plc.TypeId.INT32 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Test currently verifies metadata but not cast correctness of values
At Line 332–333, the test only checks dtype/type-id. Please also assert the converted values (and add at least one edge case like null/empty/single-element), otherwise value corruption/regressions in astype can pass unnoticed. Also, this test direction (duration→int) does not directly cover the new integral→duration path called out in this PR cohort.
As per coding guidelines: python/**/test_*.py: “Ensure test files provide comprehensive edge case coverage (empty, all-null, single-element, mixed types)”.
🤖 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/containers/test_column.py` around lines 318 - 333,
The test_astype_duration_to_narrower_integer function only validates the dtype
and type-id of the result but does not verify the actual converted values, which
means data corruption could go undetected. Add assertions that check the actual
values in result.obj match the expected converted output from the original input
[1, 2, -3]. Additionally, enhance the test with edge case coverage by adding
separate test cases or parameterization for empty columns, null/missing values,
and single-element columns to ensure robustness per the coding guidelines.
Consider also adding a complementary test that covers the integral-to-duration
conversion direction to provide complete bidirectional coverage for the astype
functionality.
Source: Coding guidelines
|
@Matt711 I'm seeing a coverage miss that I'm not sure is related to this change. It's a line that's involved in |
We only check code coverage on the latest polars version being tested. Looking at the CI logs... But on line 305 we explicitly skip code coverage. if left is None:
return right # pragma: no cover <-- 305But your branch ( Edit: Interesting, so that skip was not added by me in #22820. I remeber now adding a test specifically for that test. It was added in #22909 which is a commit your branch does not have. So
|
|
Ah this might just be being behind trunk, lets see if that fixes it 👍 |
|
/merge |
|
This PR got in?! 😂🎉 |
Part of #16481 Since theres two consumers of this API now (pandas and polars), I am wondering if we might take another look at adding libcudf APIs here. xref #16802 and cc @bdice @mroeschke @galipremsagar . WIP Authors: - https://github.com/brandon-b-miller - Vyas Ramasubramani (https://github.com/vyasr) Approvers: - Matthew Murray (https://github.com/Matt711) - Matthew Roeschke (https://github.com/mroeschke) URL: #18171
6 participants
Part of #16481
Since theres two consumers of this API now (pandas and polars), I am wondering if we might take another look at adding libcudf APIs here. xref #16802 and cc @bdice @mroeschke @galipremsagar .
WIP