Drop dynamic predicates from logical plan - #22820
Conversation
github-actions
Bot
added
Python
|
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:
📝 WalkthroughWalkthroughAdds predicate translation helpers that remove Polars ChangesDynamic Predicate Hint Translation
Engine Options Configuration
🎯 3 (Moderate) | ⏱️ ~22 minutes 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 |
| ) | ||
|
|
||
|
|
||
| def test_filter_drops_dynamic_predicate_hint(): |
There was a problem hiding this comment.
I'm testing it this way mainly because I don't to deal with getting the optimizer to deterministically insert dynamic hints.
|
/merge |
|
/merge |
|
Update: Tests are failing with 1.41, but I'm not sure which tests yet. |
| right = _drop_dyn_pred_hints(translator, node.right, schema) | ||
| if left is None: | ||
| return right | ||
| if right is None: |
There was a problem hiding this comment.
TODO code coverage is missing here
|
/merge |
| engine_options.setdefault("raise_on_fail", True) | ||
| engine = pl.GPUEngine( | ||
| executor="in-memory", | ||
| raise_on_fail=True, |
There was a problem hiding this comment.
Because currently our nightly benchmarks are not raising an error when we fallback to CPU. This was missed in all the refactoring we did to tests / benchmarks
| expr.BinOp._MAPPING[node.op], | ||
| left, | ||
| right, | ||
| ) |
There was a problem hiding this comment.
What are these hints and what is going on here?
There was a problem hiding this comment.
They are same dynamic predicate hints we discussed in pola-rs/polars#27616 (comment). I think your real question is why are they showing up in the inputs of binop expressions? I found an example:
In [2]: import polars as pl
...:
...: ldf = pl.LazyFrame(
...: {"a": [1, 2, 3, 4, 5], "b": [5, 4, 3, 2, 1], "c": [1, 1, 3, 3, 5]}
...: )
...: q = ldf.filter((pl.col("a") > 1) & (pl.col("c") == 3)).sort("b").head(3)
...: print(q.explain())
SORT BY [slice: (0, 3, dynamic_pred: 2a68ecf7-b479-48bd-9737-f7bebb1e5ecf)] [col("b")]
FILTER [([([(col("a")) > (1)]) & ([(col("c")) == (3)])]) & (col("b").dynamic_predicate())]
FROM
DF ["a", "b", "c"]; PROJECT */3 COLUMNSPolars will insert a dynamic predicate in the binary expression; essentially saying "you're doing a sort + slice and you're filtering by a > 1 and c == 3, you can also skip rows where b is above the top-3 once we (cudf-polars) figure what the threshold is"
17427c6 to
ff1e9b3
Compare
The original test I added in #22820 does not produce the dynamic predicate on the same side of the filter determinitically. ```python 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. Authors: - Matthew Murray (https://github.com/Matt711) - Tom Augspurger (https://github.com/TomAugspurger) Approvers: - Mads R. B. Kristensen (https://github.com/madsbk) URL: #22973
4 participants
Description
Fixes performance regression in nightly benchmark (due to falling back to polars CPU). In polars 1.42, we should be able to properly handle dynamic predicates since they'll be available in the node visitor.
Also ensures that we fail if we fallback to polars CPU.
Checklist