Skip to content

Commit

Permalink
fix: JS API rollups with unique aggs must specify null sentinel (#6202)…
Browse files Browse the repository at this point in the history
… (#6409)
  • Loading branch information
niloc132 authored Nov 22, 2024
1 parent 4b90d5b commit 6411450
Show file tree
Hide file tree
Showing 3 changed files with 74 additions and 4 deletions.
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
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
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);
}

}

0 comments on commit 6411450

Please sign in to comment.