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

feat: JS Clients can read constituent data in unaggregated columns #6663

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

niloc132
Copy link
Member

@niloc132 niloc132 commented Feb 25, 2025

Constituent columns are available for snapshot by the server, but the JS API until now has not made them available to users. This patch exposes those columns as part of the dh.TreeTable object, and allows them to be subscribed to.

Fixes DH-18431
Fixes #3303

@niloc132 niloc132 added this to the 0.38.0 milestone Feb 25, 2025
@niloc132 niloc132 requested a review from mofojed February 25, 2025 15:38
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.

No test plan or example for how to test this with JS?

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.

A minor nitpick for cleanup.

Tested with a few different tables:

from deephaven import agg, empty_table, ui
from deephaven.plot import express as dx

stocks = dx.data.stocks()
r = stocks.rollup(aggs=agg.sum_(["Price"]), by=["Sym", "Exchange"], include_constituents=True)
r2 = stocks.rollup(aggs=agg.sum_(["Price"]), by=["Sym", "Exchange"], include_constituents=False)
r3 = stocks.view(["Sym", "Exchange", "Size"]).rollup(aggs=agg.sum_(["Price"]), by=["Sym", "Exchange"], include_constituents=True)

constituentColumns.put(column.getName(), column);
} else {
columns[columns.length] = column;
if (definition.isRollupGroupByColumn() && !definition.isRollupConstituentNodeColumn()) {
Copy link
Member

Choose a reason for hiding this comment

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

No need for the && !definition.isRollupConstituentNodeColumn(), as that's always true in this else block

Suggested change
if (definition.isRollupGroupByColumn() && !definition.isRollupConstituentNodeColumn()) {
if (definition.isRollupGroupByColumn()) {

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

Successfully merging this pull request may close these issues.

JS API Rollup can offer unused column data
2 participants