Skip to content

Commit 62f325a

Browse files
niloc132niloc132
authored
fix: JS ChartData should write updates to the correct row (deephaven#6638)
The provided mapping function was also not applied to modified rows, also fixed in this patch. Also fixed an error in typescript types/docs for the ChartData.update() parameter. Regression introduced by deephaven#6076. Fixes DH-18632
1 parent 83490e2 commit 62f325a

File tree

3 files changed

+199
-24
lines changed

3 files changed

+199
-24
lines changed

‎web/client-api/src/main/java/io/deephaven/web/client/api/widget/plot/ChartData.java

+3-4
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,6 @@
77
import io.deephaven.web.client.api.Column;
88
import io.deephaven.web.client.api.JsTable;
99
import io.deephaven.web.client.api.TableData;
10-
import io.deephaven.web.client.api.subscription.AbstractTableSubscription;
1110
import io.deephaven.web.client.api.subscription.SubscriptionTableData;
1211
import io.deephaven.web.shared.data.Range;
1312
import io.deephaven.web.shared.data.RangeSet;
@@ -35,7 +34,7 @@ public ChartData(JsTable table) {
3534
this.table = table;
3635
}
3736

38-
public void update(AbstractTableSubscription.SubscriptionEventData tableData) {
37+
public void update(SubscriptionTableData tableData) {
3938
RangeSet positionsForAddedKeys = tableData.getFullIndex().getRange().invert(tableData.getAdded().getRange());
4039
RangeSet positionsForRemovedKeys = prevRanges.invert(tableData.getRemoved().getRange());
4140
RangeSet positionsForModifiedKeys =
@@ -82,14 +81,14 @@ private void replaceDataRange(SubscriptionTableData tableData, Range positions)
8281

8382
// rather than getting a slice and splicing it in, just update each value
8483
PrimitiveIterator.OfLong iter = keys.indexIterator();
85-
int i = 0;
84+
int i = (int) positions.getFirst();
8685
if (func == null) {
8786
while (iter.hasNext()) {
8887
arr.setAt(i++, tableData.getData(iter.next(), col));
8988
}
9089
} else {
9190
while (iter.hasNext()) {
92-
arr.setAt(i++, tableData.getData(iter.next(), col));
91+
arr.setAt(i++, func.apply(tableData.getData(iter.next(), col)));
9392
}
9493
}
9594
}

‎web/client-api/src/test/java/io/deephaven/web/client/api/AbstractAsyncGwtTestCase.java

+7-20
Original file line numberDiff line numberDiff line change
@@ -56,8 +56,8 @@ private static Promise<Void> importDhInternal() {
5656
public static class TableSourceBuilder {
5757
private final List<String> pythonScripts = new ArrayList<>();
5858

59-
public TableSourceBuilder script(String script) {
60-
pythonScripts.add(script);
59+
public TableSourceBuilder script(String... script) {
60+
pythonScripts.addAll(Arrays.asList(script));
6161
return this;
6262
}
6363

@@ -156,7 +156,7 @@ protected Promise<IdeSession> connect(TableSourceBuilder tables) {
156156
return ideSession.then(session -> {
157157

158158
if (consoleTypes.includes("python")) {
159-
return runAllScriptsInOrder(ideSession, session, tables.pythonScripts);
159+
return runAllScriptsInOrder(session, tables.pythonScripts);
160160
}
161161
throw new IllegalStateException("Unsupported script type " + consoleTypes);
162162
});
@@ -165,23 +165,10 @@ protected Promise<IdeSession> connect(TableSourceBuilder tables) {
165165
});
166166
}
167167

168-
private Promise<IdeSession> runAllScriptsInOrder(CancellablePromise<IdeSession> ideSession, IdeSession session,
169-
List<String> code) {
170-
Promise<IdeSession> result = ideSession;
171-
for (int i = 0; i < code.size(); i++) {
172-
final int index = i;
173-
result = result.then(ignore -> {
174-
delayTestFinish(4000 + index);
175-
176-
return session.runCode(code.get(index));
177-
}).then(r -> {
178-
if (r.getError() != null) {
179-
return Promise.reject(r.getError());
180-
}
181-
return ideSession;
182-
});
183-
}
184-
return result;
168+
private Promise<IdeSession> runAllScriptsInOrder(IdeSession session, List<String> code) {
169+
String block = String.join("\n", code);
170+
delayTestFinish(4000);
171+
return session.runCode(block).then(ignore -> Promise.resolve(session));
185172
}
186173

187174
public IThenable.ThenOnFulfilledCallbackFn<IdeSession, JsTable> table(String tableName) {

‎web/client-api/src/test/java/io/deephaven/web/client/api/widget/plot/ChartDataTestGwt.java

+189
Original file line numberDiff line numberDiff line change
@@ -3,17 +3,40 @@
33
//
44
package io.deephaven.web.client.api.widget.plot;
55

6+
import elemental2.core.JsArray;
67
import elemental2.promise.Promise;
78
import io.deephaven.web.client.api.AbstractAsyncGwtTestCase;
9+
import io.deephaven.web.client.api.Column;
10+
import io.deephaven.web.client.api.JsTable;
811
import io.deephaven.web.client.api.subscription.AbstractTableSubscription;
12+
import io.deephaven.web.client.api.subscription.SubscriptionTableData;
913
import io.deephaven.web.client.api.subscription.TableSubscription;
14+
import io.deephaven.web.shared.fu.JsFunction;
15+
import jsinterop.base.Any;
16+
import jsinterop.base.Js;
17+
import jsinterop.base.JsPropertyMap;
18+
19+
import java.io.Serializable;
20+
import java.util.function.BiConsumer;
1021

1122
public class ChartDataTestGwt extends AbstractAsyncGwtTestCase {
1223
private final AbstractAsyncGwtTestCase.TableSourceBuilder tables = new AbstractAsyncGwtTestCase.TableSourceBuilder()
1324
.script("from deephaven import time_table")
1425
.script("appending_table", "time_table('PT0.1s').update([\"A = i % 2\", \"B = `` + A\"])")
1526
.script("updating_table", "appending_table.last_by('A')");
1627

28+
private final TableSourceBuilder tickingData = new TableSourceBuilder()
29+
.script("from deephaven import input_table",
30+
"from deephaven import dtypes as dht",
31+
"import jpy",
32+
"from deephaven.execution_context import ExecutionContext, get_exec_ctx",
33+
"my_col_defs = {\"ID\": dht.int32, \"Value\": dht.string, \"Deleted\": dht.bool_}",
34+
"ug = jpy.get_type('io.deephaven.engine.updategraph.impl.EventDrivenUpdateGraph').newBuilder('test').existingOrBuild()",
35+
"exec_ctx = ExecutionContext(get_exec_ctx().j_object.withUpdateGraph(ug))",
36+
"with exec_ctx:",
37+
" input = input_table(col_defs=my_col_defs, key_cols=\"ID\")",
38+
" result = input.without_attributes('InputTable').where(\"!Deleted\").sort(\"ID\")");
39+
1740
@Override
1841
public String getModuleName() {
1942
return "io.deephaven.web.DeephavenIntegrationTest";
@@ -89,4 +112,170 @@ public void testModifiedChartData() {
89112
})
90113
.then(this::finish).catch_(this::report);
91114
}
115+
116+
public void testGeneratedChartData() {
117+
connect(tickingData)
118+
.then(ideSession -> {
119+
delayTestFinish(9870);
120+
Promise<JsTable> inputPromise = ideSession.getTable("input", false);
121+
Promise<JsTable> resultPromise = ideSession.getTable("result", false);
122+
return inputPromise.then(JsTable::inputTable).then(delayFinish(9871)).then(input -> {
123+
delayTestFinish(9872);
124+
return resultPromise.then(result -> {
125+
delayTestFinish(9873);
126+
ChartData chartData = new ChartData(result);
127+
128+
// Keep the same function instance, as it is used for caching data
129+
JsFunction<Any, Any> dataFunction = arr -> Js.asAny("::" + arr);
130+
131+
BiConsumer<SubscriptionTableData, SubscriptionTableData> consistencyCheck =
132+
(subscriptionUpdate, snapshot) -> {
133+
chartData.update(subscriptionUpdate);
134+
// Verify the "Value" column, the only mutable column, has the same contents
135+
// regardless of source
136+
String[] expectedValues = new String[snapshot.getRows().length];
137+
snapshot.getRows().forEach((row, index) -> {
138+
expectedValues[index] = row.get(result.findColumn("Value")).asString();
139+
return null;
140+
});
141+
JsArray<Any> values = chartData.getColumn("Value", null, subscriptionUpdate);
142+
JsArray<Any> valuesWithPrefix =
143+
chartData.getColumn("Value", dataFunction, subscriptionUpdate);
144+
assertEquals(values.length, valuesWithPrefix.length);
145+
assertEquals(expectedValues.length, values.length);
146+
for (int i = 0; i < expectedValues.length; i++) {
147+
assertEquals(expectedValues[i], values.getAt(i).asString());
148+
assertEquals("::" + expectedValues[i],
149+
valuesWithPrefix.getAt(i).asString());
150+
}
151+
};
152+
return subscriptionValidator(result, result.getColumns())
153+
// Check initial state (empty)
154+
.check(consistencyCheck)
155+
// Add three rows
156+
.when(() -> {
157+
input.addRows(new JsPropertyMap[] {
158+
row(1, "A"),
159+
row(2, "B"),
160+
row(5, "E")
161+
}, null);
162+
})
163+
.check(consistencyCheck)
164+
// Add two more rows, causing a shift
165+
.when(() -> {
166+
input.addRows(new JsPropertyMap[] {
167+
row(3, "C"),
168+
row(4, "D")
169+
}, null);
170+
})
171+
.check(consistencyCheck)
172+
// Update an existing row
173+
.when(() -> {
174+
input.addRows(new JsPropertyMap[] {
175+
row(3, "F")
176+
}, null);
177+
})
178+
.check(consistencyCheck)
179+
// Delete two rows
180+
.when(() -> {
181+
input.addRows(new JsPropertyMap[] {
182+
delete(1),
183+
delete(3)
184+
}, null);
185+
})
186+
.check(consistencyCheck)
187+
.cleanup();
188+
});
189+
});
190+
})
191+
.then(this::finish).catch_(this::report);
192+
}
193+
194+
/**
195+
* Helper to validate subscription results by comparing an updated subscription against a fresh snapshot. Intended
196+
* to be generalized for use in viewport tests, but haven't yet done this.
197+
*/
198+
private SubscriptionValidator subscriptionValidator(JsTable table, JsArray<Column> subscriptionColumns) {
199+
TableSubscription subscription = table.subscribe(subscriptionColumns);
200+
201+
return new SubscriptionValidator() {
202+
Promise<?> nextStep = Promise.resolve(subscription);
203+
204+
@Override
205+
public SubscriptionValidator when(Runnable setup) {
206+
// Run the step when the previous step completes
207+
nextStep = nextStep.then(ignore -> {
208+
setup.run();
209+
return Promise.resolve(ignore);
210+
});
211+
return this;
212+
}
213+
214+
@Override
215+
public SubscriptionValidator check(BiConsumer<SubscriptionTableData, SubscriptionTableData> check) {
216+
// Run this as a step once the previous step completes, by waiting for the original subscription to
217+
// update, then creating a new subscription and waiting for that as well. Pass both updates to the
218+
// provided check function.
219+
nextStep = nextStep.then(ignore -> new Promise<>((resolve, reject) -> {
220+
try {
221+
subscription.addEventListenerOneShot(TableSubscription.EVENT_UPDATED, e1 -> {
222+
try {
223+
SubscriptionTableData data = (SubscriptionTableData) e1.getDetail();
224+
// Now that this update has happened, we make a new subscription to get a single
225+
// snapshot of the table
226+
TableSubscription checkSub = table.subscribe(subscriptionColumns);
227+
checkSub.addEventListenerOneShot(TableSubscription.EVENT_UPDATED, e2 -> {
228+
try {
229+
SubscriptionTableData checkData = (SubscriptionTableData) e2.getDetail();
230+
check.accept(data, checkData);
231+
resolve.onInvoke(checkData);
232+
} catch (Throwable e) {
233+
reject.onInvoke(e);
234+
} finally {
235+
checkSub.close();
236+
}
237+
});
238+
} catch (Throwable e) {
239+
reject.onInvoke(e);
240+
}
241+
});
242+
} catch (Throwable e) {
243+
reject.onInvoke(e);
244+
}
245+
}));
246+
return this;
247+
}
248+
249+
@Override
250+
public Promise<?> cleanup() {
251+
// Finished, clean up after ourselves, emit success/failure
252+
return nextStep.finally_(subscription::close);
253+
}
254+
};
255+
}
256+
257+
private interface SubscriptionValidator {
258+
/**
259+
* Performs some step that will lead to the table subscription being updated.
260+
*/
261+
SubscriptionValidator when(Runnable setup);
262+
263+
/**
264+
* Checks that the actual and expected data match. Must complete synchronously.
265+
*/
266+
SubscriptionValidator check(BiConsumer<SubscriptionTableData, SubscriptionTableData> check);
267+
268+
/**
269+
* Cleans up after validation work, returning a promise that the test can await.
270+
*/
271+
Promise<?> cleanup();
272+
}
273+
274+
private static JsPropertyMap<? extends Serializable> row(int id, String value) {
275+
return JsPropertyMap.of("ID", (double) id, "Value", value, "Deleted", false);
276+
}
277+
278+
private static JsPropertyMap<? extends Serializable> delete(int id) {
279+
return JsPropertyMap.of("ID", (double) id, "Value", "deleted", "Deleted", true);
280+
}
92281
}

0 commit comments

Comments
 (0)