Fix: expand dict transformation#9561
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
1 issue found across 3 files
Architecture diagram
sequenceDiagram
participant User as User (Marimo UI)
participant DFPlugin as Dataframe Plugin
participant Handler as ExpandDict Handler (handlers.py)
participant Narwhals as Narwhals Layer
participant Polars as Polars Engine
participant Backend as Original Backend (pandas/Ibis/other)
Note over User,Backend: Expand Dict Transformation Flow
User->>DFPlugin: Trigger expand dict on column
DFPlugin->>Handler: handle_expand_dict(df, transform)
Handler->>Narwhals: collect_and_preserve_type(df)
Narwhals->>Backend: Collect actual data from original backend
Backend-->>Narwhals: Data as native type
Narwhals-->>Handler: Collected DataFrame + undo function
Handler->>Polars: collected_df.to_polars()
Note over Handler,Polars: Convert to Polars for unnest support
Polars->>Polars: polars_df.unnest(column_id)
Note over Polars: Handles null dict values correctly
Polars-->>Handler: Unnested Polars DataFrame
Handler->>Narwhals: nw.from_native(unnested)
Narwhals->>Handler: Narwhals wrapper
Handler->>Handler: undo(narwhals_df)
Note over Handler: Convert back to original backend type
Handler-->>DFPlugin: Transformed DataFrame
DFPlugin-->>User: Updated table with expanded columns
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Re-trigger cubic
mscolnick
left a comment
There was a problem hiding this comment.
need to take an optional dep on polars
… rows with the create test dataframes instead.
refactor: adding None == NaN in assert frame equal with nans method to use it in expand_dict test.
Done. |
|
There are some pandas CI errors that i am looking into. |
… errors for mixed object columns.
This is happening because of data conversion mismatch between pandas and narwahls with mixed data types for So my only option is to fallback to pandas backend processing separately for the |
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
… json normalise following the handlers code.
Bundle ReportBundle size has no change ✅ Affected Assets, Files, and Routes:view changes for bundle: marimo-esmAssets Changed:
|
Thanks, fixed. |
kirangadhave
left a comment
There was a problem hiding this comment.
Requesting a major change which should remove hard dependency on polars for other backends which support structs.
Also please address other nits.
| expanded.index = result_df.index | ||
| return undo(nw.from_native(result_df.join(expanded))) | ||
|
|
||
| DependencyManager.polars.require( |
There was a problem hiding this comment.
non pandas dataframes are forced to go through polars conversion here. For duckdb or ibis, that means forcing polars installation. We should not do that.
Narwhals has struct.field, so we can do:
schema = df.collect_schema()
fields = [f.name for f in schema[col].fields]
df.with_columns(
[nw.col(col).struct.field(f).alias(f) for f in fields]
).drop(col)This approach also stays lazy. Pandas approach with json_serialize is correct.
There was a problem hiding this comment.
Are you sure that all backends, which support struct schema would necessarily have struct.field?
There was a problem hiding this comment.
struct.field is part of the narwhals' public api, so it should translate okay. We support pandas, polars, ibis, pyarrow and duckdb backends. Except pandas all others have struct.field support.
It might be worth adding tests though. Thoughts?
There was a problem hiding this comment.
Yes, you are correct about narwahl's api.
create_test_dataframes is used for generating the dataframes for the expand dict tests.
To support duckdb, should i create a new test for expand dict or it doesn't matter as the tests are only supported for pandas, polars, ibis, pyarrow?
| # older versions of pandas running on py310 otherwise CI will fail | ||
| expanded = pd.json_normalize( | ||
| result_df.pop(transform.column_id).map( | ||
| lambda value: {} if value is None else value |
| import pandas as pd | ||
|
|
||
| result_df = native_df.copy() | ||
| # max_level=0 was used so that pandas doesn't recursively unnest dicts |
There was a problem hiding this comment.
the comment is narrating the code, simplify to explain the why instead.
| why="to expand dict/struct columns for non-pandas backends" | ||
| ) | ||
| polars_df = collected_df.to_polars() | ||
| unnested = polars_df.unnest(transform.column_id) |
There was a problem hiding this comment.
Duplicate column names after unnest will throw error here. Handle it gracefully.
feat: raising duplicate columns error test: adding necessary tests for duplicate columns
|
Thanks for the review comments. |
kirangadhave
left a comment
There was a problem hiding this comment.
Thanks again for the changes. We will be good to merge after this set of changes. Especially the test related ones are must do.
| allow_none_equals_nan=True, | ||
| ) | ||
|
|
||
| @pytest.mark.xfail( |
There was a problem hiding this comment.
xfail is used to mark tests where failure is expected. Raising error is the expected pass case here, so don't mark as xfail.
| # pandas and other backends agree on expand-dict behaviour. | ||
| expanded = pd.json_normalize( | ||
| result_df.pop(transform.column_id).map(normalise_empty_dict), | ||
| # type: ignore[arg-type] |
There was a problem hiding this comment.
type ignore comment on new line doesn't do anything
|
|
||
| schema = collected_df.collect_schema() | ||
| dtype = schema.get(transform.column_id) | ||
| fields = getattr(dtype, "fields", None) |
There was a problem hiding this comment.
use isinstance(dtype, nw.Struct) here?
| .select(expanded_columns) | ||
| ) | ||
|
|
||
| raise nw.exceptions.InvalidOperationError( |
fix: checking if schema is of dtype struct for handle expand dict. tests: add separate tests for expand dict error. fix: remove xfail from expand dict test.
Thank you again for the detailed review. |
|
Thanks @Shamik-07 for your contribution and addressing multiple review rounds |
📝 Summary
Using narwahls to convert all backend to polars and then using the
unnestfunction of polars for expanding the dict and then convert it back to the original backend.Closes #4583
Screen.Recording.2026-05-15.at.18.07.461.mov
📋 Pre-Review Checklist
✅ Merge Checklist