feat: Barrage Refactor Read/Write Chunk Factories #6065

nbauernfeind · 2024-09-13T15:08:22Z

Fixes #58 (custom type serialization / deserialization).
Fixes #936 (ColumnConversionModes is being replaced with easy to integrate custom serialization).
Fixes #2984 (refactoring has good interface documentation).
Fixes #3403 (by supporting a variety of mappings, these now must match client wiring).
Fixes #5258 (snapshot/subscribe methods with default w2w options).
Fixes #5453 (support other Timestamp arrow wire encodings).
Fixes #5864 (support for uint64_t).
Fixes #6114 (supports ObjectVector properly).

This PR adds support to (some to be implemented as this is a WIP):

enables users to target wire types and configurations
enables users to target deephaven types, including custom codecs
improves snapshot/subscription APIs for default options

nightlies: https://github.com/nbauernfeind/deephaven-core/actions/runs/12172832822

rcaudy

A little nervous about the scope, and the willingness to truncate inputs or infer units.

engine/chunk/src/main/java/io/deephaven/chunk/Chunk.java

engine/table/src/main/java/io/deephaven/engine/table/impl/QueryTable.java

engine/table/src/main/java/io/deephaven/engine/table/impl/sources/ReinterpretUtils.java

rcaudy · 2024-09-16T16:12:25Z

extensions/barrage/src/main/java/io/deephaven/extensions/barrage/chunk/ChunkReader.java

@@ -18,7 +20,56 @@
 /**
 * Consumes Flight/Barrage streams and transforms them into WritableChunks.
 */
