Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CSV download optimizations for larger files (formerly: "*doesn't work in release candidate") #1678

Closed
kosak opened this issue Dec 9, 2021 · 6 comments · Fixed by #1973
Closed
Assignees
Labels
bug Something isn't working release blocker A bug/behavior that puts is below the "good enough" threshold to release.
Milestone

Comments

@kosak
Copy link
Contributor

kosak commented Dec 9, 2021

I install Deephaven per the instructions (except with VERSION = edge)

In the Python window, I type

from deephaven.TableTools import *
t = emptyTable(10)

In the TableOptions menu, I select "Download CSV", keep the defaults and select "Download".
It pauses for a moment and then says "Download Canceled" (see screenshot).

image

@kosak kosak added bug Something isn't working triage labels Dec 9, 2021
@rcaudy rcaudy added release blocker A bug/behavior that puts is below the "good enough" threshold to release. and removed triage labels Dec 9, 2021
@rcaudy rcaudy added this to the Dec 2021 milestone Dec 9, 2021
@niloc132
Copy link
Member

niloc132 commented Dec 9, 2021

@mofojed can you look at this? The example in the description has no columns, so it is the most boring csv file ever, but even adding a column I am seeing a 404 at /ide/download/table-2021-12-09-142136.csv instead of a real download.

@mofojed mofojed self-assigned this Dec 10, 2021
@mofojed
Copy link
Member

mofojed commented Dec 10, 2021

@kosak did you test this with a table with any data? E.g:

from deephaven.TableTools import *
t = emptyTable(10).updateView("I=i")

Also what browser are you using?
There seems to be a problem exporting an empty table for sure, but that doesn't seem like a release blocker.
I can't reproduce in Firefox or Chromium, but Colin has an issue in Firefox, would like to know what you have so we can decide what to investigate. If you can test in another browser as well that would be great.

@niloc132
Copy link
Member

FF94 failed for this, but ff95 (latest, released 3 days ago) does not for me.

It is definitely a bug to not handle a table with no columns correctly, but not a critical one.

@niloc132
Copy link
Member

Okay, I reproduced this with Corey on a call, interestingly it does manifest in the same "Download Canceled" way as the no-column case. It can be seen with any large table (note that if it is too large you get a different error from the server), for example:

t = emptyTable(1000000).updateView("I=i")

The download prompt will appear, but the download will fail.

The issue is that JS's TableViewportSubscription.snapshot(rows, columns) doesnt actually use the rows and columns it is given, but returns the entire table. This seems to break the client due to a timeout of some kind.

With a much larger table, you get a different error, the server refuses to send payloads that big (could be in the grpc server, could be in envoy, I didn't investigate). This will also be fixed by fixing TableViewportSubscription.snapshot.

Options to fix:

@niloc132
Copy link
Member

Another consideration is that the UI should indicate up front the total row x col set that will be interesting, and snapshot that rather than the entire table.

@pete-petey
Copy link
Member

Relates to CSV download being limited in size in web. A "gradual" approach with optimization is appropriate -- API and JS work.

@pete-petey pete-petey modified the milestones: Dec 2021, Jan 2022 Dec 29, 2021
@pete-petey pete-petey changed the title CSV download doesn't work in release candidate CSV download optimizations for larger files (formerly: "*doesn't work in release candidate") Dec 29, 2021
@niloc132 niloc132 modified the milestones: Jan 2022, Feb 2022 Feb 15, 2022
niloc132 added a commit that referenced this issue Mar 1, 2022
…#1973)

This uses the flight DoExchange call with the BarrageSnapshotRequest
payload.

Also fixes leak of rowsets in BarrageStreamGenerator, and correctly
reports the RecordBatch length for viewport snapshots.

Fixes #1678
Fixes #1039
mofojed pushed a commit that referenced this issue Dec 11, 2023
Release notes https://github.com/deephaven/web-client-ui/releases/tag/v0.56.0

# [0.56.0](deephaven/web-client-ui@v0.55.0...v0.56.0) (2023-12-11)


### Bug Fixes

* add right margin to <Button kind='inline'/> using icons ([#1664](deephaven/web-client-ui#1664)) ([fd8a6c6](deephaven/web-client-ui@fd8a6c6))
* adjust filter bar colour ([#1666](deephaven/web-client-ui#1666)) ([4c0200e](deephaven/web-client-ui@4c0200e))
* convert organize columns component to purecomponent ([#1653](deephaven/web-client-ui#1653)) ([8ddc114](deephaven/web-client-ui@8ddc114)), closes [#1650](deephaven/web-client-ui#1650)
* Default to `Skip` operation instead of `Sum` operation ([#1648](deephaven/web-client-ui#1648)) ([6083173](deephaven/web-client-ui@6083173)), closes [#1355](deephaven/web-client-ui#1355) [#1355](deephaven/web-client-ui#1355)
* Fix button snapshots ([#1655](deephaven/web-client-ui#1655)) ([c0cc966](deephaven/web-client-ui@c0cc966))
* popper blur in styleguide ([#1672](deephaven/web-client-ui#1672)) ([6fa2204](deephaven/web-client-ui@6fa2204))
* Unable to delete selected rows in some input tables ([#1678](deephaven/web-client-ui#1678)) ([1e71550](deephaven/web-client-ui@1e71550)), closes [#1677](deephaven/web-client-ui#1677)


### Features

* Add embed-widget ([#1668](deephaven/web-client-ui#1668)) ([1b06675](deephaven/web-client-ui@1b06675)), closes [#1629](deephaven/web-client-ui#1629)
* forward and back button for organize column search ([#1641](deephaven/web-client-ui#1641)) ([89f2be5](deephaven/web-client-ui@89f2be5)), closes [#1529](deephaven/web-client-ui#1529)
* Tables that have names starting with underscore do not auto-launch from console ([#1656](deephaven/web-client-ui#1656)) ([21131fe](deephaven/web-client-ui@21131fe)), closes [#1549](deephaven/web-client-ui#1549) [#1410](deephaven/web-client-ui#1410)
* theme fontawesome icon size wrapped in spectrum icons ([#1658](deephaven/web-client-ui#1658)) ([2aa8cef](deephaven/web-client-ui@2aa8cef))
* Theme Selector ([#1661](deephaven/web-client-ui#1661)) ([5e2be64](deephaven/web-client-ui@5e2be64)), closes [#1660](deephaven/web-client-ui#1660)
* Theming - Bootstrap ([#1603](deephaven/web-client-ui#1603)) ([88bcae0](deephaven/web-client-ui@88bcae0))
* Theming - Inline svgs ([#1651](deephaven/web-client-ui#1651)) ([1e40d3e](deephaven/web-client-ui@1e40d3e))
* View cell contents in context menu ([#1657](deephaven/web-client-ui#1657)) ([90b7517](deephaven/web-client-ui@90b7517)), closes [#1605](deephaven/web-client-ui#1605)


### BREAKING CHANGES

* Bootstrap color variables are now predominantly hsl based. SCSS will need to be updated accordingly. Theme providers are needed to load themes.
* Tables assigned to variable beginning with "_" will not open automatically even if "Auto Launch Panels" is checked.

Co-authored-by: deephaven-internal <deephaven-internal@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working release blocker A bug/behavior that puts is below the "good enough" threshold to release.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants