Update JS API to support gRPC input tables #1565
Conversation
web/client-api/src/main/java/io/deephaven/web/client/api/JsTable.java
Outdated
Show resolved
Hide resolved
web/client-api/src/main/java/io/deephaven/web/client/api/barrage/def/ColumnDefinition.java
Outdated
Show resolved
Hide resolved
web/client-api/src/main/java/io/deephaven/web/client/api/Column.java
Outdated
Show resolved
Hide resolved
...ent-api/src/main/java/io/deephaven/web/client/api/barrage/def/TableAttributesDefinition.java
Outdated
Show resolved
Hide resolved
| if (onlyTable.getColumns().length == keys.length && onlyTable.findColumns(keys).length == keys.length) { | ||
| ticketToDelete = onlyTable.getHandle().makeTicket(); | ||
| } else { | ||
| // view the only table |
There was a problem hiding this comment.
Shouldn't it throw if the tablesToDelete doesn't match the schema of the input table you're deleting from?
There was a problem hiding this comment.
It will throw, but not until later. I can add a local check here to give a clearer message.
niloc132
force-pushed
the
1271-js-input-tables
branch
from
17f0ed3 to
6c2f52d
Compare
...ent-api/src/main/java/io/deephaven/web/client/api/barrage/def/TableAttributesDefinition.java
Outdated
Show resolved
Hide resolved
| ResponseStreamWrapper<ExportedTableCreationResponse> wrapper = ResponseStreamWrapper.of( | ||
| table.getConnection().tableServiceClient().batch(batch, table.getConnection().metadata())); | ||
| wrapper.onData(response -> { | ||
| // kill the promise on the first failure we see |
There was a problem hiding this comment.
Is it possible to get more than one failure? Then we might be double rejecting the promise
There was a problem hiding this comment.
Double rejecting just means you lose a later reject (or resolve), no? This code is operating on that assumption for now, rather than accumulate errors or something - is that okay?
web/client-api/src/main/java/io/deephaven/web/client/api/parse/JsDataHandler.java
Show resolved
Hide resolved
web/client-api/src/main/java/io/deephaven/web/client/api/parse/JsDataHandler.java
Outdated
Show resolved
Hide resolved
web/client-api/src/main/java/io/deephaven/web/client/api/parse/JsDataHandler.java
Show resolved
Hide resolved
|
|
||
| // write the value, and mark non-null if necessary | ||
| if (boolValue != NULL_BOOLEAN_AS_BYTE) { | ||
| nulls.set(i); |
There was a problem hiding this comment.
this is inconsistent w.r.t. a corresponding nullCount -- however, I think this use case also applies; that we explicitly want to send the empty validity buffer.
There was a problem hiding this comment.
At this time, I am not sending a dhNulls=true, but doing a vanilla DoPut, just with deephaven:type metadata.
| private static void writeSimpleNumbers(Object[] data, JsConsumer<Node> addNode, JsConsumer<Uint8Array> addBuffer, | ||
| double bytesPerElement, double nullValue, JsFunction<ArrayBuffer, ? extends TypedArray> bufferConstructor) { | ||
| int nullCount = 0; | ||
| BitSet nulls = new BitSet(data.length); |
There was a problem hiding this comment.
same comment here on the null counting; I'd send the empty validity buffer explicitly
| table.getConnection().inputTableServiceClient().addTableToInputTable(addTableRequest, | ||
| table.getConnection().metadata(), c::apply); | ||
| }).then(success -> { | ||
| if (merged != tablesToAdd[0]) { |
There was a problem hiding this comment.
I understand the check; but correctness might be more obvious if you stick to the length check like on line 101.
There was a problem hiding this comment.
The goal in having a different check was to acknowledge "this is an intermediate instance so it must be closed" - if the logic between here and 101 changes, I would want to allow for this still to be correct.
What do you think of something instead like Arrays.stream(tablesToAdd).allMatch(t -> t != merged)?
niloc132
force-pushed
the
1271-js-input-tables
branch
from
e8ce571 to
a950e51
Compare
Building on #1440, this adds support in the JS API for input tables.
CSV upload is rebuilt in this as well, and expanded to be more generic, so that input tables can reuse the same logic. This should be at least as flexible as the UI previously required (though the ability to handle arrays of data has been removed temporarily, but to my knowledge it was never in active use).