-public interface ChunkReader {
+public interface ChunkReader<ReadChunkType extends WritableChunk<Values>> {


We wondered if you could make the bound looser, e.g. PoolableChunk or SafeCloseable. Looks like maybe not? Should PoolableChunk be a Chunk (it's not right now)?

We need these to return the Chunks to support usages such as LongChunkReader#transformTo. Since ownership is also being passed, it is best to advertise that they're WritableChunks.

extensions/barrage/src/main/java/io/deephaven/extensions/barrage/chunk/ChunkReader.java

...s/barrage/src/main/java/io/deephaven/extensions/barrage/chunk/DefaultChunkWriterFactory.java

rcaudy · 2024-09-16T20:49:29Z

...s/barrage/src/main/java/io/deephaven/extensions/barrage/chunk/DefaultChunkWriterFactory.java

+        }
+    }
+
+    private static long factorForTimeUnit(final TimeUnit unit) {


Ryan's placeholder.

flakiness (#9948)"

response (#9314)"

…lification

…protobuf repeated fields (#6434) The fix is taken from code in #6065. That branch has been in WIP state for several months and we need this fix for DHE. fixes #6433

…protobuf repeated fields (deephaven#6434) The fix is taken from code in deephaven#6065. That branch has been in WIP state for several months and we need this fix for DHE. fixes deephaven#6433

…protobuf repeated fields (#6434) (#6461) The fix is taken from code in #6065. That branch has been in WIP state for several months and we need this fix for DHE. fixes #6433

… gRPC client (deephaven#6420) This reverts commit 6ada0cb. See deephaven#6401 See deephaven#6400 See deephaven#5996

cpwright

The biggest thing I would like to see is unit tests that demonstrate coverage of the various different modes and cases that can happen for the types. There is a lot of code, and I think the testing you've done has mostly been using our clients or flight sql to show that the data makes it back and forth; but there is a lot of room in the spec and a lot of cases that we should make sure get covered for each of these types. We really do have a matrix that is pretty darn big with all of the various input types and output types in the DefaultChunkReaderFactory and DefaultChunkWriterFactory methods.

engine/api/src/main/java/io/deephaven/engine/table/Table.java

engine/table/src/main/java/io/deephaven/engine/table/impl/BaseTable.java

engine/table/src/main/java/io/deephaven/engine/table/impl/remote/ConstructSnapshot.java

engine/table/src/main/java/io/deephaven/engine/updategraph/impl/PeriodicUpdateGraph.java

extensions/barrage/src/main/java/io/deephaven/extensions/barrage/ChunkListWriter.java

extensions/barrage/src/main/java/io/deephaven/extensions/barrage/chunk/UnionChunkWriter.java

...rrage/src/main/java/io/deephaven/extensions/barrage/chunk/SingleElementListHeaderReader.java

extensions/barrage/src/main/java/io/deephaven/extensions/barrage/chunk/MapChunkReader.java

...nsions/barrage/src/main/java/io/deephaven/extensions/barrage/chunk/VarBinaryChunkWriter.java

cpwright · 2024-12-16T18:11:47Z

R/rdeephaven/inst/tests/testthat/test_table_handle_wrapper.R

@@ -56,6 +56,8 @@ test_that("is_static returns the correct value", {
 })

 test_that("nrow returns the correct number of rows", {
+  skip()


Is this just temporary until you work with Corey to fix?

These few test skip()s are going to be committed with this PR to enable independent PRs between Corey and I.

The issue is that the cpp client cannot handle the data type explicitly uploaded by the r client in this test (as now DH assume you want to round-trip schema).

extensions/barrage/src/main/java/io/deephaven/extensions/barrage/chunk/BooleanChunkWriter.java

extensions/barrage/src/main/java/io/deephaven/extensions/barrage/chunk/ByteChunkWriter.java

extensions/barrage/src/main/java/io/deephaven/extensions/barrage/chunk/ListChunkWriter.java

...nsions/barrage/src/main/java/io/deephaven/extensions/barrage/chunk/VarBinaryChunkWriter.java

cpwright · 2024-12-16T18:34:18Z

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

@@ -275,7 +275,8 @@ public <T extends WritableChunk<Values>> ChunkReader<T> newReader(
                final ExpansionKernel<?> kernel =
                        ArrayExpansionKernel.makeExpansionKernel(chunkType, componentTypeInfo.type());
                final ChunkReader<?> componentReader = newReader(componentTypeInfo, options);
-                return (ChunkReader<T>) new ListChunkReader<>(ListChunkReader.Mode.DENSE, 0, kernel, componentReader);
+                return (ChunkReader<T>) new ListChunkReader<>(ListChunkReader.Mode.VARIABLE, 0, kernel,


Why hardcoded here?

Primary reason is/was that the UI client (at this time) can only speak with a DH server. So, we know that the Schema field is going to be a List. I've added ListView and FixedSizeList to this case and differentiated the modes.

We don't have a way to currently test the code paths for ListView nor FixedSizeList for the web reader .. but I see that it might be useful if someone finds a way to get their paws on our reader in js or typescript and want to read from a non-deephaven arrow host.

It's also easy for me to add what looks to be correct.

...s/barrage/src/main/java/io/deephaven/extensions/barrage/chunk/DefaultChunkWriterFactory.java

niloc132 · 2024-12-20T21:58:55Z

I think this should fix #6201 - at one point I believe you had nbauernfeind@b18d660 in this branch, can you bring it back in so we can verify the fix, remove the TODO, and mark that issue as fixed in the description?

nbauernfeind added barrage core Core development tasks DocumentationNeeded barrage-wrkr2wrkr ReleaseNotesNeeded Release notes are needed labels Sep 13, 2024

nbauernfeind added this to the 0.37.0 milestone Sep 13, 2024

nbauernfeind self-assigned this Sep 13, 2024

nbauernfeind requested review from niloc132 and rcaudy as code owners September 13, 2024 15:08

nbauernfeind marked this pull request as draft September 13, 2024 15:08

nbauernfeind force-pushed the barrage_types branch from a6ad4c9 to c4e970d Compare September 16, 2024 05:30

Barrage: Refactor Read/Write Chunk Factories

28b25d1

nbauernfeind force-pushed the barrage_types branch from c4e970d to 28b25d1 Compare September 16, 2024 14:21

rcaudy reviewed Sep 16, 2024

View reviewed changes

Ryan's feedback and bug fixes

8876208

niloc132 mentioned this pull request Oct 15, 2024

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

Open

nbauernfeind and others added 14 commits October 16, 2024 16:44

dunno .. stuf

27df3d3

Restore initial commit from grpc-java, plus a few local changes

1852b0a

Guard writing payload as hex if FINEST is enabled

68b925a

Apply upstream "Fix AsyncServletOutputStreamWriterConcurrencyTest

e88c47e

flakiness (#9948)"

Apply upstream "Avoid flushing headers when the server returns a single

f9a19fc

response (#9314)"

Apply upstream "servlet: introduce ServletServerBuilder.buildServlet()"

4733524

Bump grpc vers, add inprocess dep for tests

06e63ec

Merge branch 'main' into grpc-history-replay

09ade64

Apply deephaven#6301

c8af47c

Bump to 1.65.1 to better match arrow 18

57c8008

Merge remote-tracking branch 'colin/grpc-history-replay' into vp_simp…

cbf8ab2

…lification

Version Upgrades; MavenLocal

85f604f

Implement Simplified Viewport Table Updates in BMP/BT

70a0207

Ryan's Synchronous Review

0089d62

jcferretti mentioned this pull request Dec 2, 2024

fix: Boxed primitives trigger ClassCastException in ArrayPreview for protobuf repeated fields (#6434) #6461

Merged

nbauernfeind added 3 commits December 2, 2024 11:42

java client support for column as list feature

65d596d

finish getSqlInfo up to new schema

efa5dee

was able to acquire all union child writers

00b66a7

nbauernfeind added 7 commits December 3, 2024 16:36

impl most of union writer; need non-nullable support

a219b84

Can officially write sql info

675e745

Dense Union Reader + fixedChunkLength fixes

4eadb96

remove stubs for type mapping

4ae05b8

Bug fixes

be9f2df

Merge remote-tracking branch 'upstream/main' into barrage_types

c5000e7

More BugFixes

2dba609

nbauernfeind modified the milestones: 0.37.0, 0.37.1 Dec 5, 2024

nbauernfeind marked this pull request as ready for review December 5, 2024 03:55

nbauernfeind requested a review from cpwright as a code owner December 5, 2024 03:55

nbauernfeind changed the title ~~feat: [WIP] Barrage Refactor Read/Write Chunk Factories and Default Type Mappings~~ feat: Barrage Refactor Read/Write Chunk Factories and Default Type Mappings Dec 5, 2024

nbauernfeind changed the title ~~feat: Barrage Refactor Read/Write Chunk Factories and Default Type Mappings~~ feat: Barrage Refactor Read/Write Chunk Factories Dec 5, 2024

nbauernfeind and others added 2 commits December 4, 2024 21:50

fix go tests

9c3db73

revert: RST_STREAM(cancel) fix for gRPC, this seems to be breaking JS…

2ab1088

… gRPC client (deephaven#6420) This reverts commit 6ada0cb. See deephaven#6401 See deephaven#6400 See deephaven#5996

cpwright reviewed Dec 6, 2024

View reviewed changes

Charles' Feedback

ae71435

nbauernfeind requested a review from chipkent as a code owner December 16, 2024 16:00

cpwright reviewed Dec 16, 2024

View reviewed changes

nbauernfeind added 3 commits December 16, 2024 16:30

More Feedback + Tests + BugFixes

0400086

spotless + pull casting out of loops

1e68d61

WebReaderFactory support for ListView and FixedSizeList

fe1eb5c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Barrage Refactor Read/Write Chunk Factories #6065

feat: Barrage Refactor Read/Write Chunk Factories #6065

nbauernfeind commented Sep 13, 2024 •

edited

Loading

rcaudy left a comment

rcaudy Sep 16, 2024

nbauernfeind Nov 19, 2024

rcaudy Sep 16, 2024

cpwright left a comment

cpwright Dec 16, 2024

nbauernfeind Dec 16, 2024

cpwright Dec 16, 2024

nbauernfeind Dec 17, 2024

niloc132 commented Dec 20, 2024

feat: Barrage Refactor Read/Write Chunk Factories #6065

Are you sure you want to change the base?

feat: Barrage Refactor Read/Write Chunk Factories #6065

Conversation

nbauernfeind commented Sep 13, 2024 • edited Loading

rcaudy left a comment

Choose a reason for hiding this comment

rcaudy Sep 16, 2024

Choose a reason for hiding this comment

nbauernfeind Nov 19, 2024

Choose a reason for hiding this comment

rcaudy Sep 16, 2024

Choose a reason for hiding this comment

cpwright left a comment

Choose a reason for hiding this comment

cpwright Dec 16, 2024

Choose a reason for hiding this comment

nbauernfeind Dec 16, 2024

Choose a reason for hiding this comment

cpwright Dec 16, 2024

Choose a reason for hiding this comment

nbauernfeind Dec 17, 2024

Choose a reason for hiding this comment

niloc132 commented Dec 20, 2024

nbauernfeind commented Sep 13, 2024 •

edited

Loading