fix: Evaluate simple rules before joining group rule columns - #374
Conversation
`with_evaluation_rules` joined the group rule evaluation columns onto the frame first and only then evaluated the simple rule expressions. Group rule columns are boolean, so a simple rule using dynamic column selection (e.g. `~pl.any_horizontal(pl.col(pl.Boolean))`) also picked up those internal columns and could fail validation for rows that are actually valid. Simple expressions are now added before the group rules are joined, so they only ever see the user's own columns. Group rule evaluation is unaffected because those rules are computed from the original frame either way. Adds a regression test covering a dynamic-selection rule alongside a group rule.
There was a problem hiding this comment.
Pull request overview
This PR fixes an interaction bug in rule evaluation where simple rules using dynamic column selectors (e.g. pl.col(pl.Boolean)) could inadvertently “see” internal boolean columns created for group rule evaluation, causing unexpected validation failures (as reported in #332).
Changes:
- Reorders evaluation so simple rule expressions are evaluated before group-rule columns are joined.
- Adds a regression test covering dynamic boolean-column selection together with a group rule.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
dataframely/_rule.py |
Changes evaluation order in with_evaluation_rules to prevent dynamic selectors in simple rules from including group-rule evaluation columns. |
tests/core_validation/test_rule_evaluation.py |
Adds a regression test ensuring dynamic boolean selection remains stable when group rules are present. |
| # NOTE: A value of `null` always validates successfully as nullability should | ||
| # already be checked via dedicated rules. | ||
| lf.pipe(_with_group_rules, group_rules).with_columns( | ||
| lf.with_columns( | ||
| **{name: expr.fill_null(True) for name, expr in simple_exprs.items()}, | ||
| ) | ||
| ).pipe(_with_group_rules, group_rules) |
Oliver Borchert (borchero)
left a comment
Oliver Borchert (borchero)
left a comment
There was a problem hiding this comment.
Thank you for tackling this Sanjay Santhanam (@Sanjays2402)! I'm wondering whether this is the most ergonomic way to solve it: an alternative option would be to evaluate group rules via over() instead of group_by+join which would also potentially result in speedups. Could you try this out and benchmark before and after using the existing benchmark tests (by running pixi run test-bench)?
Oliver Borchert (borchero)
changed the title
Closes #332
Motivation
A rule using dynamic column selection can fail unexpectedly as soon as the schema also has a group rule. In the reported MWE,
~pl.any_horizontal(pl.col(pl.Boolean))validates fine on its own but fails once a@dy.rule(group_by="x")is added.with_evaluation_rulesjoined the group rule evaluation columns onto the frame first and only then evaluated the simple rule expressions. Group rule columns are boolean, so the dynamic selector also picked up those internal columns.Changes
Simple expressions are now added before the group rules are joined, so they only ever see the user's own columns. Group rule evaluation is unaffected because those rules are computed from the original frame either way.
Adds
test_dynamic_column_selection_with_group_rule, which fails onmainand passes with this change.