fix: DH-18655: JS API can upload the same types it can download#7995
Open
niloc132 wants to merge 21 commits intodeephaven:mainfrom
Open
fix: DH-18655: JS API can upload the same types it can download#7995niloc132 wants to merge 21 commits intodeephaven:mainfrom
niloc132 wants to merge 21 commits intodeephaven:mainfrom
Conversation
Contributor
No docs changes detected for f55499d |
cpwright
reviewed
May 7, 2026
cpwright
reviewed
May 8, 2026
Contributor
cpwright
left a comment
cpwright
left a comment
There was a problem hiding this comment.
The only serious thing is the question about null handling for some types, I am worried that a null BigInteger or friends will result in an NPE as we try to write it.
| case Type.Binary: { | ||
| if (typeInfo.type() == BigIntegerWrapper.class) { | ||
| return (ChunkWriter<T>) new VarBinaryChunkWriter<BigIntegerWrapper>(true, | ||
| (out, item) -> out.write(item.getWrapped().toByteArray())); |
Contributor
There was a problem hiding this comment.
We don't have to worry about item nulls here or in BigDecimal/utf8?
Comment on lines
+68
to
+70
| if (this.isEmpty() || rowSet.isEmpty()) { | ||
| return RowSetFactory.empty(); | ||
| } |
Contributor
There was a problem hiding this comment.
I would flip the empty checks, which should be cheap compared to an equals.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Uses BarrageMessageWriterImpl to serialize chunks to upload with DoPut from the JS API, ensuring that we have matching chunk reader and writer support.
Extends our JS emulation to handle more of RowSet, including RowSetShiftData and BarrageMessage. This patch does not go further to remove WebBarrageMessage where it exists already and read/write the same message type, but that probably will be helpful eventually.
JsDataHandler still serves a role here, normalizing arrays of data passed from JS into a form that we can wrap with chunks and write to the server. It also has mappings for JVM types and how we expect to encode them in flight.