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

Improve JS API's usage of subscriptions #188

Closed
niloc132 opened this issue Mar 23, 2021 · 5 comments · Fixed by #5890
Closed

Improve JS API's usage of subscriptions #188

niloc132 opened this issue Mar 23, 2021 · 5 comments · Fixed by #5890
Assignees
Milestone

Comments

@niloc132
Copy link
Member

niloc132 commented Mar 23, 2021

To expedite development, JS is still using flatten, and re-creating subscriptions rather than updating them out-of-band. Fortunately, by flattening we reduce the cost of re-creating subscriptions, at least in terms of sending large sparse indexes.

The first fix should be to change both types of subscriptions to share the same code to allow for non-flat indexes - to achieve this, viewports will need to handle receiving a snapshot that only covers part of the new viewport. Ideally, out-of-band subscription updates should be implemented before this work is done.

Additionally, subscriptions are using ColumnData rather than just creating the real data that is used by the API - some of this indirection can be cleaned up. Doing this work will also let us make the kinds of data we can read much more flexible, and possibly consider delegating to arrow.

Breaking this down as bulletpoints rather than text above:

  • Stop re-creating subscriptions when the viewport or columns change, but still subscribed to the same table
  • Support the "reverse viewport" feature
  • Stop requiring "flat" tables - some code should be sharable with the non-viewport subscription to achieve this, but it will need to be generalized to support viewports.
  • Drop the ColumnData, TableSnapshot, DeltaUpdates intermediate types, as long as so much of the above is being rewritten, to make it easier to support new types.
@niloc132 niloc132 added the jsapi label Mar 23, 2021
@niloc132 niloc132 self-assigned this Mar 23, 2021
@niloc132
Copy link
Member Author

niloc132 commented Mar 30, 2021

Also, the refresh interval isn't being passed to the server, should be updated with these other changes.

See #186

@niloc132
Copy link
Member Author

See #2435

@niloc132
Copy link
Member Author

niloc132 commented Sep 5, 2023

Example query that is slow due to "redirection index"

from deephaven import empty_table
from deephaven.plot.figure import Figure

t = empty_table(100000).update(["X=ii", "Y=ii*2"])
figure = Figure()
before = figure\
    .plot_xy(series_name="Y", t=t, x="X", y="Y")\
    .show()

@niloc132
Copy link
Member Author

niloc132 commented Sep 5, 2023

#1917

niloc132 added a commit that referenced this issue Nov 2, 2023
Reintroduces the column statistics feature from DHE. This code is
copied, with a few changes to how it behaves and how it functions:

 * Results are sent to the client in a static table rather than a
 simple object payload.
 * Client can control the max number of unique values to display,
 up to a point (previously the max was technically 19, new default
 is 20).
 * Non-Comparable objects now have their "popularity" counts as well
 as Comparable, until a max limit is reached. Only the most common 20
 entries will be sent to the client.
 * Deephaven "null" values are used to represent that a standard
 deviation or average cannot be computed.

Note that until #188 is solved, the JS API and web UI will not display
the unique value list.

Fixes #697
niloc132 added a commit to niloc132/deephaven-core that referenced this issue Nov 14, 2023
niloc132 added a commit that referenced this issue Nov 14, 2023
stanbrub pushed a commit that referenced this issue Nov 15, 2023
niloc132 added a commit to niloc132/deephaven-core that referenced this issue Jul 10, 2024
Math.nextUp is not presently usable in GWT, so this patch lets the JS
API share QueryConstants and ensures that the MIN_FINITE values are
correct, without actually initializing them via a call to Math.nextUp.

Partial deephaven#188
niloc132 added a commit to niloc132/deephaven-core that referenced this issue Jul 10, 2024
niloc132 added a commit to niloc132/deephaven-core that referenced this issue Jul 10, 2024
niloc132 added a commit that referenced this issue Jul 11, 2024
Math.nextUp is not presently usable in GWT, so this patch lets the JS
API share QueryConstants and ensures that the MIN_FINITE values are
correct, without actually initializing them via a call to Math.nextUp.

