Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: JS API rollups with unique aggs must specify null sentinel (#6202) #6409

Merged
merged 1 commit into from
Nov 22, 2024
Merged

fix: JS API rollups with unique aggs must specify null sentinel (#6202) #6409

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions web/client-api/src/main/java/io/deephaven/web/client/api/tree/JsRollupConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
import elemental2.core.JsArray;
import elemental2.core.JsObject;
import elemental2.core.JsString;
import io.deephaven.javascript.proto.dhinternal.io.deephaven.proto.Table_pb;
import io.deephaven.javascript.proto.dhinternal.io.deephaven.proto.hierarchicaltable_pb.RollupRequest;
import io.deephaven.javascript.proto.dhinternal.io.deephaven.proto.table_pb.AggSpec;
import io.deephaven.javascript.proto.dhinternal.io.deephaven.proto.table_pb.Aggregation;
Expand All @@ -20,6 +21,7 @@
import io.deephaven.javascript.proto.dhinternal.io.deephaven.proto.table_pb.aggspec.AggSpecLast;
import io.deephaven.javascript.proto.dhinternal.io.deephaven.proto.table_pb.aggspec.AggSpecMax;
import io.deephaven.javascript.proto.dhinternal.io.deephaven.proto.table_pb.aggspec.AggSpecMin;
import io.deephaven.javascript.proto.dhinternal.io.deephaven.proto.table_pb.aggspec.AggSpecNonUniqueSentinel;
import io.deephaven.javascript.proto.dhinternal.io.deephaven.proto.table_pb.aggspec.AggSpecStd;
import io.deephaven.javascript.proto.dhinternal.io.deephaven.proto.table_pb.aggspec.AggSpecSum;
import io.deephaven.javascript.proto.dhinternal.io.deephaven.proto.table_pb.aggspec.AggSpecUnique;
Expand Down Expand Up @@ -58,7 +60,7 @@ public class JsRollupConfig {
* Mapping from each aggregation name to the ordered list of columns it should be applied to in the resulting
* roll-up table.
*/
public JsPropertyMap<JsArray<@TsTypeRef(JsAggregationOperation.class) String>> aggregations =
public JsPropertyMap<JsArray<String>> aggregations =
Js.cast(JsObject.create(null));
/**
* Optional parameter indicating if an extra leaf node should be added at the bottom of the hierarchy, showing the
Expand Down Expand Up @@ -245,7 +247,11 @@ public RollupRequest buildRequest(JsArray<Column> tableColumns) {
}
case JsAggregationOperation.UNIQUE: {
AggSpec spec = new AggSpec();
spec.setUnique(new AggSpecUnique());
AggSpecUnique unique = new AggSpecUnique();
AggSpecNonUniqueSentinel sentinel = new AggSpecNonUniqueSentinel();
sentinel.setNullValue(Table_pb.NullValue.getNULL_VALUE());
unique.setNonUniqueSentinel(sentinel);
spec.setUnique(unique);
columns = new AggregationColumns();
columns.setSpec(spec);
agg.setColumns(columns);
Expand Down
4 changes: 4 additions & 0 deletions web/client-api/src/main/java/io/deephaven/web/public/rollup.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,14 +55,18 @@ <h3>Rollup Table Configuration</h3>
<label for="operation">Aggregation Operation</label>
<select id="operation">
<option value="Count">Count</option>
<option value="CountDistinct">Count Distinct</option>
<option value="Distinct">Distinct</option>
<option value="Min">Min</option>
<option value="Max">Max</option>
<option value="Sum" selected>Sum</option>
<option value="AbsSum" selected>Absolute Sum</option>
<option value="Var">Var</option>
<option value="Avg">Avg</option>
<option value="Std">Std</option>
<option value="First">First</option>
<option value="Last">Last</option>
<option value="Unique">Unique</option>
</select>
</div>
</div>
Expand Down
64 changes: 62 additions & 2 deletions web/client-api/src/test/java/io/deephaven/web/client/api/HierarchicalTableTestGwt.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,18 @@
//
package io.deephaven.web.client.api;

import elemental2.core.JsArray;
import elemental2.dom.CustomEvent;
import io.deephaven.web.client.api.tree.JsTreeTable;
import elemental2.promise.Promise;
import io.deephaven.web.client.api.tree.JsRollupConfig;
import io.deephaven.web.client.api.tree.enums.JsAggregationOperation;
import jsinterop.base.Js;
import jsinterop.base.JsPropertyMap;

import java.util.ArrayList;
import java.util.List;
import java.util.function.Supplier;

public class HierarchicalTableTestGwt extends AbstractAsyncGwtTestCase {
@Override
Expand All @@ -13,11 +23,15 @@ public String getModuleName() {
}

private final TableSourceBuilder tables = new TableSourceBuilder()
.script("from deephaven import empty_table, time_table")
.script("from deephaven import empty_table, time_table, agg")
.script("static_tree",
"empty_table(1000).update(['ID=i', 'Parent=i == 0 ? null : (int)(i/10)']).tree('ID', 'Parent')")
.script("ticking_tree",
"time_table('PT0.1s').update(['ID=i', 'Parent=i == 0 ? null : (int)(i/10)']).tree('ID', 'Parent')");
"time_table('PT0.1s').update(['ID=i', 'Parent=i == 0 ? null : (int)(i/10)']).format_columns(['ID=ID>0 ? GREEN : RED']).tree('ID', 'Parent')")
.script("table_to_rollup",
"time_table('PT0.1s').update(['Y=Math.sin(i/3)', 'X=i%3']).format_columns(['Y=Y>0 ? GREEN : RED'])")
.script("ticking_rollup",
"table_to_rollup.rollup(aggs=[agg.first('Y')],by=['X'],include_constituents=True)");

public void testStaticTreeTable() {
connect(tables)
Expand Down Expand Up @@ -86,4 +100,50 @@ public void testRefreshingTreeTable() {
})
.then(this::finish).catch_(this::report);
}

public void testCreateRollup() {
connect(tables)
.then(table("table_to_rollup"))
.then(table -> {
List<Supplier<Promise<JsTreeTable>>> tests = new ArrayList<>();
// For each supported operation, apply it to the numeric column
// Then expand to verify data can load
String[] count = new String[] {
JsAggregationOperation.COUNT,
JsAggregationOperation.COUNT_DISTINCT,
// TODO(deephaven-core#6201) re-enable this line when fixed
// JsAggregationOperation.DISTINCT,
JsAggregationOperation.MIN,
JsAggregationOperation.MAX,
JsAggregationOperation.SUM,
JsAggregationOperation.ABS_SUM,
JsAggregationOperation.VAR,
JsAggregationOperation.AVG,
JsAggregationOperation.STD,
JsAggregationOperation.FIRST,
JsAggregationOperation.LAST,
JsAggregationOperation.UNIQUE
};
for (int i = 0; i < count.length; i++) {
final int step = i;
String operation = count[i];
JsRollupConfig cfg = new JsRollupConfig();
cfg.groupingColumns = Js.uncheckedCast(JsArray.of("X"));
cfg.includeConstituents = true;
cfg.aggregations = (JsPropertyMap) JsPropertyMap.of(operation, JsArray.of("Y"));
tests.add(() -> table.rollup(cfg).then(r -> {
r.setViewport(0, 99, null, null);
delayTestFinish(15000 + step);

return waitForEventWhere(r, JsTreeTable.EVENT_UPDATED,
(CustomEvent<JsTreeTable.TreeViewportData> d) -> r.getSize() == 4, 13000 + step)
.then(event -> Promise.resolve(r));
}));
}

return tests.stream().reduce((p1, p2) -> () -> p1.get().then(result -> p2.get())).get().get();
})
.then(this::finish).catch_(this::report);
}

}
Loading