feat: Barrage Refactor Read/Write Chunk Factories #6065

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

This is an overhaul of type mapping from arrow to deephaven columns. We can now coerce from many kinds of types, enabling widening / truncating and conversion between types. For example uint64_t is now mapped to BigInteger by default, but can be explicitly mapped to long.class (may overflow).

To set an explicit mapping set the attribute "deephaven:type" to the canonical name of the class.

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).

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

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

…Time64 to ColumnSource

extensions/flight-sql/src/main/java/io/deephaven/server/flightsql/FlightSqlResolver.java

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

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

...sions/barrage/src/main/java/io/deephaven/extensions/barrage/chunk/BigDecimalChunkWriter.java

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

...rage/src/main/java/io/deephaven/extensions/barrage/chunk/array/ByteArrayExpansionKernel.java

...ge/src/main/java/io/deephaven/extensions/barrage/chunk/vector/ByteVectorExpansionKernel.java

...e/src/main/java/io/deephaven/extensions/barrage/chunk/array/BooleanArrayExpansionKernel.java

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

extensions/barrage/src/main/java/io/deephaven/extensions/barrage/util/BarrageUtil.java

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

cpwright · 2025-02-10T18:41:15Z

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

+                        final float value = src.get(ii);
+                        dst.set(dstOffset + ii, value == QueryConstants.NULL_FLOAT
+                                ? null
+                                : BigInteger.valueOf((long) value));


The long conversion in the middle here is causing us to truncate things inappropriately. I think you need to do a BigDecimal.valueOf followed by a toBigInteger. Same for double. For the Float16, we should be safe.

Interesting. I wonder why the random tests never pick a value that hit the early truncation. Maybe my equality tolerance is too loose?

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

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

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

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

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

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

cpwright · 2025-02-11T14:27:57Z

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

@@ -0,0 +1,74 @@
+//


This doesn't get hit in the Java test, but presumably it is fine because we are exercising it with C++ tests on each nightly build?

There are explicit unit tests for this; just not in the type-cross sectional use. (FlightMessageRoundTripTest)

I ran full coverage with Stan's jacoco merge tool, and it is still a zero:

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

...ge/src/main/java/io/deephaven/extensions/barrage/chunk/vector/CharVectorExpansionKernel.java

...e/src/main/java/io/deephaven/extensions/barrage/chunk/array/BooleanArrayExpansionKernel.java

.../main/java/io/deephaven/extensions/barrage/chunk/array/BoxedBooleanArrayExpansionKernel.java

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

nbauernfeind and others added 17 commits January 30, 2025 06:42

Interval/Errors/CustomBinary/Union/Head/Tail Tests

767e7a1

rm extra newline

93cd532

feat(cpp-client): Honor Arrow TimeUnit when converting Timestamp and …

f40b628

…Time64 to ColumnSource

generic union test + fixes

0db9d19

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

fae471f

update copyright headers

60a23b9

Default mapping for Map and Union; fix TODOs

d121063

Most of hd/tl testing

34f1406

Fix the other not-working tests

df7b92d

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

1eb6c4d

fix type propagation to client

bfdb17c

actually fix deephaven#6114

52a486d

Merge remote-tracking branch 'kosak/kosak_time-units' into barrage_types

614d172

W/R and cpp test fix

a7a6361

cpp tests don't run with this name

af1e760

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

48b5884

Reference arrow-java issue for List of Union

36ce6ae

cpwright reviewed Feb 10, 2025

View reviewed changes

revert bool object vector.. need to debug more

4e3a1f6

cpwright reviewed Feb 10, 2025

View reviewed changes

cpwright reviewed Feb 11, 2025

View reviewed changes

nbauernfeind added 3 commits February 11, 2025 09:37

2nd attempt at ObjectVector<Boolean> w2w transfer

d3c302b

initial quick set of feedback

1904411

apply spotless

bad569f

cpwright reviewed Feb 11, 2025

View reviewed changes

...e/src/main/java/io/deephaven/extensions/barrage/chunk/array/BooleanArrayExpansionKernel.java Show resolved Hide resolved

.../main/java/io/deephaven/extensions/barrage/chunk/array/BoxedBooleanArrayExpansionKernel.java Show resolved Hide resolved

nbauernfeind added 4 commits February 18, 2025 12:20

checkpoint charles' feedback

85b7636

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

336af7c

fix go client test

21d91f8

Fix a few more tests and real bugs

2e791ae

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 Feb 10, 2025

nbauernfeind Feb 11, 2025

cpwright Feb 11, 2025

nbauernfeind Feb 11, 2025

cpwright Feb 12, 2025

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nbauernfeind commented Sep 13, 2024 •

edited

Loading