Skip to content

fix(dataframes): generated filter code should match the widget for the literal contains operator - #10617

Merged
Light2Dark merged 3 commits into
marimo-team:mainfrom
winklemad:fix/dataframe-contains-literal-printed-code
Aug 25, 2026
Merged

Light2Dark merged 3 commits into
marimo-team:mainfrom
winklemad:fix/dataframe-contains-literal-printed-code

Conversation

@winklemad

Copy link
Copy Markdown
Contributor

This pull request was authored by a coding agent.

📝 Summary

The dataframe transform code printer emits an unflagged .str.contains(value) for the literal contains filter operator, so the code marimo shows (and that the user copies out of the notebook) evaluates the search string as a regex, while the handler that actually filters the displayed table passes literal=True.

The result is that the generated code silently returns different rows than the widget displays, and raises outright on an unbalanced metacharacter.

This does not affect the widget's own filtering — NarwhalsTransformHandler._evaluate_condition (handlers.py:268-270) is correct. The defect is purely printed-code vs. handler divergence.

Two sites, fixed together:

Backend Line Emitted before Default Now
pandas print_code.py:52 df[col].str.contains(value) Series.str.contains defaults regex=True regex=False
polars print_code.py:320 pl.col(c).str.contains(value) Expr.str.contains defaults literal=False literal=True

Reproduction (rows ["abc", "a.c", "f(x)", "zzz"]):

  • value a.c — handler keeps ["a.c"]; the printed pandas/polars code keeps ["abc", "a.c"].
  • value ( — handler keeps ["f(x)"]; the printed pandas code raises pyarrow.lib.ArrowInvalid: Invalid regular expression: missing ), and the printed polars code raises ComputeError: regex parse error ... unclosed group.

Why literal is the intended behavior here (not a judgement call):

  1. The regex operator immediately below each of these two branches already passes the flag explicitly — regex=True for pandas (line 54), literal=False for polars (line 322). contains and regex are separate operators precisely so that one is literal and one is not.
  2. The ibis printer at print_code.py:557 emits .contains(value), which is literal in ibis and therefore already agrees with the handler.

