Skip to content

fix(table): depth-limit nested sample values (salvage of #9383, credit @weiguangli-io)#10229

Closed
Bartok9 wants to merge 4 commits into
marimo-team:mainfrom
Bartok9:top10/salvage-9383-sample-depth
Closed

fix(table): depth-limit nested sample values (salvage of #9383, credit @weiguangli-io)#10229
Bartok9 wants to merge 4 commits into
marimo-team:mainfrom
Bartok9:top10/salvage-9383-sample-depth

Conversation

@Bartok9

@Bartok9 Bartok9 commented Jul 18, 2026

Copy link
Copy Markdown
Contributor

This pull request was authored by a coding agent.

salvage: depth-limit nested get_sample_values (credit @weiguangli-io #9383)

Original PR #9383 (credit @weiguangli-io) added a nesting depth cap so deeply nested Polars Struct columns could not stall dataset registration (#9378). That PR is CONFLICTING against current main: to_primitive was rewritten to use json.dumps for list/dict values.

This salvage

  • Reapplies the MAX_NESTING_DEPTH = 5 idea on top of current main
  • Depth-limits the structure before json.dumps, preserving Enum/int/float behavior
  • Adds test_get_sample_values_nested_depth_limit (pandas nested dict depth 20 must finish <1s)

Closes #9378
Supersedes / salvages #9383 (full credit to original author)

AI-assisted; human-reviewed. Human Co-authored-by retained for salvage credit.

  • Any AI generated code has been reviewed line-by-line by the human PR author, who stands by it.
  • Tests added
  • Contributor guidelines read

@Bartok9 @weiguangli-io
fix(table): depth-limit nested sample values (salvage marimo-team#9383)
7ff789f 
Cap list/dict walking in get_sample_values before json.dumps so
pathologically nested Polars/pandas struct samples cannot stall dataset
registration.

Salvage of marimo-team#9383 (credit @weiguangli-io). Closes marimo-team#9378.

Co-authored-by: Weiguang Li <weiguangli-io@users.noreply.github.com>
@vercel

vercel Bot commented Jul 18, 2026 •
edited
Loading

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, Comment Jul 20, 2026 1:16am

Request Review

@Bartok9

Bartok9 commented Jul 18, 2026

Copy link
Copy Markdown
Contributor Author

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

@Bartok9
Bartok9 marked this pull request as ready for review July 19, 2026 22:50
Sanjays2402
Sanjays2402 reviewed Jul 19, 2026
View reviewed changes
Comment thread marimo/_plugins/ui/_impl/tables/narwhals_table.py Outdated
@Bartok9
fix(table): use fixed sentinel at sample nesting depth cap
e5bf854 
str(value) at MAX_NESTING_DEPTH still walks large list/dict trees
from branching payloads (marimo-team#9378). Emit "..." for containers instead,
and add a branching fixture that asserts the sentinel + time bound.

Addresses review on marimo-team#10229.
@Bartok9

Bartok9 commented Jul 20, 2026

Copy link
Copy Markdown
Contributor Author

Thanks @Sanjays2402 — good catch.

Pushed a follow-up: at MAX_NESTING_DEPTH we now emit a fixed "..." sentinel for list/dict containers instead of str(value), so branching payloads no longer force a full remaining-tree walk. Added test_get_sample_values_branching_payload_is_bounded for that case (time bound + sentinel present).

Latest: e5bf854

@Bartok9

Bartok9 commented Jul 20, 2026

Copy link
Copy Markdown
Contributor Author

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

@Light2Dark

Light2Dark commented Jul 20, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

This bug has already been fixed iirc, are there issues with that fix?

@Bartok9

Bartok9 commented Jul 20, 2026

Copy link
Copy Markdown
Contributor Author

You're right @Light2Dark — thanks for catching this. This overlaps with #9506, which already landed a fix for the nested-struct sample-value blow-up (via json.dumps(..., default=...)). My depth-limit approach here is now redundant against main, so I'll close this. Appreciate the review time. 🙏

@Bartok9 Bartok9 closed this Jul 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
@Sanjays2402 Sanjays2402 Sanjays2402 left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Named Polars DataFrames with deeply nested struct columns become extremely slow when registered as datasets

3 participants

@Bartok9 @Light2Dark @Sanjays2402