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

Restore totals table support in the JS API #3662

Merged
merged 11 commits into from
Jun 2, 2023

Conversation

niloc132
Copy link
Member

@niloc132 niloc132 commented Apr 5, 2023

Fixes #3314

Copy link
Member

@mofojed mofojed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@niloc132 At a quick glance, how would @emilyhuxng use this for the databar stuff? We can create a totals table, but we can't join it back to the original table. JsTable.join expects another JsTable, whereas this is a JsTotalsTable. Or am I misthinking this?

@niloc132
Copy link
Member Author

niloc132 commented Apr 5, 2023

Good call, I'll work on relaxing that a bit further. Under the hood, a TotalsTable is just a Table, except we don't let you perform certain operations on it that don't make sense.

@niloc132
Copy link
Member Author

niloc132 commented Apr 5, 2023

@emilyhuxng I've added a new interface which will will permit tables and totals tables to be joined to tables and totals tables.

@niloc132 niloc132 marked this pull request as ready for review May 24, 2023 00:04
@niloc132
Copy link
Member Author

@mofojed this isn't quite ready, but I wanted to discuss two things, see if we can find a way to finalize and ship it:

  • Count doesn't work the same way it did in DHE, which was to append the column name to __Count. From playing a bit with DHE, it will then render the count under each selected column (by default, this is "all of them"), but just as "Count". DHC doesn't let the client create a count aggregation on each column, which makes a bit more sense anyway, but the web UI doesn't presently know how to handle that. Do you want to have synthetic columns created to match DHE behavior, or can the UI handle this?
  • Distinct needs to be able to fetch the array of data that ... is distinct for that column - likely to be a big output, and while in DHE we could download it as a large string, in DHC the server will send an array of whatever type it is, which the JS API can't yet handle (but will be fixed in Improve JS API's usage of subscriptions #188, in the near-ish future). Would it be reasonable to disable this in the short term?

@niloc132
Copy link
Member Author

Discussed with Ryan, and we could paper over both of these with custom columns (updateview to normalize arrays to string, and rename Count columns to ColA__Count, ColB__Count...), but ideally we wouldnt do too much of this if we can help it.

@mofojed
Copy link
Member

mofojed commented May 25, 2023

For the second, I'd prefer custom columns to match DHE behaviour, until we've improved the JS API usage of subscriptions.
For the first, I thought COUNT only counted non-null rows or something, but that doesn't seem to be the case. Certain that it's always the same on every column (basically just row count)? I think I'd still like it to behave like DHE, UI wise we'll need to change up a few things if it essentially just applies to the whole table instead.

@niloc132
Copy link
Member Author

In column statistics, the Count operation is only non-nulls (as distinct from Size), but not in aggregations (in either dhe or dhc).

@niloc132
Copy link
Member Author

niloc132 commented Jun 2, 2023

Column renaming and toString-ing is complete, plus also removing requested aggregations that don't make sense and the server will reject.

@niloc132 niloc132 requested a review from mofojed June 2, 2023 17:11
@niloc132 niloc132 merged commit 4e419b9 into deephaven:main Jun 2, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Jun 2, 2023
@deephaven-internal
Copy link
Contributor

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

JS API support for totals tables
3 participants