Fix last aggregate to preserve status

finos · Apr 23, 2021 · cd39eb9 · cd39eb9
1 parent e09e6b9
commit cd39eb9
Show file tree

Hide file tree

Showing 6 changed files with 116 additions and 80 deletions.
diff --git a/cpp/perspective/src/cpp/aggregate.cpp b/cpp/perspective/src/cpp/aggregate.cpp
@@ -211,59 +211,6 @@ t_aggregate::init() {
                 default: { PSP_COMPLAIN_AND_ABORT("Unexpected dtype"); }
             }
         } break;
-        case AGGTYPE_LAST_VALUE: {
-            switch (m_icolumns[0]->get_dtype()) {
-                case DTYPE_TIME:
-                case DTYPE_INT64: {
-                    build_aggregate<
-                        t_aggimpl_last_value<std::int64_t, std::int64_t, std::int64_t>>();
-                } break;
-                case DTYPE_INT32: {
-                    build_aggregate<
-                        t_aggimpl_last_value<std::int32_t, std::int32_t, std::int32_t>>();
-                } break;
-                case DTYPE_INT16: {
-                    build_aggregate<
-                        t_aggimpl_last_value<std::int16_t, std::int16_t, std::int16_t>>();
-                } break;
-                case DTYPE_INT8: {
-                    build_aggregate<
-                        t_aggimpl_last_value<std::int8_t, std::int8_t, std::int8_t>>();
-                } break;
-                case DTYPE_UINT64: {
-                    build_aggregate<
-                        t_aggimpl_last_value<std::uint64_t, std::uint64_t, std::uint64_t>>();
-                } break;
-                case DTYPE_DATE:
-                case DTYPE_UINT32: {
-                    build_aggregate<
-                        t_aggimpl_last_value<std::uint32_t, std::uint32_t, std::uint32_t>>();
-                } break;
-                case DTYPE_UINT16: {
-                    build_aggregate<
-                        t_aggimpl_last_value<std::uint16_t, std::uint16_t, std::uint16_t>>();
-                } break;
-                case DTYPE_UINT8: {
-                    build_aggregate<
-                        t_aggimpl_last_value<std::uint8_t, std::uint8_t, std::uint8_t>>();
-                } break;
-                case DTYPE_FLOAT64: {
-                    build_aggregate<t_aggimpl_last_value<double, double, double>>();
-                } break;
-                case DTYPE_FLOAT32: {
-                    build_aggregate<t_aggimpl_last_value<float, float, float>>();
-                } break;
-                case DTYPE_BOOL: {
-                    build_aggregate<
-                        t_aggimpl_last_value<std::uint8_t, std::uint8_t, std::uint8_t>>();
-                } break;
-                case DTYPE_STR: {
-                    build_aggregate<
-                        t_aggimpl_last_value<const char*, const char*, const char*>>();
-                } break;
-                default: { PSP_COMPLAIN_AND_ABORT("Unexpected dtype"); }
-            }
-        } break;
         case AGGTYPE_HIGH_WATER_MARK: {
             switch (m_icolumns[0]->get_dtype()) {
                 case DTYPE_TIME:

diff --git a/cpp/perspective/src/cpp/gnode_state.cpp b/cpp/perspective/src/cpp/gnode_state.cpp
@@ -371,6 +371,18 @@ t_gstate::get_table() const {
     return m_table;
 }
 
+t_tscalar
+t_gstate::read_by_pkey(const std::string& colname, t_tscalar& pkey) const {
+    std::shared_ptr<const t_column> col = m_table->get_const_column(colname);
+    const t_column* col_ = col.get();
+    t_mapping::const_iterator iter = m_mapping.find(pkey);
+    if (iter != m_mapping.end()) {
+        return col_->get_scalar(iter->second);
+    } else {
+        PSP_COMPLAIN_AND_ABORT("Called without pkey");
+    }
+}
+
 void
 t_gstate::read_column(const std::string& colname, const std::vector<t_tscalar>& pkeys,
     std::vector<t_tscalar>& out_data) const {

diff --git a/cpp/perspective/src/cpp/sparse_tree.cpp b/cpp/perspective/src/cpp/sparse_tree.cpp
@@ -1108,13 +1108,29 @@ t_stree::update_agg_table(t_uindex nidx, t_agg_update_info& info, t_uindex src_r
                 dst->set_scalar(dst_ridx, new_value);
             } break;
             case AGGTYPE_LAST_VALUE: {
-                t_tscalar src_scalar = src->get_scalar(src_ridx);
                 t_tscalar dst_scalar = dst->get_scalar(dst_ridx);
+                old_value.set(dst_scalar);                
+                t_uindex leaf;
+                if (is_leaf(nidx)) {
+                    leaf = nidx;
+                } else {
+                    auto iters = m_idxleaf->get<by_idx_lfidx>().equal_range(nidx);
+                    if (iters.first != iters.second) {
+                        leaf = (--iters.second)->m_lfidx;
+                    } else {
+                        dst->set_scalar(dst_ridx, mknone());
+                        break;
+                    }
+                }
 
-                old_value.set(dst_scalar);
-                new_value.set(src_scalar);
-
-                dst->set_scalar(dst_ridx, new_value);
+                auto iters = m_idxpkey->get<by_idx_pkey>().equal_range(leaf);
+                if (iters.first != iters.second) {
+                    t_tscalar pkey = (--iters.second)->m_pkey;
+                    std::vector<t_tscalar> values;
+                    dst->set_scalar(dst_ridx, gstate.read_by_pkey(spec.get_dependencies()[0].name(), pkey));
+                } else {
+                    dst->set_scalar(dst_ridx, mknone());
+                }
             } break;
             case AGGTYPE_HIGH_WATER_MARK: {
                 t_tscalar src_scalar = src->get_scalar(src_ridx);

diff --git a/cpp/perspective/src/include/perspective/aggregate.h b/cpp/perspective/src/include/perspective/aggregate.h
@@ -126,28 +126,6 @@ class PERSPECTIVE_EXPORT t_aggimpl_mean : public t_aggimpl<RAW_DATA_T, ROLLING_T
     }
 };
 
-template <typename RAW_DATA_T, typename ROLLING_T, typename RESULT_T>
-class PERSPECTIVE_EXPORT t_aggimpl_last_value
-    : public t_aggimpl<RAW_DATA_T, ROLLING_T, RESULT_T> {
-public:
-    RESULT_T
-    value(ROLLING_T rs) { return RESULT_T(rs); }
-
-    ROLLING_T
-    reduce(const RAW_DATA_T* biter, const RAW_DATA_T* eiter) {
-        if (biter >= eiter)
-            return ROLLING_T();
-        return ROLLING_T(*(eiter - 1));
-    }
-
-    ROLLING_T
-    roll_up(const ROLLING_T* biter, const ROLLING_T* eiter) {
-        if (biter >= eiter)
-            return ROLLING_T();
-
-        return ROLLING_T(*(eiter - 1));
-    }
-};
 
 template <typename RAW_DATA_T, typename ROLLING_T, typename RESULT_T>
 class PERSPECTIVE_EXPORT t_aggimpl_hwm : public t_aggimpl<RAW_DATA_T, ROLLING_T, RESULT_T> {

diff --git a/cpp/perspective/src/include/perspective/gnode_state.h b/cpp/perspective/src/include/perspective/gnode_state.h
@@ -92,6 +92,9 @@ class PERSPECTIVE_EXPORT t_gstate {
         const std::vector<t_uindex>& master_table_indexes,
         t_uindex num_rows);
 
+    t_tscalar read_by_pkey(
+        const std::string& colname, t_tscalar& pkey) const;
+
     /**
      * @brief Read the values with the specified `pkeys` from the column at 
      * `colname`, writing into `out_data`.

diff --git a/packages/perspective/test/js/pivot_nulls.js b/packages/perspective/test/js/pivot_nulls.js
@@ -9,6 +9,86 @@
 
 module.exports = perspective => {
     describe("Pivotting with nulls", function() {
+        describe("last aggregate", function() {
+            it("preserves null when it is the last element in a leaf", async function() {
+                const DATA = {a: ["a", "a", "a", "b", "b", "b", "c", "c", "c"], b: [1, 2, null, 3, 4, 5, null, null, null]};
+                var table = await perspective.table(DATA);
+                var view = await table.view({
+                    row_pivots: ["a"],
+                    columns: ["b"],
+                    aggregates: {b: "last"}
+                });
+                var answer = [
+                    {__ROW_PATH__: [], b: null},
+                    {__ROW_PATH__: ["a"], b: null},
+                    {__ROW_PATH__: ["b"], b: 5},
+                    {__ROW_PATH__: ["c"], b: null}
+                ];
+                let result = await view.to_json();
+                expect(result).toEqual(answer);
+                view.delete();
+                table.delete();
+            });
+
+            it("preserves null when it is the last element in a leaf under 2 levels", async function() {
+                const DATA = {
+                    a: ["a", "a", "a", "b", "b", "b", "c", "c", "c", "a", "a", "a", "b", "b", "b", "c", "c", "c"],
+                    b: [1, 2, null, 3, 4, 5, null, null, null, 1, 2, null, null, null, null, 3, 4, 5],
+                    c: ["x", "x", "x", "x", "x", "x", "x", "x", "x", "y", "y", "y", "y", "y", "y", "y", "y", "y"]
+                };
+                var table = await perspective.table(DATA);
+                var view = await table.view({
+                    row_pivots: ["c", "a"],
+                    columns: ["b"],
+                    aggregates: {b: "last"}
+                });
+                var answer = [
+                    {__ROW_PATH__: [], b: 5},
+                    {__ROW_PATH__: ["x"], b: null},
+                    {__ROW_PATH__: ["x", "a"], b: null},
+                    {__ROW_PATH__: ["x", "b"], b: 5},
+                    {__ROW_PATH__: ["x", "c"], b: null},
+                    {__ROW_PATH__: ["y"], b: 5},
+                    {__ROW_PATH__: ["y", "a"], b: null},
+                    {__ROW_PATH__: ["y", "b"], b: null},
+                    {__ROW_PATH__: ["y", "c"], b: 5}
+                ];
+                let result = await view.to_json();
+                expect(result).toEqual(answer);
+                view.delete();
+                table.delete();
+            });
+
+            it("preserves null when it is the last element in a leaf under 2 levels when grand total is null", async function() {
+                const DATA = {
+                    a: ["a", "a", "a", "b", "b", "b", "c", "c", "c", "a", "a", "a", "b", "b", "b", "c", "c", "c"],
+                    b: [1, 2, null, null, null, null, 3, 4, 5, 1, 2, null, 3, 4, 5, null, null, null],
+                    c: ["x", "x", "x", "x", "x", "x", "x", "x", "x", "y", "y", "y", "y", "y", "y", "y", "y", "y"]
+                };
+                var table = await perspective.table(DATA);
+                var view = await table.view({
+                    row_pivots: ["c", "a"],
+                    columns: ["b"],
+                    aggregates: {b: "last"}
+                });
+                var answer = [
+                    {__ROW_PATH__: [], b: null},
+                    {__ROW_PATH__: ["x"], b: 5},
+                    {__ROW_PATH__: ["x", "a"], b: null},
+                    {__ROW_PATH__: ["x", "b"], b: null},
+                    {__ROW_PATH__: ["x", "c"], b: 5},
+                    {__ROW_PATH__: ["y"], b: null},
+                    {__ROW_PATH__: ["y", "a"], b: null},
+                    {__ROW_PATH__: ["y", "b"], b: 5},
+                    {__ROW_PATH__: ["y", "c"], b: null}
+                ];
+                let result = await view.to_json();
+                expect(result).toEqual(answer);
+                view.delete();
+                table.delete();
+            });
+        });
+
         it("shows one pivot for the nulls on initial load", async function() {
             const dataWithNulls = [
                 {name: "Homer", value: 1},