Support pl.Expr.to_physical/hash#23313
Conversation
|
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds ChangesUnary expression support
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/cudf_polars/dsl/expressions/unary.py`:
- Around line 491-497: Update the unary hash handling in the name == "hash"
branch to honor all provided Polars seeds instead of silently using only the
first; since plc.hashing.xxhash_64 accepts one seed, implement the required
multi-seed behavior or explicitly raise NotImplementedError for multiple seeds,
while preserving single-seed hashing.
In `@python/cudf_polars/tests/expressions/test_numeric_unaryops.py`:
- Around line 219-232: Update test_hash to validate collected hash values and
null handling by replacing the schema-only assertion with
assert_gpu_result_equal(q, engine=engine). Preserve both parametrized seed
variants and the existing query inputs.
In `@python/pylibcudf/pylibcudf/unary.pyx`:
- Around line 175-191: Expand the bit_cast docstring to document its
bit-preserving conversion behavior, the input Column, target DataType, optional
stream and memory resource parameters, and the returned Column. Follow the
surrounding public wrapper docstring conventions and describe the result without
changing the implementation.
🪄 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: 9f971490-e674-441f-ab50-123b3ff57d23
📒 Files selected for processing (5)
python/cudf_polars/cudf_polars/dsl/expressions/unary.pypython/cudf_polars/tests/expressions/test_numeric_unaryops.pypython/pylibcudf/pylibcudf/libcudf/column/column_view.pxdpython/pylibcudf/pylibcudf/unary.pyipython/pylibcudf/pylibcudf/unary.pyx
| "dtype", | ||
| [ | ||
| pl.Date, | ||
| pl.Datetime("us"), |
There was a problem hiding this comment.
Do we support pl.Time too? https://docs.pola.rs/api/python/dev/reference/expressions/api/polars.Expr.to_physical.html mentions it.
There was a problem hiding this comment.
We don't support either types (we raise during translation)
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
python/cudf_polars/cudf_polars/dsl/expressions/unary.py (1)
491-497: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPass all
Expr.hashseeds through explicitly
hash(tuple(self.options))collapsesseed,seed_1,seed_2, andseed_3into one Python hash, so this cannot preserve Polars’ seed semantics. Forward the four seeds directly toxxhash_64with the documented defaults, or raiseNotImplementedErrorfor unsupported arities.🤖 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/unary.py` around lines 491 - 497, The Expr hash implementation in the name == "hash" branch incorrectly combines all options into one Python hash. Extract and forward seed, seed_1, seed_2, and seed_3 individually to plc.hashing.xxhash_64 using Polars’ documented defaults, and raise NotImplementedError for unsupported seed arities.
🤖 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.
Outside diff comments:
In `@python/cudf_polars/cudf_polars/dsl/expressions/unary.py`:
- Around line 491-497: The Expr hash implementation in the name == "hash" branch
incorrectly combines all options into one Python hash. Extract and forward seed,
seed_1, seed_2, and seed_3 individually to plc.hashing.xxhash_64 using Polars’
documented defaults, and raise NotImplementedError for unsupported seed arities.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 5e356c2e-1f5c-4a68-9a74-731919834e83
📒 Files selected for processing (2)
python/cudf_polars/cudf_polars/dsl/expressions/unary.pypython/cudf_polars/tests/expressions/test_numeric_unaryops.py
🚧 Files skipped from review as they are similar to previous changes (1)
- python/cudf_polars/tests/expressions/test_numeric_unaryops.py
Description
xref #23151
https://docs.pola.rs/api/python/dev/reference/expressions/api/polars.Expr.to_physical.html
https://docs.pola.rs/api/python/dev/reference/expressions/api/polars.Expr.hash.html
Checklist