Skip to content

Skip code coverage where dynamic predicates are dropped - #22973

Merged
rapids-bot[bot] merged 5 commits into
NVIDIA:mainfrom
Matt711:imp/polars/det-dyn-pred-test
Jun 25, 2026
Merged

Skip code coverage where dynamic predicates are dropped#22973
rapids-bot[bot] merged 5 commits into
NVIDIA:mainfrom
Matt711:imp/polars/det-dyn-pred-test

Conversation

@Matt711

@Matt711 Matt711 commented Jun 24, 2026

Copy link
Copy Markdown
Member

Description

The original test I added in #22820 does not produce the dynamic predicate on the same side of the filter determinitically.

import polars as pl

df = pl.LazyFrame({"a": [1, 2, 3, 4, 5], "b": [5, 4, 3, 2, 1], "c": [1, 1, 3, 3, 5]})
q = df.filter((pl.col("a") > 1) & (pl.col("c") == 3)).sort("b").head(3)

Inspect the logical plan on successive calls to explain()

SORT BY [slice: (0, 3, dynamic_pred: c6412d2b-fd62-4ecb-b5fc-c575f7fe4411)] [col("b")]
  FILTER [([(col("b").dynamic_predicate()) & ([(col("a")) > (1)])]) & ([(col("c")) == (3)])]
  FROM
    DF ["a", "b", "c"]; PROJECT */3 COLUMNS
SORT BY [slice: (0, 3, dynamic_pred: 347e337d-20db-4311-900f-53d387531102)] [col("b")]
  FILTER [([([(col("a")) > (1)]) & ([(col("c")) == (3)])]) & (col("b").dynamic_predicate())]
  FROM
    DF ["a", "b", "c"]; PROJECT */3 COLUMNS

So this PR skips code coverage to avoid this nondeterminism. We could write a hacky test to without sacrificing code coverage, but it's not worth it IMO.

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

@Matt711
Matt711 requested a review from a team as a code owner June 24, 2026 22:34
@Matt711
Matt711 requested a review from madsbk June 24, 2026 22:34
@Matt711 Matt711 added bug Something isn't working non-breaking Non-breaking change labels Jun 24, 2026
@github-actions github-actions Bot added Python Affects Python cuDF API. cudf-polars Issues specific to cudf-polars labels Jun 24, 2026
@GPUtester GPUtester moved this to In Progress in cuDF Python Jun 24, 2026
@coderabbitai

coderabbitai Bot commented Jun 24, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: e8a5f51f-5606-4b36-a034-cf152c1abbee

📥 Commits

Reviewing files that changed from the base of the PR and between 89e4ecc and 3ceb789.

📒 Files selected for processing (2)
  • python/cudf_polars/cudf_polars/dsl/translate.py
  • python/cudf_polars/tests/test_filter.py

📝 Walkthrough

Summary by CodeRabbit

  • Documentation
    • Added clarifying comments about how dynamic predicate handling behaves in filter logic.
  • Chores
    • Updated the copyright notice in a test file.

Walkthrough

Adds explanatory comments in _drop_dyn_pred_hints about nondeterministic AND-tree branch pruning, and updates the SPDX copyright header in python/cudf_polars/tests/test_filter.py.

Changes

Dynamic predicate hint comments

Layer / File(s) Summary
Branch selection comments
python/cudf_polars/cudf_polars/dsl/translate.py
Adds comments explaining which AND branch survives when _drop_dyn_pred_hints returns the non-None side.

SPDX header update

Layer / File(s) Summary
Header text update
python/cudf_polars/tests/test_filter.py
Appends “All rights reserved.” to the SPDX-FileCopyrightText comment.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~2 minutes

Possibly related PRs

  • rapidsai/cudf#22820: Shares the same dynamic-predicate hint dropping logic referenced by the translate.py comment change.

Suggested labels

Python, cudf-polars

Suggested reviewers

  • nirandaperera
  • TomAugspurger
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly matches the main change: skipping coverage for nondeterministic dynamic-predicate drops.
Description check ✅ Passed The description directly explains the nondeterministic dynamic-predicate issue and the resulting test coverage skip.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@TomAugspurger

Copy link
Copy Markdown
Contributor

Thanks! I got very about how this could fail while looking into this yesterday.

I merged main now that #22979 is in.

@Matt711

Matt711 commented Jun 25, 2026

Copy link
Copy Markdown
Member Author

/merge

@rapids-bot
rapids-bot Bot merged commit 14e9e9c into NVIDIA:main Jun 25, 2026
108 checks passed
@github-project-automation github-project-automation Bot moved this from In Progress to Done in cuDF Python Jun 25, 2026
@Matt711
Matt711 deleted the imp/polars/det-dyn-pred-test branch June 25, 2026 14:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working cudf-polars Issues specific to cudf-polars non-breaking Non-breaking change Python Affects Python cuDF API.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants