From 62f325a09e40f77b2fd58e84e253b39e6fd832ad Mon Sep 17 00:00:00 2001 From: Colin Alworth Date: Wed, 12 Feb 2025 09:46:36 -0600 Subject: [PATCH] fix: JS ChartData should write updates to the correct row (#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 #6076. Fixes DH-18632 --- .../web/client/api/widget/plot/ChartData.java | 7 +- .../client/api/AbstractAsyncGwtTestCase.java | 27 +-- .../api/widget/plot/ChartDataTestGwt.java | 189 ++++++++++++++++++ 3 files changed, 199 insertions(+), 24 deletions(-) diff --git a/web/client-api/src/main/java/io/deephaven/web/client/api/widget/plot/ChartData.java b/web/client-api/src/main/java/io/deephaven/web/client/api/widget/plot/ChartData.java index 74c39363605..f2c3fc88159 100644 --- a/web/client-api/src/main/java/io/deephaven/web/client/api/widget/plot/ChartData.java +++ b/web/client-api/src/main/java/io/deephaven/web/client/api/widget/plot/ChartData.java @@ -7,7 +7,6 @@ import io.deephaven.web.client.api.Column; import io.deephaven.web.client.api.JsTable; import io.deephaven.web.client.api.TableData; -import io.deephaven.web.client.api.subscription.AbstractTableSubscription; import io.deephaven.web.client.api.subscription.SubscriptionTableData; import io.deephaven.web.shared.data.Range; import io.deephaven.web.shared.data.RangeSet; @@ -35,7 +34,7 @@ public ChartData(JsTable table) { this.table = table; } - public void update(AbstractTableSubscription.SubscriptionEventData tableData) { + public void update(SubscriptionTableData tableData) { RangeSet positionsForAddedKeys = tableData.getFullIndex().getRange().invert(tableData.getAdded().getRange()); RangeSet positionsForRemovedKeys = prevRanges.invert(tableData.getRemoved().getRange()); RangeSet positionsForModifiedKeys = @@ -82,14 +81,14 @@ private void replaceDataRange(SubscriptionTableData tableData, Range positions) // rather than getting a slice and splicing it in, just update each value PrimitiveIterator.OfLong iter = keys.indexIterator(); - int i = 0; + int i = (int) positions.getFirst(); if (func == null) { while (iter.hasNext()) { arr.setAt(i++, tableData.getData(iter.next(), col)); } } else { while (iter.hasNext()) { - arr.setAt(i++, tableData.getData(iter.next(), col)); + arr.setAt(i++, func.apply(tableData.getData(iter.next(), col))); } } } diff --git a/web/client-api/src/test/java/io/deephaven/web/client/api/AbstractAsyncGwtTestCase.java b/web/client-api/src/test/java/io/deephaven/web/client/api/AbstractAsyncGwtTestCase.java index 33af08dadf4..eb8b4b3c80c 100644 --- a/web/client-api/src/test/java/io/deephaven/web/client/api/AbstractAsyncGwtTestCase.java +++ b/web/client-api/src/test/java/io/deephaven/web/client/api/AbstractAsyncGwtTestCase.java @@ -56,8 +56,8 @@ private static Promise importDhInternal() { public static class TableSourceBuilder { private final List pythonScripts = new ArrayList<>(); - public TableSourceBuilder script(String script) { - pythonScripts.add(script); + public TableSourceBuilder script(String... script) { + pythonScripts.addAll(Arrays.asList(script)); return this; } @@ -156,7 +156,7 @@ protected Promise connect(TableSourceBuilder tables) { return ideSession.then(session -> { if (consoleTypes.includes("python")) { - return runAllScriptsInOrder(ideSession, session, tables.pythonScripts); + return runAllScriptsInOrder(session, tables.pythonScripts); } throw new IllegalStateException("Unsupported script type " + consoleTypes); }); @@ -165,23 +165,10 @@ protected Promise connect(TableSourceBuilder tables) { }); } - private Promise runAllScriptsInOrder(CancellablePromise ideSession, IdeSession session, - List code) { - Promise result = ideSession; - for (int i = 0; i < code.size(); i++) { - final int index = i; - result = result.then(ignore -> { - delayTestFinish(4000 + index); - - return session.runCode(code.get(index)); - }).then(r -> { - if (r.getError() != null) { - return Promise.reject(r.getError()); - } - return ideSession; - }); - } - return result; + private Promise runAllScriptsInOrder(IdeSession session, List code) { + String block = String.join("\n", code); + delayTestFinish(4000); + return session.runCode(block).then(ignore -> Promise.resolve(session)); } public IThenable.ThenOnFulfilledCallbackFn table(String tableName) { diff --git a/web/client-api/src/test/java/io/deephaven/web/client/api/widget/plot/ChartDataTestGwt.java b/web/client-api/src/test/java/io/deephaven/web/client/api/widget/plot/ChartDataTestGwt.java index 1da60a23664..378a63eb337 100644 --- a/web/client-api/src/test/java/io/deephaven/web/client/api/widget/plot/ChartDataTestGwt.java +++ b/web/client-api/src/test/java/io/deephaven/web/client/api/widget/plot/ChartDataTestGwt.java @@ -3,10 +3,21 @@ // package io.deephaven.web.client.api.widget.plot; +import elemental2.core.JsArray; import elemental2.promise.Promise; import io.deephaven.web.client.api.AbstractAsyncGwtTestCase; +import io.deephaven.web.client.api.Column; +import io.deephaven.web.client.api.JsTable; import io.deephaven.web.client.api.subscription.AbstractTableSubscription; +import io.deephaven.web.client.api.subscription.SubscriptionTableData; import io.deephaven.web.client.api.subscription.TableSubscription; +import io.deephaven.web.shared.fu.JsFunction; +import jsinterop.base.Any; +import jsinterop.base.Js; +import jsinterop.base.JsPropertyMap; + +import java.io.Serializable; +import java.util.function.BiConsumer; public class ChartDataTestGwt extends AbstractAsyncGwtTestCase { private final AbstractAsyncGwtTestCase.TableSourceBuilder tables = new AbstractAsyncGwtTestCase.TableSourceBuilder() @@ -14,6 +25,18 @@ public class ChartDataTestGwt extends AbstractAsyncGwtTestCase { .script("appending_table", "time_table('PT0.1s').update([\"A = i % 2\", \"B = `` + A\"])") .script("updating_table", "appending_table.last_by('A')"); + private final TableSourceBuilder tickingData = new TableSourceBuilder() + .script("from deephaven import input_table", + "from deephaven import dtypes as dht", + "import jpy", + "from deephaven.execution_context import ExecutionContext, get_exec_ctx", + "my_col_defs = {\"ID\": dht.int32, \"Value\": dht.string, \"Deleted\": dht.bool_}", + "ug = jpy.get_type('io.deephaven.engine.updategraph.impl.EventDrivenUpdateGraph').newBuilder('test').existingOrBuild()", + "exec_ctx = ExecutionContext(get_exec_ctx().j_object.withUpdateGraph(ug))", + "with exec_ctx:", + " input = input_table(col_defs=my_col_defs, key_cols=\"ID\")", + " result = input.without_attributes('InputTable').where(\"!Deleted\").sort(\"ID\")"); + @Override public String getModuleName() { return "io.deephaven.web.DeephavenIntegrationTest"; @@ -89,4 +112,170 @@ public void testModifiedChartData() { }) .then(this::finish).catch_(this::report); } + + public void testGeneratedChartData() { + connect(tickingData) + .then(ideSession -> { + delayTestFinish(9870); + Promise inputPromise = ideSession.getTable("input", false); + Promise resultPromise = ideSession.getTable("result", false); + return inputPromise.then(JsTable::inputTable).then(delayFinish(9871)).then(input -> { + delayTestFinish(9872); + return resultPromise.then(result -> { + delayTestFinish(9873); + ChartData chartData = new ChartData(result); + + // Keep the same function instance, as it is used for caching data + JsFunction dataFunction = arr -> Js.asAny("::" + arr); + + BiConsumer consistencyCheck = + (subscriptionUpdate, snapshot) -> { + chartData.update(subscriptionUpdate); + // Verify the "Value" column, the only mutable column, has the same contents + // regardless of source + String[] expectedValues = new String[snapshot.getRows().length]; + snapshot.getRows().forEach((row, index) -> { + expectedValues[index] = row.get(result.findColumn("Value")).asString(); + return null; + }); + JsArray values = chartData.getColumn("Value", null, subscriptionUpdate); + JsArray valuesWithPrefix = + chartData.getColumn("Value", dataFunction, subscriptionUpdate); + assertEquals(values.length, valuesWithPrefix.length); + assertEquals(expectedValues.length, values.length); + for (int i = 0; i < expectedValues.length; i++) { + assertEquals(expectedValues[i], values.getAt(i).asString()); + assertEquals("::" + expectedValues[i], + valuesWithPrefix.getAt(i).asString()); + } + }; + return subscriptionValidator(result, result.getColumns()) + // Check initial state (empty) + .check(consistencyCheck) + // Add three rows + .when(() -> { + input.addRows(new JsPropertyMap[] { + row(1, "A"), + row(2, "B"), + row(5, "E") + }, null); + }) + .check(consistencyCheck) + // Add two more rows, causing a shift + .when(() -> { + input.addRows(new JsPropertyMap[] { + row(3, "C"), + row(4, "D") + }, null); + }) + .check(consistencyCheck) + // Update an existing row + .when(() -> { + input.addRows(new JsPropertyMap[] { + row(3, "F") + }, null); + }) + .check(consistencyCheck) + // Delete two rows + .when(() -> { + input.addRows(new JsPropertyMap[] { + delete(1), + delete(3) + }, null); + }) + .check(consistencyCheck) + .cleanup(); + }); + }); + }) + .then(this::finish).catch_(this::report); + } + + /** + * Helper to validate subscription results by comparing an updated subscription against a fresh snapshot. Intended + * to be generalized for use in viewport tests, but haven't yet done this. + */ + private SubscriptionValidator subscriptionValidator(JsTable table, JsArray subscriptionColumns) { + TableSubscription subscription = table.subscribe(subscriptionColumns); + + return new SubscriptionValidator() { + Promise nextStep = Promise.resolve(subscription); + + @Override + public SubscriptionValidator when(Runnable setup) { + // Run the step when the previous step completes + nextStep = nextStep.then(ignore -> { + setup.run(); + return Promise.resolve(ignore); + }); + return this; + } + + @Override + public SubscriptionValidator check(BiConsumer check) { + // Run this as a step once the previous step completes, by waiting for the original subscription to + // update, then creating a new subscription and waiting for that as well. Pass both updates to the + // provided check function. + nextStep = nextStep.then(ignore -> new Promise<>((resolve, reject) -> { + try { + subscription.addEventListenerOneShot(TableSubscription.EVENT_UPDATED, e1 -> { + try { + SubscriptionTableData data = (SubscriptionTableData) e1.getDetail(); + // Now that this update has happened, we make a new subscription to get a single + // snapshot of the table + TableSubscription checkSub = table.subscribe(subscriptionColumns); + checkSub.addEventListenerOneShot(TableSubscription.EVENT_UPDATED, e2 -> { + try { + SubscriptionTableData checkData = (SubscriptionTableData) e2.getDetail(); + check.accept(data, checkData); + resolve.onInvoke(checkData); + } catch (Throwable e) { + reject.onInvoke(e); + } finally { + checkSub.close(); + } + }); + } catch (Throwable e) { + reject.onInvoke(e); + } + }); + } catch (Throwable e) { + reject.onInvoke(e); + } + })); + return this; + } + + @Override + public Promise cleanup() { + // Finished, clean up after ourselves, emit success/failure + return nextStep.finally_(subscription::close); + } + }; + } + + private interface SubscriptionValidator { + /** + * Performs some step that will lead to the table subscription being updated. + */ + SubscriptionValidator when(Runnable setup); + + /** + * Checks that the actual and expected data match. Must complete synchronously. + */ + SubscriptionValidator check(BiConsumer check); + + /** + * Cleans up after validation work, returning a promise that the test can await. + */ + Promise cleanup(); + } + + private static JsPropertyMap row(int id, String value) { + return JsPropertyMap.of("ID", (double) id, "Value", value, "Deleted", false); + } + + private static JsPropertyMap delete(int id) { + return JsPropertyMap.of("ID", (double) id, "Value", "deleted", "Deleted", true); + } }