Why the existing tests missed it: test_print_code_result_matches_actual_transform_pandas and ..._polars already assert printed-code result == handler result, but they share create_transform_strategy, whose string filter value strategy is st.text(alphabet=string.ascii_letters + string.digits) — an alphabet that can never emit a regex metacharacter, so contains and regex are indistinguishable under it. The two new tests are therefore explicit rather than generated, and use values that are regexes (a.c, (); an alphanumeric value would pass both before and after this change.

Deliberately left alone: the superficially similar str.contains(f"(?i){query}") calls in tables/narwhals_table.py:359,373 and tables/polars_table.py:262,274. Those are intended regex search and are locked by tests/_plugins/ui/_impl/tables/test_narwhals.py::test_search_with_regex. print_code.py:557 (ibis) is already correct and is untouched.

Testing: tests/_plugins/ui/_impl/dataframes/test_print_code.py goes 13 → 17 passed; the new cases fail on main for the reasons above. tests/_plugins/ui/_impl/tables/ + tests/_plugins/ui/_impl/dataframes/ together: 862 passed, 0 failed. ruff check, ruff format --check, and mypy are clean.

📋 Pre-Review Checklist

  • For large changes, or changes that affect the public API: this change was discussed or approved through an issue, on Discord, or the community discussions.
  • Any AI generated code has been reviewed line-by-line by the human PR author, who stands by it.
  • Video or media evidence is provided for any visual changes (optional).

✅ Merge Checklist

  • I have read the contributor guidelines.
  • Documentation has been updated where applicable, including docstrings for API changes.
  • Tests have been added for the changes made.

…polars

The contains operator filters literally in the transform handler, but the
generated code left the regex flag at its default, so copying the code out
of the dataframe UI gave different rows than the table showed, and raised
on an unbalanced metacharacter.

Add explicit regex=False (pandas) and literal=True (polars), matching the
regex operator right below each site, which already passes the flag.
@vercel

vercel Bot commented Aug 20, 2026 •

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
marimo-docs Ready Ready Preview Aug 24, 2026 10:21pm

Request Review

@winklemad

Copy link
Copy Markdown
Contributor Author

I have read the CLA Document and I hereby sign the CLA

@github-actions

Copy link
Copy Markdown
Contributor

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Updates generated dataframe filter code so literal contains behavior matches the widget for pandas and Polars.

Changes:

  • Adds explicit literal-matching flags.
  • Adds regression tests for regex-like values.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Summary
tests/_plugins/ui/_impl/dataframes/test_print_code.py Tests literal matching for pandas and Polars.
marimo/_plugins/ui/_impl/dataframes/transforms/print_code.py Emits literal contains filters.
Suppressed comments (3)

marimo/_plugins/ui/_impl/dataframes/transforms/print_code.py:320

  • literal=True fixes regex interpretation, but the generated expression still omits the handler's fill_null("") from handlers.py:268-270. For contains with an empty value, a null cell is converted to an empty string and kept by the widget, while Polars filtering the raw str.contains result sees a null predicate and drops it. Please mirror the handler's null handling (and its behavior for negated conditions) and add an empty-value/null regression case.
            result = f"{col}.str.contains({_as_literal(value)}, literal=True)"

marimo/_plugins/ui/_impl/dataframes/transforms/print_code.py:52

  • regex=False fixes metacharacter handling, but this still applies the operation to a raw nullable Series. The handler first fills nulls with "" (see handlers.py:268-270), so an empty contains value matches null cells in the widget, whereas Series.str.contains("", regex=False) returns NA/None and the generated dataframe filter drops or errors on that row; negated object masks can also fail. Please generate the same null-safe expression, including the handler's null exclusion for negated conditions, and cover the empty-value/null case.
            result = f"{col}.str.contains({_as_literal(value)}, regex=False)"

tests/_plugins/ui/_impl/dataframes/test_print_code.py:620

  • Because this PR is agent-authored, please also validate the updated printer on the edge paths rather than only the direct positive case: remove_rows/FilterCondition.negate and string columns containing nulls. The existing handler tests cover null and negated contains (tests/_plugins/ui/_impl/dataframes/test_handlers.py:339 and 3308), but these new cases only compare a non-null keep_rows filter, so they do not show that copied pandas/Polars code stays aligned for those paths. Screenshots are not applicable to this backend-only change, but the additional test results would be useful.
@pytest.mark.parametrize("value", ["a.c", "("])

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@kirangadhave kirangadhave left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@winklemad looks good to me; just a minor comment below; I'm good to approve when ready for review.

Comment on lines +618 to +619
# The property tests above draw filter values from an alphanumeric alphabet,
# so they never exercise a search string that is also a regex.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

comments should reflect what the tests they document are doing, they shouldn't refer to tests above as those could be independently deleted/changed.

Describe what each test's own parametrized values exercise rather than
referring to the property tests above, which could change independently.
@winklemad

Copy link
Copy Markdown
Contributor Author

Thanks @kirangadhave — good catch. Rewrote both the pandas and polars comments to describe what their own parametrized values (a.c, () exercise and why they'd fail without a literal flag, so they no longer lean on the tests above. Marking ready for review.

@winklemad
winklemad marked this pull request as ready for review August 24, 2026 21:47
@kirangadhave kirangadhave added the bug Something isn't working label Aug 24, 2026
@Light2Dark

Copy link
Copy Markdown
Member

test failures are unrelated

@Light2Dark
Light2Dark merged commit 9fc1796 into marimo-team:main Aug 25, 2026
38 of 40 checks passed
@winklemad

Copy link
Copy Markdown
Contributor Author

Thanks for the review and merge, @kirangadhave!

williambdean pushed a commit to williambdean/marimo that referenced this pull request Sep 6, 2026
…e literal `contains` operator (marimo-team#10617)

**This pull request was authored by a coding agent.**

## 📝 Summary

The dataframe transform code printer emits an unflagged
`.str.contains(value)` for the literal `contains` filter operator, so
the code marimo shows (and that the user copies out of the notebook)
evaluates the search string as a **regex**, while the handler that
actually filters the displayed table passes `literal=True`.

The result is that the generated code silently returns different rows
than the widget displays, and raises outright on an unbalanced
metacharacter.

**This does not affect the widget's own filtering** —
`NarwhalsTransformHandler._evaluate_condition` (`handlers.py:268-270`)
is correct. The defect is purely printed-code vs. handler divergence.

Two sites, fixed together:

| Backend | Line | Emitted before | Default | Now |
| --- | --- | --- | --- | --- |
| pandas | `print_code.py:52` | `df[col].str.contains(value)` |
`Series.str.contains` defaults `regex=True` | `regex=False` |
| polars | `print_code.py:320` | `pl.col(c).str.contains(value)` |
`Expr.str.contains` defaults `literal=False` | `literal=True` |

**Reproduction** (rows `["abc", "a.c", "f(x)", "zzz"]`):

- value `a.c` — handler keeps `["a.c"]`; the printed pandas/polars code
keeps `["abc", "a.c"]`.
- value `(` — handler keeps `["f(x)"]`; the printed pandas code raises
`pyarrow.lib.ArrowInvalid: Invalid regular expression: missing )`, and
the printed polars code raises `ComputeError: regex parse error ...
unclosed group`.

**Why literal is the intended behavior here** (not a judgement call):

1. The `regex` operator immediately below each of these two branches
already passes the flag explicitly — `regex=True` for pandas (line 54),
`literal=False` for polars (line 322). `contains` and `regex` are
separate operators precisely so that one is literal and one is not.
2. The ibis printer at `print_code.py:557` emits `.contains(value)`,
which is literal in ibis and therefore already agrees with the handler.

**Why the existing tests missed it:**
`test_print_code_result_matches_actual_transform_pandas` and
`..._polars` already assert printed-code result == handler result, but
they share `create_transform_strategy`, whose string filter value
strategy is `st.text(alphabet=string.ascii_letters + string.digits)` —
an alphabet that can never emit a regex metacharacter, so `contains` and
`regex` are indistinguishable under it. The two new tests are therefore
explicit rather than generated, and use values that *are* regexes
(`a.c`, `(`); an alphanumeric value would pass both before and after
this change.

**Deliberately left alone:** the superficially similar
`str.contains(f"(?i){query}")` calls in
`tables/narwhals_table.py:359,373` and `tables/polars_table.py:262,274`.
Those are intended regex search and are locked by
`tests/_plugins/ui/_impl/tables/test_narwhals.py::test_search_with_regex`.
`print_code.py:557` (ibis) is already correct and is untouched.

**Testing:** `tests/_plugins/ui/_impl/dataframes/test_print_code.py`
goes 13 → 17 passed; the new cases fail on `main` for the reasons above.
`tests/_plugins/ui/_impl/tables/` +
`tests/_plugins/ui/_impl/dataframes/` together: 862 passed, 0 failed.
`ruff check`, `ruff format --check`, and `mypy` are clean.

## 📋 Pre-Review Checklist

- [ ] For large changes, or changes that affect the public API: this
change was discussed or approved through an issue, on Discord, or the
community discussions. <!-- not applicable: two-line behavioral fix to
an internal code printer, no public API change -->
- [x] Any AI generated code has been reviewed line-by-line by the human
PR author, who stands by it.
- [ ] Video or media evidence is provided for any visual changes
(optional).

## ✅ Merge Checklist

- [x] I have read the [contributor
guidelines](https://github.com/marimo-team/marimo/blob/main/CONTRIBUTING.md).
- [x] Documentation has been updated where applicable, including
docstrings for API changes.
- [x] Tests have been added for the changes made.

---------

Co-authored-by: Kiran Gadhave <[email protected]>

This branch was successfully deployed

1 active deployment
Preview — 7e9f54be Deployed Aug 24, 2026 by vercel[bot]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants