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

fix: convert primitives to Objects inside of WebChunkReaderFactory #6669

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

Conversation

nbauernfeind
Copy link
Member

Fixes #6201.

@nbauernfeind nbauernfeind added bug Something isn't working jsapi barrage NoDocumentationNeeded ReleaseNotesNeeded Release notes are needed labels Feb 28, 2025
@nbauernfeind nbauernfeind added this to the 0.38.0 milestone Feb 28, 2025
@nbauernfeind nbauernfeind self-assigned this Feb 28, 2025
@nbauernfeind nbauernfeind changed the title Fix #6201 by converting primitives to Objects inside of WebChunkReaderFactory fix: convert primitives to Objects inside of WebChunkReaderFactory Feb 28, 2025
@nbauernfeind nbauernfeind requested a review from Copilot March 3, 2025 17:34

Choose a reason for hiding this comment

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

PR Overview

This pull request fixes issue #6201 by converting primitives to objects in the web client’s Barrage API. Key changes include:

  • Consolidation of column data handling into a single concrete WebColumnData class with new implementations for fillFromChunk and applyUpdate.
  • Refactoring of WebChunkReaderFactory to use new helper methods (e.g. newIntReader, newFloatReader) that convert primitive chunks into WritableObjectChunk instances.
  • Removal of auto‐generated, primitive-specific column data classes in favor of a unified object‐based approach.

Reviewed Changes

File Description
web/client-api/src/main/java/io/deephaven/web/client/api/barrage/data/WebColumnData.java Converted from abstract to concrete class and added object–based update logic.
web/client-api/src/main/java/io/deephaven/web/client/api/barrage/WebChunkReaderFactory.java Introduced new helper methods to simplify primitive to object conversion.
web/client-api/src/test/java/io/deephaven/web/client/api/HierarchicalTableTestGwt.java Re-enabled the DISTINCT aggregation to reflect the updated conversion logic.
Other files Adjustments throughout the subscription and state layers to accommodate the changes above.

Copilot reviewed 17 out of 17 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (3)

web/client-api/src/main/java/io/deephaven/web/client/api/barrage/WebChunkReaderFactory.java:389

  • [nitpick] Ensure that the new newIntReader implementation is adequately covered by tests, particularly for edge cases such as unsigned 64-bit integers and varying input sizes.
throw new IllegalArgumentException("Unsigned 64bit integers not supported");

web/client-api/src/main/java/io/deephaven/web/client/api/barrage/WebBarrageMessageReader.java:125

  • [nitpick] Review the decision to instantiate all chunks using WritableObjectChunk to ensure that the conversion from primitives does not lead to unintended type conversion issues for expected primitive values.
final WritableChunk<Values> chunk = WritableObjectChunk.makeWritableChunk(chunkSize);

web/client-api/src/test/java/io/deephaven/web/client/api/HierarchicalTableTestGwt.java:372

  • [nitpick] After re-enabling the DISTINCT aggregation, confirm that tests adequately cover scenarios involving DISTINCT operations to prevent regression issues.
JsAggregationOperation.DISTINCT,
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
barrage bug Something isn't working jsapi NoDocumentationNeeded ReleaseNotesNeeded Release notes are needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

JS API cannot read numeric array columns, such as from DISTINCT in a UI-created rollup
1 participant