Skip to content

table: Add ranged dumps for batched export - #2755

Merged
madcodelife merged 4 commits into
longbridge:mainfrom
lurenjia534:fix/unbounded-table-csv-export
Aug 21, 2026
Merged

table: Add ranged dumps for batched export#2755
madcodelife merged 4 commits into
longbridge:mainfrom
lurenjia534:fix/unbounded-table-csv-export

Conversation

@lurenjia534

@lurenjia534 lurenjia534 commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

Closes #2754

Description

TableState::dump_range lets callers materialize a selected row range while
preserving the existing full-table dump API. The requested range is clamped
to the delegate's current row count so applications can iterate over large
tables in fixed-size batches.

The DataTable story now prompts for the destination before reading table data,
then materializes 2,000 rows at a time on the foreground GPUI context. A
capacity-one channel sends those batches to one background csv::Writer<File>,
which streams directly to disk without accumulating the complete table, CSV
buffer, or String in memory.

TableState::dump remains an eager whole-table API for compatibility. Large
data exporters should use dump_range with an application-level batch size.
Progress UI, cancellation, remote pagination, and delegate changes remain
outside this PR.

Screenshot

Not applicable. This changes export scheduling and file I/O rather than the
rendered table UI.

How to Test

cargo fmt --all -- --check
cargo test -p gpui-component --test table_dump_range
cargo test -p gpui-component
cargo check -p gpui-component-story
RUSTC_BOOTSTRAP=1 cargo check \
  -p gpui-component-story-web \
  --target wasm32-unknown-unknown

Observed results:

  • formatting passed;
  • both table_dump_range tests passed;
  • the complete gpui-component test suite passed;
  • the native story crate compiled successfully;
  • the WASM story check passed after enabling the unstable feature required by
    the current wasm_thread dependency (RUSTC_BOOTSTRAP=1); a plain stable
    check stops in that dependency with E0554 before compiling the story;
  • the DataTable story opened its save dialog before export work; cancelling
    produced no output, while exporting the default table produced one header
    and 5,000 records with 45 columns each;
  • the first exported record had ID 0, the last had ID 4999, and the CSV
    parsed successfully with consistent column counts.

The ranged dump regression tests verify that dump_range(98..103) is clamped
to a 100-row table and only calls cell_text for rows 98 and 99, while the
existing dump(cx) still returns the complete table.

AI Assistance

AI assisted with root-cause analysis, implementation, test design, validation
planning, and PR wording. I reviewed the final diff and ran the checks listed
above.

Checklist

  • I have read the CONTRIBUTING document and followed the guidelines.
  • Reviewed the changes in this PR and confirmed AI-generated code is accurate.
  • Passed cargo run for story tests related to the changes.
  • Tested macOS, Windows, and Linux platform performance (not platform-specific).

@lurenjia534
lurenjia534 force-pushed the fix/unbounded-table-csv-export branch from 8154244 to 04a80ac Compare August 18, 2026 05:51
@lurenjia534
lurenjia534 marked this pull request as ready for review August 18, 2026 05:51

@huacnlee huacnlee 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.

I'd think add a limit is not a good solution.

In this case, I think we can have a async, batch export to support a large data table. @madcodelife

@lurenjia534

Copy link
Copy Markdown
Contributor Author

I'd think add a limit is not a good solution.

In this case, I think we can have a async, batch export to support a large data table. @madcodelife

Thanks review, that makes sense. A total export limit would reject legitimate large tables rather than support them.

A possible direction would be an asynchronous batched export: prompt for the destination first, materialize only one fixed-size row batch at a time through the foreground GPUI context, and pass each batch through a bounded channel to a background CSV writer. This would avoid building the complete table or CSV in memory, while still exporting every row. The batch size would control peak materialization and scheduling rather than impose a total data limit.

The existing eager, unbounded dump path would need to be removed or replaced. Progress UI, cancellation, remote pagination, and delegate redesign could remain outside the initial patch unless they are considered necessary.

I’ll hold off on further changes to the current limit-based approach and wait for your discussion with @madcodelife on the preferred design and ownership.

@madcodelife, if you would like me to continue with this change, would you prefer a public row-range batching primitive on TableState for downstream exporters, or should the batching remain internal to the DataTable CSV export path?

@madcodelife

Copy link
Copy Markdown
Member

Agreed, in fact in our application dump is already called from an async context, so this direction matches real usage.

For your question: prefer a public row-range API on TableState, e.g. dump_range(range, cx), and keep dump(cx) as is. The CSV writing and async loop should stay in the app layer, the library only provides bounded materialization, so downstream apps can build their own exporters on it.

The story's write_csv can be the reference implementation: fetch a few thousand rows per batch on the foreground, stream each batch to a background writer, don't accumulate them in memory.

@lurenjia534

Copy link
Copy Markdown
Contributor Author

Thanks, that clarifies the intended boundary. I’ll rework this PR to add a
public TableState::dump_range, keep dump unchanged, and move the batched
async CSV loop into the DataTable story.

I’ll keep progress UI, cancellation, remote pagination, and delegate changes
outside this PR.

@lurenjia534
lurenjia534 marked this pull request as draft August 19, 2026 08:29
@lurenjia534
lurenjia534 force-pushed the fix/unbounded-table-csv-export branch from 04a80ac to 8169b02 Compare August 20, 2026 03:27
@lurenjia534 lurenjia534 changed the title table: Limit dump resource usage table: Add ranged dumps for batched export Aug 20, 2026
@lurenjia534
lurenjia534 force-pushed the fix/unbounded-table-csv-export branch from 8169b02 to 3964f6d Compare August 20, 2026 03:35
@lurenjia534
lurenjia534 marked this pull request as ready for review August 20, 2026 03:42
@lurenjia534

Copy link
Copy Markdown
Contributor Author

Reworked this PR based on the review discussion:

  • restored the existing TableState::dump(cx) API unchanged;
  • added public TableState::dump_range(range, cx);
  • changed the DataTable story to prompt for the path first;
  • streamed 2,000-row batches through a capacity-one channel to a background CSV writer;
  • replaced the limit tests with focused ranged-dump and compatibility tests.

Progress UI, cancellation, remote pagination, delegate redesign, and snapshot semantics remain outside this PR.

@huacnlee @madcodelife, could you please take another look?

@madcodelife

Copy link
Copy Markdown
Member

Thank you.

@madcodelife
madcodelife enabled auto-merge (squash) August 21, 2026 03:09
@madcodelife
madcodelife merged commit 1a27414 into longbridge:main Aug 21, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

TableState::dump eagerly materializes unbounded table data and can exhaust memory

3 participants