Update behavior to not omit non-default aggregates

Signed-off-by: Andrew Stein <[email protected]>

Author: texodus

rust/perspective-js/test/js/view_config.spec.js

@@ -41,17 +41,17 @@ const data = [
             table.delete();
         });
 
-        test("Default aggregates are omitted", async function () {
+        test("Non-default aggregates are provided", async function () {
             const table = await perspective.table(data);
             const view = await table.view({
                 group_by: ["y"],
                 columns: ["x", "z"],
-                aggregates: { x: "sum", z: "count" },
+                aggregates: { x: "count", z: "sum" },
             });
 
             const config = await view.get_config();
             expect(config).toEqual({
-                aggregates: {},
+                aggregates: { x: "count", z: "sum" },
                 columns: ["x", "z"],
                 expressions: {},
                 filter: [],
@@ -64,18 +64,18 @@ const data = [
             table.delete();
         });
 
-        test("Non-default aggregates are provided", async function () {
+        test("Compound aggregates are provided", async function () {
             const table = await perspective.table(data);
             const view = await table.view({
                 group_by: ["y"],
-                columns: ["x", "z"],
-                aggregates: { x: "count", z: "sum" },
+                columns: ["x"],
+                aggregates: { x: ["weighted mean", ["y"]] },
             });
 
             const config = await view.get_config();
             expect(config).toEqual({
-                aggregates: { x: "count", z: "sum" },
-                columns: ["x", "z"],
+                aggregates: { x: ["weighted mean", ["y"]] },
+                columns: ["x"],
                 expressions: {},
                 filter: [],
                 group_by: ["y"],
@@ -87,17 +87,55 @@ const data = [
             table.delete();
         });
 
-        test("Mixed aggregates are provided and omitted", async function () {
+        test("Expression aggregates are provided", async function () {
+            const table = await perspective.table(data);
+            const view = await table.view({
+                group_by: ["y"],
+                columns: ["x", "z", "new"],
+                expressions: { new: `"x" + 1` },
+                aggregates: { x: ["weighted mean", ["y"]] },
+            });
+
+            const config = await view.get_config();
+            expect(config).toEqual({
+                aggregates: {
+                    new: "sum",
+                    z: "count",
+                    x: ["weighted mean", ["y"]],
+                },
+                columns: ["x", "z", "new"],
+                expressions: { new: `"x" + 1` },
+                filter: [],
+                group_by: ["y"],
+                sort: [],
+                split_by: [],
+            });
+
+            view.delete();
+            table.delete();
+        });
+
+        // The `aggregates` field was omitted entirely in `3.0.0` until `3.9.0`
+        // (due to a bug introduced by an inebriated office drone). Prior to
+        // this, columns were sleectively omitted from the result if they were
+        // _non-default_ (to shorten JSON encoding). For example if a `"flaot"`
+        // column was anything other than `"sum"`.
+        //
+        // In revisiting this broken feature, I've updated the behavior to
+        // reflect a less certain view of what is "default", and output all
+        // columns which need aggregates. These tests preserve the `2.x`
+        // behavior.
+        test.skip("Default aggregates are omitted", async function () {
             const table = await perspective.table(data);
             const view = await table.view({
                 group_by: ["y"],
                 columns: ["x", "z"],
-                aggregates: { x: "sum", z: "mean" },
+                aggregates: { x: "sum", z: "count" },
             });
 
             const config = await view.get_config();
             expect(config).toEqual({
-                aggregates: { z: "mean" },
+                aggregates: {},
                 columns: ["x", "z"],
                 expressions: {},
                 filter: [],
@@ -110,17 +148,17 @@ const data = [
             table.delete();
         });
 
-        test("Compound aggregates are provided", async function () {
+        test.skip("Mixed aggregates are provided and omitted", async function () {
             const table = await perspective.table(data);
             const view = await table.view({
                 group_by: ["y"],
                 columns: ["x", "z"],
-                aggregates: { x: ["weighted mean", ["y"]] },
+                aggregates: { x: "sum", z: "mean" },
             });
 
             const config = await view.get_config();
             expect(config).toEqual({
-                aggregates: { x: ["weighted mean", ["y"]] },
+                aggregates: { z: "mean" },
                 columns: ["x", "z"],
                 expressions: {},
                 filter: [],

rust/perspective-server/cpp/perspective/src/cpp/server.cpp

@@ -2245,17 +2245,9 @@ ProtoServer::_handle_request(std::uint32_t client_id, Request&& req) {
             }
 
             if (!view_config->get_row_pivots().empty()) {
-                const auto& schema =
-                    m_resources.get_table_for_view(req.entity_id())
-                        ->get_schema();
                 for (const auto& aggspec : view_config->get_aggspecs()) {
                     auto* proto_exprs = view_config_proto->mutable_aggregates();
                     const auto agg = aggspec;
-                    const auto table_type = schema.get_dtype(aggspec.name());
-
-                    // TODO(texodus): This behavior is arbitrary, I nominate we
-                    // we kill it but it is anothe rbreaking change.
-
                     if (aggspec.agg() == AGGTYPE_WEIGHTED_MEAN
                         || aggspec.agg() == AGGTYPE_MAX_BY
                         || aggspec.agg() == AGGTYPE_MIN_BY) {
@@ -2264,11 +2256,7 @@ ProtoServer::_handle_request(std::uint32_t client_id, Request&& req) {
                         agglist.add_aggregations(agg.agg_str());
                         agglist.add_aggregations(agg.get_input_depnames()[1]);
                         (*proto_exprs)[aggspec.name()] = agglist;
-                    } else if ((is_numeric_type(table_type)
-                         && aggspec.agg() != AGGTYPE_SUM)
-                        || (!is_numeric_type(table_type)
-                            && aggspec.agg() != AGGTYPE_COUNT)) {
-
+                    } else {
                         proto::ViewConfig_AggList agglist;
                         agglist.add_aggregations(agg.agg_str());
                         (*proto_exprs)[aggspec.name()] = agglist;