Pin Polars<1.43 - #22993
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
/ok to test c55949f |
|
/ok to test f95b02b |
|
/ok to test 5f9aaf7 |
|
/ok to test 93434d6 |
|
/ok to test 1ad3e5f |
|
/ok to test 585c4b0 |
|
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:
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
WalkthroughThis PR widens Polars version bounds, adds a Polars 1.42 gate constant, updates translation and BooleanFunction handling for newer Polars behavior, and adjusts tests plus expected-failure mappings. ChangesPolars version and compatibility updates
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
python/cudf_polars/tests/expressions/test_booleanfunction.py (1)
298-326: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd edge-case coverage for
is_sorted.Test only covers a 5-element column with at most one null. Consider adding empty, single-element, and all-null cases to exercise
plc.sorting.is_sortedboundary behavior.Based on path instructions, "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/expressions/test_booleanfunction.py` around lines 298 - 326, Extend test_boolean_is_sorted to cover the missing boundary cases for pl.col(...).is_sorted, not just the current 5-element inputs. Add parametrized assertions for empty, single-element, and all-null Series in the same test module so the GPUEngine path exercises plc.sorting.is_sorted across these edge cases, alongside the existing asc/desc/unsorted coverage.Source: Path instructions
🤖 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/testing/inject_gpu_engine.py`:
- Around line 249-251: Scope the xfail entries in inject_gpu_engine.py to the
Polars version that actually triggers them by guarding the affected test names
in the mapping with a version check or version-specific condition. Update the
entries for test_filter_contradiction_fallible_error_handling,
test_hconcat_predicate, and test_union_lazyframe_horizontal so they only apply
when running against Polars 1.42.1, rather than unconditionally disabling those
tests for all versions.
---
Nitpick comments:
In `@python/cudf_polars/tests/expressions/test_booleanfunction.py`:
- Around line 298-326: Extend test_boolean_is_sorted to cover the missing
boundary cases for pl.col(...).is_sorted, not just the current 5-element inputs.
Add parametrized assertions for empty, single-element, and all-null Series in
the same test module so the GPUEngine path exercises plc.sorting.is_sorted
across these edge cases, alongside the existing asc/desc/unsorted coverage.
🪄 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: 765bd2d6-bd98-4070-a616-81d578d85c38
📒 Files selected for processing (16)
.pre-commit-config.yamlconda/environments/all_cuda-129_arch-aarch64.yamlconda/environments/all_cuda-129_arch-x86_64.yamlconda/environments/all_cuda-133_arch-aarch64.yamlconda/environments/all_cuda-133_arch-x86_64.yamlconda/recipes/cudf-polars/recipe.yamldependencies.yamlpython/cudf_polars/cudf_polars/containers/dataframe.pypython/cudf_polars/cudf_polars/dsl/expressions/boolean.pypython/cudf_polars/cudf_polars/dsl/translate.pypython/cudf_polars/cudf_polars/testing/inject_gpu_engine.pypython/cudf_polars/cudf_polars/utils/versions.pypython/cudf_polars/pyproject.tomlpython/cudf_polars/tests/dsl/test_serialization.pypython/cudf_polars/tests/expressions/test_booleanfunction.pypython/cudf_polars/tests/test_scan.py
Yes it allows us to support it. I have 3-4 follow-up PRs in mind
|
|
/merge |
| # test_dtypes: narwhals' dtype mapping changed with polars 1.40 (reports Object where the test expects Int8). | ||
| # test_namespace_len[polars[lazy]]: len() row count lost in zero-column streaming chunks | ||
| # (https://github.com/rapidsai/cudf/issues/21428). | ||
| # test_explode_*[polars[lazy]-*]: polars 1.42 emits a DeprecationWarning when explode() is called |
There was a problem hiding this comment.
I haven't found time TODO #19594 yet. Maybe I'll put an agent on it
|
/merge |
Description
Updated cudf-polars to support Polars 1.42
I have 3-4 follow-up PRs in mind
NodeTraverserdoes not supportIRFunctionExpr::Fused#22797Checklist