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 #1505, #998, #1225 - results after remove are correct #1528

Merged
merged 6 commits into from
Sep 2, 2021
Merged

Fix #1505, #998, #1225 - results after remove are correct #1528

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
065f495
Tests to reproduce #1505
sc1f Aug 23, 2021
eecc95b
fixes #1505 and #998` by refactoring t_gstate::get_pkeyed_table to no…
sc1f Aug 24, 2021
307b040
call_process in num_rows, add remove tests
sc1f Aug 25, 2021
43857c8
Fix #1225: table.size() correct with remove
sc1f Aug 25, 2021
28216fc
Add new removes test suite to assert correctness
sc1f Sep 2, 2021
2117e4d
eslint
sc1f Sep 2, 2021
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
15 changes: 0 additions & 15 deletions cpp/perspective/src/cpp/gnode.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1253,16 +1253,6 @@ t_gnode::clear_output_ports() {
}
}

t_data_table*
t_gnode::_get_pkeyed_table() const {
return m_gstate->_get_pkeyed_table();
}

std::shared_ptr<t_data_table>
t_gnode::get_pkeyed_table_sptr() const {
return m_gstate->get_pkeyed_table();
}

void
t_gnode::set_pool_cleanup(std::function<void()> cleanup) {
m_pool_cleanup = cleanup;
Expand All @@ -1283,11 +1273,6 @@ t_gnode::clear_updated() {
m_was_updated = false;
}

std::shared_ptr<t_data_table>
t_gnode::get_sorted_pkeyed_table() const {
return m_gstate->get_sorted_pkeyed_table();
}

std::string
t_gnode::repr() const {
std::stringstream ss;
Expand Down
182 changes: 21 additions & 161 deletions cpp/perspective/src/cpp/gnode_state.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,7 +209,8 @@ t_gstate::update_master_table(const t_data_table* flattened) {
m_pkcol->set_scalar(master_table_indexes[idx], pkey);
} break;
case OP_DELETE: {
// Actually erase the specified pkey from the master table here
// Erase the pkey from the master table, but this does not
// change the size as the row isn't removed, just cleared out.
erase(pkey);
} break;
default: { PSP_COMPLAIN_AND_ABORT("Unexpected OP"); } break;
Expand Down Expand Up @@ -579,186 +580,45 @@ t_gstate::get_pkey_dtype() const {
return iter->first.get_dtype();
}

std::shared_ptr<t_data_table>
t_gstate::get_sorted_pkeyed_table() const {
std::map<t_tscalar, t_uindex> ordered(m_mapping.begin(), m_mapping.end());
auto sch = m_input_schema.drop({"psp_op"});
auto rv = std::make_shared<t_data_table>(sch, 0);
rv->init();
rv->reserve(mapping_size());

auto pkey_col = rv->get_column("psp_pkey");
std::vector<std::shared_ptr<t_column>> icolumns;
std::vector<std::shared_ptr<t_column>> ocolumns;

icolumns.reserve(m_output_schema.m_columns.size());
ocolumns.reserve(m_output_schema.m_columns.size());

for (const std::string& cname : m_output_schema.m_columns) {
ocolumns.push_back(rv->get_column(cname));
icolumns.push_back(m_table->get_column(cname));
}

for (auto it = ordered.begin(); it != ordered.end(); ++it) {
auto ridx = it->second;
pkey_col->push_back(it->first);
for (t_uindex cidx = 0, loop_end = m_output_schema.size(); cidx < loop_end; ++cidx) {
auto scalar = icolumns[cidx]->get_scalar(ridx);
ocolumns[cidx]->push_back(scalar);
}
}
rv->set_size(mapping_size());
return rv;
}

std::shared_ptr<t_data_table>
t_gstate::get_pkeyed_table(const t_schema& schema) const {
return std::shared_ptr<t_data_table>(_get_pkeyed_table(schema));
}

std::shared_ptr<t_data_table>
t_gstate::get_pkeyed_table() const {
if (m_mapping.size() == m_table->size())
return m_table;
return std::shared_ptr<t_data_table>(_get_pkeyed_table(m_input_schema));
}
// If there are no removes, just return the gstate table. Removes would
// cause m_mapping to be smaller than m_table.
if (m_mapping.size() == m_table->size()) return m_table;

t_data_table*
t_gstate::_get_pkeyed_table() const {
return _get_pkeyed_table(m_input_schema);
}

t_data_table*
t_gstate::_get_pkeyed_table(const std::vector<t_tscalar>& pkeys) const {
return _get_pkeyed_table(m_input_schema, pkeys);
}

t_data_table*
t_gstate::_get_pkeyed_table(const t_schema& schema, const std::vector<t_tscalar>& pkeys) const {
t_mask mask(num_rows());
// Otherwise mask out the removed rows and return the table.
auto mask = get_cpp_mask();

for (const auto& pkey : pkeys) {
auto lk = lookup(pkey);
if (lk.m_exists) {
mask.set(lk.m_idx, true);
}
}
// count = total number of rows - number of removed rows
t_uindex table_size = mask.count();

return _get_pkeyed_table(schema, mask);
}
const auto& schema_columns = m_input_schema.m_columns;
t_uindex num_columns = schema_columns.size();

t_data_table*
t_gstate::_get_pkeyed_table(const t_schema& schema) const {
auto mask = get_cpp_mask();
return _get_pkeyed_table(schema, mask);
}
// Clone from the gstate master table
const std::shared_ptr<t_data_table>& master_table = m_table;

t_data_table*
t_gstate::_get_pkeyed_table(const t_schema& schema, const t_mask& mask) const {
static bool const enable_pkeyed_table_mask_fix = true;
t_uindex o_ncols = schema.m_columns.size();
auto sz = enable_pkeyed_table_mask_fix ? mask.count() : mask.size();
auto rval = new t_data_table(schema, sz);
std::shared_ptr<t_data_table> rval = std::make_shared<t_data_table>(m_input_schema, table_size);
rval->init();
rval->set_size(sz);

const auto& sch_cols = schema.m_columns;
rval->set_size(table_size);

const t_data_table* tbl = m_table.get();

#ifdef PSP_PARALLEL_FOR
tbb::parallel_for(0, int(o_ncols), 1,
[&sch_cols, rval, tbl, &mask](int colidx)
tbb::parallel_for(0, int(num_columns), 1,
[&schema_columns, rval, master_table, &mask](int colidx)
#else
for (t_uindex colidx = 0; colidx < o_ncols; ++colidx)
for (t_uindex colidx = 0; colidx < num_columns; ++colidx)
#endif
{
const std::string& c = sch_cols[colidx];
if (c != "psp_op" && c != "psp_pkey") {
rval->set_column(c, tbl->get_const_column(c)->clone(mask));
}
const std::string& colname = schema_columns[colidx];
rval->set_column(colname, master_table->get_const_column(colname)->clone(mask));
}

#ifdef PSP_PARALLEL_FOR
);
#endif
auto pkey_col = rval->get_column("psp_pkey").get();
auto op_col = rval->get_column("psp_op").get();

op_col->raw_fill<std::uint8_t>(OP_INSERT);
op_col->valid_raw_fill();
pkey_col->valid_raw_fill();

std::vector<std::pair<t_tscalar, t_uindex>> order(
enable_pkeyed_table_mask_fix ? sz : m_mapping.size());
if (enable_pkeyed_table_mask_fix) {
std::vector<t_uindex> mapping;
mapping.resize(mask.size());
{
t_uindex mapped = 0;
for (t_uindex idx = 0; idx < mask.size(); ++idx) {
mapping[idx] = mapped;
if (mask.get(idx))
++mapped;
}
}

t_uindex oidx = 0;
for (const auto& kv : m_mapping) {
if (mask.get(kv.second)) {
order[oidx] = std::make_pair(kv.first, mapping[kv.second]);
++oidx;
}
}
} else // enable_pkeyed_table_mask_fix
{
t_uindex oidx = 0;
for (const auto& kv : m_mapping) {
order[oidx] = kv;
++oidx;
}
}

std::sort(order.begin(), order.end(),
[](const std::pair<t_tscalar, t_uindex>& a, const std::pair<t_tscalar, t_uindex>& b) {
return a.second < b.second;
});

if (get_pkey_dtype() == DTYPE_STR) {
static const t_tscalar empty = get_interned_tscalar("");
static bool const enable_pkeyed_table_vocab_reserve = true;

t_uindex offset = has_pkey(empty) ? 0 : 1;

size_t total_string_size = 0;

if (enable_pkeyed_table_vocab_reserve) {
total_string_size += offset;
for (t_uindex idx = 0, loop_end = order.size(); idx < loop_end; ++idx) {
total_string_size += strlen(order[idx].first.get_char_ptr()) + 1;
}
}

// if the m_mapping is empty, get_pkey_dtype() may lie about our pkeys being strings
// don't try to reserve in this case
if (!order.size())
total_string_size = 0;

pkey_col->set_vocabulary(order, total_string_size);
auto base = pkey_col->get_nth<t_uindex>(0);

for (t_uindex idx = 0, loop_end = order.size(); idx < loop_end; ++idx) {
base[idx] = idx + offset;
}
} else {
t_uindex ridx = 0;
for (const auto& e : order) {
pkey_col->set_scalar(ridx, e.first);
++ridx;
}
}

return rval;
return rval;
}

t_uindex
Expand Down
4 changes: 3 additions & 1 deletion cpp/perspective/src/cpp/table.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,9 @@ Table::init(t_data_table& data_table, std::uint32_t row_count, const t_op op, co
t_uindex
Table::size() const {
PSP_VERBOSE_ASSERT(m_init, "touching uninited object");
return m_gnode->get_table()->size();
// the gstate master table has all rows including removed ones; the mapping
// contains only the current rows in the table.
return m_gnode->mapping_size();
}

t_schema
Expand Down
4 changes: 0 additions & 4 deletions cpp/perspective/src/include/perspective/gnode.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,10 +176,6 @@ class PERSPECTIVE_EXPORT t_gnode {
void clear_input_ports();
void clear_output_ports();

t_data_table* _get_pkeyed_table() const;
std::shared_ptr<t_data_table> get_pkeyed_table_sptr() const;
std::shared_ptr<t_data_table> get_sorted_pkeyed_table() const;

bool has_pkey(t_tscalar pkey) const;

std::vector<t_tscalar> get_row_data_pkeys(const std::vector<t_tscalar>& pkeys) const;
Expand Down
13 changes: 0 additions & 13 deletions cpp/perspective/src/include/perspective/gnode_state.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,21 +224,8 @@ class PERSPECTIVE_EXPORT t_gstate {

// Getters
std::shared_ptr<t_data_table> get_table() const;

std::shared_ptr<t_data_table> get_pkeyed_table(const t_schema& schema) const;
std::shared_ptr<t_data_table> get_pkeyed_table() const;

// Only for tests
std::shared_ptr<t_data_table> get_sorted_pkeyed_table() const;

t_data_table* _get_pkeyed_table() const;
t_data_table* _get_pkeyed_table(const std::vector<t_tscalar>& pkeys) const;
t_data_table* _get_pkeyed_table(
const t_schema& schema, const std::vector<t_tscalar>& pkeys) const;
t_data_table* _get_pkeyed_table(const t_schema& schema) const;
t_data_table* _get_pkeyed_table(
const t_schema& schema, const t_mask& mask) const;

const t_schema& get_input_schema() const;
const t_schema& get_output_schema() const;

Expand Down
6 changes: 0 additions & 6 deletions packages/perspective-jupyterlab/test/jupyter/widget.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,12 +41,6 @@ utils.with_jupyterlab(process.env.__JUPYTERLAB_PORT__, () => {
expect(num_rows).toEqual(5);
}
);






},
{name: "Simple", root: path.join(__dirname, "..", "..")}
);
Expand Down
2 changes: 1 addition & 1 deletion packages/perspective-viewer-d3fc/test/js/integration/bar.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ const path = require("path");

const utils = require("@finos/perspective-test");
const simple_tests = require("@finos/perspective-viewer/test/js/simple_tests.js");
const render_warning_tests = require("@finos/perspective-viewer/test/js/render_warning_tests.js");
// const render_warning_tests = require("@finos/perspective-viewer/test/js/render_warning_tests.js");

const {withTemplate} = require("./simple-template");

Expand Down
2 changes: 1 addition & 1 deletion packages/perspective-viewer-d3fc/test/js/integration/heatmap.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ const path = require("path");

const utils = require("@finos/perspective-test");
const simple_tests = require("@finos/perspective-viewer/test/js/simple_tests.js");
const render_warning_tests = require("@finos/perspective-viewer/test/js/render_warning_tests.js");
// const render_warning_tests = require("@finos/perspective-viewer/test/js/render_warning_tests.js");

const {withTemplate} = require("./simple-template");
withTemplate("heatmap", "Heatmap");
Expand Down
2 changes: 1 addition & 1 deletion packages/perspective-viewer-d3fc/test/js/integration/yscatter.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ const path = require("path");

const utils = require("@finos/perspective-test");
const simple_tests = require("@finos/perspective-viewer/test/js/simple_tests.js");
const render_warning_tests = require("@finos/perspective-viewer/test/js/render_warning_tests.js");
// const render_warning_tests = require("@finos/perspective-viewer/test/js/render_warning_tests.js");

const {withTemplate} = require("./simple-template");
withTemplate("yscatter", "Y Scatter");
Expand Down
28 changes: 14 additions & 14 deletions packages/perspective-viewer-datagrid/test/js/superstore.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,18 +125,18 @@ utils.with_server({}, () => {
);
});

const click_details = async (page, x = 310, y = 300) => {
const viewer = await page.$("perspective-viewer");
// const click_details = async (page, x = 310, y = 300) => {
// const viewer = await page.$("perspective-viewer");

const click_event = page.evaluate(
element =>
new Promise(resolve => {
element.addEventListener("perspective-click", e => {
resolve(e.detail);
});
}),
viewer
);
await page.mouse.click(x, y);
return await click_event;
};
// const click_event = page.evaluate(
// element =>
// new Promise(resolve => {
// element.addEventListener("perspective-click", e => {
// resolve(e.detail);
// });
// }),
// viewer
// );
// await page.mouse.click(x, y);
// return await click_event;
// };
1 change: 0 additions & 1 deletion packages/perspective-workspace/src/js/workspace/discrete.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
*/

import {DockPanel, SplitPanel} from "@lumino/widgets";
import {toArray} from "@lumino/algorithm";
import {Signal} from "@lumino/signaling";

const DISCRETE_STEP_SIZE = 1;
Expand Down
2 changes: 2 additions & 0 deletions packages/perspective/src/js/perspective.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -698,6 +698,7 @@ export default function(Module) {
* integer/float type, the Promise returns undefined.
*/
view.prototype.col_to_js_typed_array = function(col_name, options = {}) {
_call_process(this.table.get_id());
const format_function = __MODULE__[`col_to_js_typed_array`];
return column_to_format.call(this, col_name, options, format_function);
};
Expand Down Expand Up @@ -751,6 +752,7 @@ export default function(Module) {
* @returns {Promise<number>} The number of aggregated rows.
*/
view.prototype.num_rows = function() {
_call_process(this.table.get_id());
return this._View.num_rows();
};

Expand Down
Loading