Repository navigation
fix(dataframes): generated filter code should match the widget for the literal contains operator - #10617
Conversation
…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.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
I have read the CLA Document and I hereby sign the CLA |
|
All contributors have signed the CLA ✍️ ✅ |
There was a problem hiding this comment.
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=Truefixes regex interpretation, but the generated expression still omits the handler'sfill_null("")fromhandlers.py:268-270. Forcontainswith an empty value, a null cell is converted to an empty string and kept by the widget, while Polars filtering the rawstr.containsresult 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=Falsefixes metacharacter handling, but this still applies the operation to a raw nullable Series. The handler first fills nulls with""(seehandlers.py:268-270), so an emptycontainsvalue matches null cells in the widget, whereasSeries.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.negateand string columns containing nulls. The existing handler tests cover null and negatedcontains(tests/_plugins/ui/_impl/dataframes/test_handlers.py:339and3308), but these new cases only compare a non-nullkeep_rowsfilter, 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
left a comment
There was a problem hiding this comment.
@winklemad looks good to me; just a minor comment below; I'm good to approve when ready for review.
| # The property tests above draw filter values from an alphanumeric alphabet, | ||
| # so they never exercise a search string that is also a regex. |
There was a problem hiding this comment.
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.
|
Thanks @kirangadhave — good catch. Rewrote both the pandas and polars comments to describe what their own parametrized values ( |
|
test failures are unrelated |
|
Thanks for the review and merge, @kirangadhave! |
…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 pull request was authored by a coding agent.
📝 Summary
The dataframe transform code printer emits an unflagged
.str.contains(value)for the literalcontainsfilter 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 passesliteral=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:
print_code.py:52df[col].str.contains(value)Series.str.containsdefaultsregex=Trueregex=Falseprint_code.py:320pl.col(c).str.contains(value)Expr.str.containsdefaultsliteral=Falseliteral=TrueReproduction (rows
["abc", "a.c", "f(x)", "zzz"]):a.c— handler keeps["a.c"]; the printed pandas/polars code keeps["abc", "a.c"].(— handler keeps["f(x)"]; the printed pandas code raisespyarrow.lib.ArrowInvalid: Invalid regular expression: missing ), and the printed polars code raisesComputeError: regex parse error ... unclosed group.Why literal is the intended behavior here (not a judgement call):
regexoperator immediately below each of these two branches already passes the flag explicitly —regex=Truefor pandas (line 54),literal=Falsefor polars (line 322).containsandregexare separate operators precisely so that one is literal and one is not.print_code.py:557emits.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_pandasand..._polarsalready assert printed-code result == handler result, but they sharecreate_transform_strategy, whose string filter value strategy isst.text(alphabet=string.ascii_letters + string.digits)— an alphabet that can never emit a regex metacharacter, socontainsandregexare 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 intables/narwhals_table.py:359,373andtables/polars_table.py:262,274. Those are intended regex search and are locked bytests/_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.pygoes 13 → 17 passed; the new cases fail onmainfor the reasons above.tests/_plugins/ui/_impl/tables/+tests/_plugins/ui/_impl/dataframes/together: 862 passed, 0 failed.ruff check,ruff format --check, andmypyare clean.📋 Pre-Review Checklist
✅ Merge Checklist