Partial #188
Co-authored-by: Ryan Caudy <rcaudy@gmail.com>
niloc132 added a commit that referenced this issue Jul 11, 2024
Reusing this code for the JS implementation of flight/barrage, and
noticed some dead/unused code.

Partial #188
niloc132 added a commit that referenced this issue Jul 11, 2024
The string `culprit` as a marker for pointing to an issue in a stack
trace has never appeared in a DHC issue, nor in DHC slack, and the last
DHE slack message to use it in a mildly helpful way was more than two
years ago.

This patch removes the functionality from `RequirementFailure`, and also
removes any overload of a method in `Require` that would allow callers
to customize the depth within the stack that should be marked.

Partial #188
Co-authored-by: Ryan Caudy <rcaudy@gmail.com>
niloc132 added a commit that referenced this issue Jul 12, 2024
Removes usages of gnu.trove from the API of rowset related classes.
These usages were previously unused, though this is technically an API
breaking change.

Partial #188
Co-authored-by: Ryan Caudy <rcaudy@gmail.com>
niloc132 added a commit that referenced this issue Jul 15, 2024
…5748)

Remaining arrays are deprecated to allow gradual removal. All other
methods have been removed, or relocated/rewritten to where they are
used.

Partial #188
niloc132 added a commit to niloc132/deephaven-core that referenced this issue Jul 17, 2024
niloc132 added a commit that referenced this issue Jul 18, 2024
Like #5552, applies some after-the-fact review of the design for reading
Barrage/Flight stream, in anticipation of sharing this code with
JavaScript clients.

 * Removes unused BarrageChunkAppendingMarshaller
 * Adds more error detail when stream contents don't match metadata
 * Removes an unused parameter when parsing Flight messages
 * Inlines width of various types into their readers
 * Introduces an interface for reading data into chunks, and a factory
interface to allow JS clients to supply their own implementations.

Partial #188
niloc132 added a commit that referenced this issue Jul 19, 2024
…ed with JS API (#5780)

Removed some unused methods, rewrote some implementations to avoid
reflection where unnecessary, removed
ObjectInputStream/ObjectOutputStream usage.

BREAKING CHANGES: Moved isBoxedNumeric, isNumeric, isBigNumeric to
NumericTypeUtils, removed @IsDateTime, removed other reflective methods
from TypeUtils and ArrayTypeUtils.
Partial #188
niloc132 added a commit that referenced this issue Jul 19, 2024
… be shared with JS API (#5782)

Where necessary, inlined these methods as one-liners. In some cases, the
instance checks are reduced to not-null checks, since Java already would
guarantee that the variables were the correct type.

BREAKING CHANGES: Removed holdsLock, notHoldsLock, instanceOf,
notInstanceOf from Assert and Require.
Partial #188
niloc132 added a commit that referenced this issue Jul 31, 2024
* Extracted ExposedByteArrayOutputStream to its own file
* Avoid String.format in a few places
* Extract Array.newInstance so the JS API can replace with a different call

Partial #188
Co-authored-by: Ryan Caudy <rcaudy@gmail.com>
niloc132 added a commit that referenced this issue Aug 2, 2024
…5811)

Rather than relying on a static method to create CISG instances,
extracts a factory so that the JS client can provide its own
implementation of how CISG instances should be built.

Also cleans up ChunkReader/ChunkInputStreamGenerator split a bit more,
since for now JS can only use the former.

BREAKING CHANGES: Removes
ChunkInputStreamGenerator.makeInputStreamGenerator, use the default
factory instance instead.
Partial #188
niloc132 added a commit that referenced this issue Aug 7, 2024
Reduces some duplication of code, by sharing the ReferenceCounted logic
for tracking when a CISG type should close its chunks or other managed
resources.

Partial #188
Co-authored-by: Ryan Caudy <rcaudy@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants