Skip incompatible columns

deephaven · Jun 2, 2023 · 1130bad · 1130bad
1 parent 6eaa287
commit 1130bad
Show file tree

Hide file tree

Showing 3 changed files with 84 additions and 9 deletions.
diff --git a/web/client-api/src/main/java/io/deephaven/web/client/api/JsTable.java b/web/client-api/src/main/java/io/deephaven/web/client/api/JsTable.java
@@ -661,7 +661,7 @@ private Promise<JsTotalsTable> fetchTotals(Object config, JsProvider<ClientTable
                     "(", LazyString.of(target::getHandle), "), into ", LazyString.of(newState::getHandle), "(",
                     newState, ")");
 
-            AggregateRequest requestMessage = directive.buildRequest(getColumns().map((p0, p1, p2) -> p0.getName()));
+            AggregateRequest requestMessage = directive.buildRequest(getColumns());
             JsArray<String> updateViewExprs = directive.getCustomColumns();
             JsArray<String> dropColumns = directive.getDropColumns();
             requestMessage.setSourceId(target.getHandle().makeTableReference());

diff --git a/web/client-api/src/main/java/io/deephaven/web/client/api/JsTotalsTableConfig.java b/web/client-api/src/main/java/io/deephaven/web/client/api/JsTotalsTableConfig.java
@@ -11,7 +11,6 @@
 import io.deephaven.javascript.proto.dhinternal.io.deephaven.proto.table_pb.AggSpec;
 import io.deephaven.javascript.proto.dhinternal.io.deephaven.proto.table_pb.AggregateRequest;
 import io.deephaven.javascript.proto.dhinternal.io.deephaven.proto.table_pb.Aggregation;
-import io.deephaven.javascript.proto.dhinternal.io.deephaven.proto.table_pb.NullValueMap;
 import io.deephaven.javascript.proto.dhinternal.io.deephaven.proto.table_pb.aggregation.AggregationColumns;
 import io.deephaven.javascript.proto.dhinternal.io.deephaven.proto.table_pb.aggregation.AggregationCount;
 import io.deephaven.javascript.proto.dhinternal.io.deephaven.proto.table_pb.aggspec.AggSpecAbsSum;
@@ -43,6 +42,7 @@
 import java.util.Map;
 import java.util.Set;
 import java.util.stream.Collector;
+import java.util.stream.Collectors;
 
 @JsType(name = "TotalsTableConfig", namespace = "dh")
 public class JsTotalsTableConfig {
@@ -196,22 +196,26 @@ public String serialize() {
     }
 
     @JsIgnore
-    public AggregateRequest buildRequest(JsArray<String> allColumns) {
+    public AggregateRequest buildRequest(JsArray<Column> allColumns) {
         AggregateRequest request = new AggregateRequest();
         customColumns = new JsArray<>();
         dropColumns = new JsArray<>();
 
         request.setGroupByColumnsList(Js.<JsArray<String>>uncheckedCast(groupBy));
         JsArray<Aggregation> aggregations = new JsArray<>();
         request.setAggregationsList(aggregations);
+        Map<String, String> columnTypes = Arrays.stream(Js.<Column[]>uncheckedCast(allColumns)).collect(Collectors.toMap(Column::getName, Column::getType));
         Map<String, LinkedHashSet<String>> aggs = new HashMap<>();
         List<String> colsNeedingCompoundNames = new ArrayList<>();
         Set<String> seenColNames = new HashSet<>();
         groupBy.forEach((col, p1, p2) -> seenColNames.add(Js.cast(col)));
-        this.operationMap.forEach(col -> {
-            this.operationMap.get(col).forEach((agg, index, arr) -> {
-                String colName = Js.cast(col);
-                aggs.computeIfAbsent(Js.cast(agg), ignore -> new LinkedHashSet<>()).add(colName);
+        this.operationMap.forEach(colName -> {
+            this.operationMap.get(colName).forEach((agg, index, arr) -> {
+                if (!JsAggregationOperation.canAggregateType(agg, columnTypes.get(colName))) {
+                    // skip this column. to follow DHE's behavior
+                    return null;
+                }
+                aggs.computeIfAbsent(agg, ignore -> new LinkedHashSet<>()).add(colName);
                 if (seenColNames.contains(colName)) {
                     colsNeedingCompoundNames.add(colName);
                 } else {
@@ -220,10 +224,10 @@ public AggregateRequest buildRequest(JsArray<String> allColumns) {
                 return null;
             });
         });
-        Set<String> unusedColumns = new HashSet<>(Arrays.asList(Js.<String[]>uncheckedCast(allColumns)));
+        Set<String> unusedColumns = new HashSet<>(columnTypes.keySet());
         unusedColumns.removeAll(seenColNames);
         // no unused column can collide, add to the default operation list
-        aggs.computeIfAbsent(defaultOperation, ignore -> new LinkedHashSet<>()).addAll(unusedColumns);
+        aggs.computeIfAbsent(defaultOperation, ignore -> new LinkedHashSet<>()).addAll(unusedColumns.stream().filter(colName -> JsAggregationOperation.canAggregateType(defaultOperation, columnTypes.get(colName))).collect(Collectors.toList()));
 
         aggs.forEach((aggregationType, cols) -> {
             Aggregation agg = new Aggregation();

diff --git a/...ient-api/src/main/java/io/deephaven/web/client/api/tree/enums/JsAggregationOperation.java b/...ient-api/src/main/java/io/deephaven/web/client/api/tree/enums/JsAggregationOperation.java
@@ -4,6 +4,7 @@
 package io.deephaven.web.client.api.tree.enums;
 
 import com.vertispan.tsdefs.annotations.TsTypeDef;
+import jsinterop.annotations.JsIgnore;
 import jsinterop.annotations.JsType;
 
 @JsType(name = "AggregationOperation", namespace = "dh")
@@ -31,4 +32,74 @@ public class JsAggregationOperation {
     // WSUM = "WeightedSum";
     @Deprecated
     public static final String SKIP = "Skip";
+
+    @JsIgnore
+    public static boolean canAggregateType(String aggregationType, String columnType) {
+        switch (aggregationType) {
+            case COUNT:
+            case COUNT_DISTINCT:
+            case FIRST:
+            case LAST:
+            case UNIQUE: {
+                // These operations are always safe
+                return true;
+            }
+            case ABS_SUM:
+            case SUM: {
+                // Both sum operators will count "true" boolean values
+                return isNumericOrBoolean(columnType);
+            }
+            case AVG:
+            case VAR:
+            case STD: {
+                return isNumeric(columnType);
+            }
+            case MIN:
+            case MAX: {
+                // Can only apply to Comparables - JS can't work this out, so we'll stick to known types
+                return isComparable(columnType);
+            }
+        }
+        return false;
+    }
+
+    private static boolean isNumeric(String columnType) {
+        switch (columnType) {
+            case "double":
+            case "float":
+            case "int":
+            case "long":
+            case "short":
+            case "char":
+            case "byte": {
+                return true;
+            }
+        }
+        return false;
+    }
+
+    private static boolean isNumericOrBoolean(String columnType) {
+        if (isNumeric(columnType)) {
+            return true;
+        }
+        return columnType.equals("boolean") || columnType.equals("java.lang.Boolean");
+    }
+    private static boolean isComparable(String columnType) {
+        if (isNumericOrBoolean(columnType)) {
+            return true;
+        }
+        switch (columnType) {
+            case "java.lang.String":
+            case "java.time.Instant":
+            case "java.time.ZonedDateTime":
+            case "io.deephaven.time.DateTime":
+            case "java.time.LocalTime":
+            case "java.time.LocalDate":
+            case "java.math.BigDecimal":
+            case "java.math.BigInteger":
+                return true;
+        }
+        return false;
+    }
+
 }