Skip to content

feat: DH-18856: Add Median to jsapi Aggregation Options#6700

Merged
dgodinez-dh merged 8 commits intodeephaven:mainfrom
dgodinez-dh:dag_MedianAggJsApi
Mar 20, 2025
Merged

feat: DH-18856: Add Median to jsapi Aggregation Options#6700
dgodinez-dh merged 8 commits intodeephaven:mainfrom
dgodinez-dh:dag_MedianAggJsApi

Conversation

@dgodinez-dh
Copy link
Copy Markdown
Contributor

@dgodinez-dh dgodinez-dh commented Mar 13, 2025

https://deephaven.atlassian.net/browse/DH-18856

Tested using the following branch from web-client-ui:
https://github.com/dgodinez-dh/web-client-ui/tree/dag_MedianAgg
Tested median on numeric, string, and date columns.

Copy link
Copy Markdown
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.

Can you include in the PR description how this was tested?
I assume you have tested via JS API (e.g. from the console on one of the jsapi example pages).
Also there may be additional unit tests to add (@niloc132 ?), and the jsapi/rollup.html page should be updated with the Median aggregation.

@dgodinez-dh dgodinez-dh requested a review from mofojed March 14, 2025 17:48
@dgodinez-dh dgodinez-dh marked this pull request as draft March 17, 2025 14:46
@dgodinez-dh dgodinez-dh marked this pull request as ready for review March 17, 2025 16:01
@dgodinez-dh
Copy link
Copy Markdown
Contributor Author

Can you include in the PR description how this was tested? I assume you have tested via JS API (e.g. from the console on one of the jsapi example pages). Also there may be additional unit tests to add (@niloc132 ?), and the jsapi/rollup.html page should be updated with the Median aggregation.

Updated the description. But I ended removing Median from the jsapi rollup code. It is not supported in the engine. I am talking to Don about this because I think the UI will need to avoid applying Median to rollup.
The jsapi handles this gracefully if to do: rollup -> Median. It logs a warning and ignores it.
If you do: Median -> rollup, we hang forever on viewport and log an exception.

Copy link
Copy Markdown
Member

@niloc132 niloc132 left a comment

Choose a reason for hiding this comment

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

Please add a test for this and other supported operations to TotalsTableTestGwt, like HIerarchicalTableTestGwt.testCreateRollupAggTypes(), just to validate that we get results at all.

@dgodinez-dh dgodinez-dh requested a review from niloc132 March 18, 2025 19:00

return waitForEvent(totals, JsTable.EVENT_UPDATED, update -> {
ViewportData viewportData = (ViewportData) update.getDetail();
assertEquals(2, viewportData.getRows().length);
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.

Should probably include checks for the content of the cells in these rows, similar to the above test cases.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Colin asked for a test similar to HierarchicalTableTestGwt.testCreateRollupAggTypes, which looks like it iterates over all the agg types and checks that they work and send an update. Checking the data might be complicated because we are doing every agg, so the data will be different per agg.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added some value checking.

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.

Data checking in the rollup test doesnt exist either - the intention was just to make sure we could at least read every type, and that no supported agg causes a failure when passing it to the API.

@dgodinez-dh dgodinez-dh requested a review from mofojed March 19, 2025 16:27
@dgodinez-dh dgodinez-dh merged commit 66afa5b into deephaven:main Mar 20, 2025
@github-actions github-actions Bot locked and limited conversation to collaborators Mar 20, 2025
@dgodinez-dh dgodinez-dh deleted the dag_MedianAggJsApi branch March 24, 2025 14:37
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants