Skip to content

Commit c75d8b0

Browse files
texodustexodus
authored
Merge pull request #2328 from finos/fix-expression-update-overcalc
Fix `update()` with `expressions` overcalc
2 parents f4a9774 + eca996a commit c75d8b0

12 files changed

Lines changed: 113 additions & 62 deletions

File tree

‎cpp/perspective/src/cpp/context_grouped_pkey.cpp‎

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -684,7 +684,6 @@ t_ctx_grouped_pkey::get_column_dtype(t_uindex idx) const {
684684

685685
void
686686
t_ctx_grouped_pkey::compute_expressions(std::shared_ptr<t_data_table> master,
687-
std::shared_ptr<t_data_table> flattened,
688687
t_expression_vocab& expression_vocab, t_regex_mapping& regex_mapping) {
689688
// Clear the transitional expression tables on the context so they are
690689
// ready for the next update.
@@ -693,10 +692,6 @@ t_ctx_grouped_pkey::compute_expressions(std::shared_ptr<t_data_table> master,
693692
std::shared_ptr<t_data_table> master_expression_table
694693
= m_expression_tables->m_master;
695694

696-
t_uindex flattened_num_rows = flattened->size();
697-
m_expression_tables->reserve_transitional_table_size(flattened_num_rows);
698-
m_expression_tables->set_transitional_table_size(flattened_num_rows);
699-
700695
// Set the master table to the right size.
701696
t_uindex num_rows = master->size();
702697
master_expression_table->reserve(num_rows);
@@ -707,9 +702,6 @@ t_ctx_grouped_pkey::compute_expressions(std::shared_ptr<t_data_table> master,
707702
// Compute the expressions on the master table.
708703
expr->compute(
709704
master, master_expression_table, expression_vocab, regex_mapping);
710-
711-
expr->compute(flattened, m_expression_tables->m_flattened,
712-
expression_vocab, regex_mapping);
713705
}
714706
}
715707

‎cpp/perspective/src/cpp/context_one.cpp‎

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -612,7 +612,6 @@ t_ctx1::get_trav_depth(t_index idx) const {
612612

613613
void
614614
t_ctx1::compute_expressions(std::shared_ptr<t_data_table> master,
615-
std::shared_ptr<t_data_table> flattened,
616615
t_expression_vocab& expression_vocab, t_regex_mapping& regex_mapping) {
617616
// Clear the transitional expression tables on the context so they are
618617
// ready for the next update.
@@ -621,10 +620,6 @@ t_ctx1::compute_expressions(std::shared_ptr<t_data_table> master,
621620
std::shared_ptr<t_data_table> master_expression_table
622621
= m_expression_tables->m_master;
623622

624-
t_uindex flattened_num_rows = flattened->size();
625-
m_expression_tables->reserve_transitional_table_size(flattened_num_rows);
626-
m_expression_tables->set_transitional_table_size(flattened_num_rows);
627-
628623
// Set the master table to the right size.
629624
t_uindex num_rows = master->size();
630625
master_expression_table->reserve(num_rows);
@@ -635,9 +630,6 @@ t_ctx1::compute_expressions(std::shared_ptr<t_data_table> master,
635630
// Compute the expressions on the master table.
636631
expr->compute(
637632
master, master_expression_table, expression_vocab, regex_mapping);
638-
639-
expr->compute(flattened, m_expression_tables->m_flattened,
640-
expression_vocab, regex_mapping);
641633
}
642634
}
643635

‎cpp/perspective/src/cpp/context_two.cpp‎

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1054,7 +1054,6 @@ t_ctx2::get_column_dtype(t_uindex idx) const {
10541054

10551055
void
10561056
t_ctx2::compute_expressions(std::shared_ptr<t_data_table> master,
1057-
std::shared_ptr<t_data_table> flattened,
10581057
t_expression_vocab& expression_vocab, t_regex_mapping& regex_mapping) {
10591058
// Clear the transitional expression tables on the context so they are
10601059
// ready for the next update.
@@ -1063,10 +1062,6 @@ t_ctx2::compute_expressions(std::shared_ptr<t_data_table> master,
10631062
std::shared_ptr<t_data_table> master_expression_table
10641063
= m_expression_tables->m_master;
10651064

1066-
t_uindex flattened_num_rows = flattened->size();
1067-
m_expression_tables->reserve_transitional_table_size(flattened_num_rows);
1068-
m_expression_tables->set_transitional_table_size(flattened_num_rows);
1069-
10701065
// Set the master table to the right size.
10711066
t_uindex num_rows = master->size();
10721067
master_expression_table->reserve(num_rows);
@@ -1077,9 +1072,6 @@ t_ctx2::compute_expressions(std::shared_ptr<t_data_table> master,
10771072
// Compute the expressions on the master table.
10781073
expr->compute(
10791074
master, master_expression_table, expression_vocab, regex_mapping);
1080-
1081-
expr->compute(flattened, m_expression_tables->m_flattened,
1082-
expression_vocab, regex_mapping);
10831075
}
10841076
}
10851077

‎cpp/perspective/src/cpp/context_zero.cpp‎

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -620,7 +620,6 @@ t_ctx0::get_step_delta(t_index bidx, t_index eidx) {
620620

621621
void
622622
t_ctx0::compute_expressions(std::shared_ptr<t_data_table> master,
623-
std::shared_ptr<t_data_table> flattened,
624623
t_expression_vocab& expression_vocab, t_regex_mapping& regex_mapping) {
625624
// Clear the transitional expression tables on the context so they are
626625
// ready for the next update.
@@ -629,10 +628,6 @@ t_ctx0::compute_expressions(std::shared_ptr<t_data_table> master,
629628
std::shared_ptr<t_data_table> master_expression_table
630629
= m_expression_tables->m_master;
631630

632-
t_uindex flattened_num_rows = flattened->size();
633-
m_expression_tables->reserve_transitional_table_size(flattened_num_rows);
634-
m_expression_tables->set_transitional_table_size(flattened_num_rows);
635-
636631
// Set the master table to the right size.
637632
t_uindex num_rows = master->size();
638633
master_expression_table->reserve(num_rows);
@@ -643,9 +638,6 @@ t_ctx0::compute_expressions(std::shared_ptr<t_data_table> master,
643638
// Compute the expressions on the master table.
644639
expr->compute(
645640
master, master_expression_table, expression_vocab, regex_mapping);
646-
647-
expr->compute(flattened, m_expression_tables->m_flattened,
648-
expression_vocab, regex_mapping);
649641
}
650642
}
651643

‎cpp/perspective/src/cpp/expression_tables.cpp‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,19 @@ t_expression_tables::get_table() const {
5151
return m_master.get();
5252
}
5353

54+
void
55+
t_expression_tables::set_flattened(std::shared_ptr<t_data_table> flattened) {
56+
t_uindex flattened_num_rows = flattened->size();
57+
reserve_transitional_table_size(flattened_num_rows);
58+
set_transitional_table_size(flattened_num_rows);
59+
const t_schema& schema = m_flattened->get_schema();
60+
const std::vector<std::string>& column_names = schema.m_columns;
61+
for (const auto& colname : column_names) {
62+
m_flattened->set_column(
63+
colname, flattened->get_column(colname)->clone());
64+
}
65+
}
66+
5467
void
5568
t_expression_tables::calculate_transitions(
5669
std::shared_ptr<t_data_table> existed) {

‎cpp/perspective/src/cpp/gnode.cpp‎

Lines changed: 43 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -871,8 +871,13 @@ t_gnode::_register_context(
871871
ctx->reset();
872872

873873
if (should_update) {
874-
ctx->compute_expressions(m_gstate->get_table(), pkeyed_table,
874+
ctx->compute_expressions(m_gstate->get_table(),
875875
expression_vocab, expression_regex_mapping);
876+
ctx->get_expression_tables()->set_flattened(
877+
m_gstate->get_pkeyed_table(
878+
ctx->get_expression_tables()->m_master->get_schema(),
879+
ctx->get_expression_tables()->m_master));
880+
876881
update_context_from_state<t_ctx2>(ctx, name, pkeyed_table);
877882
}
878883
} break;
@@ -881,8 +886,12 @@ t_gnode::_register_context(
881886
t_ctx1* ctx = static_cast<t_ctx1*>(ptr_);
882887
ctx->reset();
883888
if (should_update) {
884-
ctx->compute_expressions(m_gstate->get_table(), pkeyed_table,
889+
ctx->compute_expressions(m_gstate->get_table(),
885890
expression_vocab, expression_regex_mapping);
891+
ctx->get_expression_tables()->set_flattened(
892+
m_gstate->get_pkeyed_table(
893+
ctx->get_expression_tables()->m_master->get_schema(),
894+
ctx->get_expression_tables()->m_master));
886895

887896
update_context_from_state<t_ctx1>(ctx, name, pkeyed_table);
888897
}
@@ -892,8 +901,13 @@ t_gnode::_register_context(
892901
t_ctx0* ctx = static_cast<t_ctx0*>(ptr_);
893902
ctx->reset();
894903
if (should_update) {
895-
ctx->compute_expressions(m_gstate->get_table(), pkeyed_table,
904+
ctx->compute_expressions(m_gstate->get_table(),
896905
expression_vocab, expression_regex_mapping);
906+
ctx->get_expression_tables()->set_flattened(
907+
m_gstate->get_pkeyed_table(
908+
ctx->get_expression_tables()->m_master->get_schema(),
909+
ctx->get_expression_tables()->m_master));
910+
897911
update_context_from_state<t_ctx0>(ctx, name, pkeyed_table);
898912
}
899913
} break;
@@ -913,8 +927,13 @@ t_gnode::_register_context(
913927
ctx->reset();
914928

915929
if (should_update) {
916-
ctx->compute_expressions(m_gstate->get_table(), pkeyed_table,
930+
ctx->compute_expressions(m_gstate->get_table(),
917931
expression_vocab, expression_regex_mapping);
932+
ctx->get_expression_tables()->set_flattened(
933+
m_gstate->get_pkeyed_table(
934+
ctx->get_expression_tables()->m_master->get_schema(),
935+
ctx->get_expression_tables()->m_master));
936+
918937
update_context_from_state<t_ctx_grouped_pkey>(
919938
ctx, name, pkeyed_table);
920939
}
@@ -999,27 +1018,39 @@ t_gnode::_compute_expressions(std::shared_ptr<t_data_table> flattened_masked) {
9991018
case TWO_SIDED_CONTEXT: {
10001019
t_ctx2* ctx = static_cast<t_ctx2*>(ctxh.m_ctx);
10011020
ctx->compute_expressions(m_gstate->get_table(),
1002-
flattened_masked, expression_vocab,
1003-
expression_regex_mapping);
1021+
expression_vocab, expression_regex_mapping);
1022+
ctx->get_expression_tables()->set_flattened(
1023+
m_gstate->get_pkeyed_table(
1024+
ctx->get_expression_tables()->m_master->get_schema(),
1025+
ctx->get_expression_tables()->m_master));
10041026
} break;
10051027
case ONE_SIDED_CONTEXT: {
10061028
t_ctx1* ctx = static_cast<t_ctx1*>(ctxh.m_ctx);
10071029
ctx->compute_expressions(m_gstate->get_table(),
1008-
flattened_masked, expression_vocab,
1009-
expression_regex_mapping);
1030+
expression_vocab, expression_regex_mapping);
1031+
ctx->get_expression_tables()->set_flattened(
1032+
m_gstate->get_pkeyed_table(
1033+
ctx->get_expression_tables()->m_master->get_schema(),
1034+
ctx->get_expression_tables()->m_master));
10101035
} break;
10111036
case ZERO_SIDED_CONTEXT: {
10121037
t_ctx0* ctx = static_cast<t_ctx0*>(ctxh.m_ctx);
10131038
ctx->compute_expressions(m_gstate->get_table(),
1014-
flattened_masked, expression_vocab,
1015-
expression_regex_mapping);
1039+
expression_vocab, expression_regex_mapping);
1040+
ctx->get_expression_tables()->set_flattened(
1041+
m_gstate->get_pkeyed_table(
1042+
ctx->get_expression_tables()->m_master->get_schema(),
1043+
ctx->get_expression_tables()->m_master));
10161044
} break;
10171045
case GROUPED_PKEY_CONTEXT: {
10181046
t_ctx_grouped_pkey* ctx
10191047
= static_cast<t_ctx_grouped_pkey*>(ctxh.m_ctx);
10201048
ctx->compute_expressions(m_gstate->get_table(),
1021-
flattened_masked, expression_vocab,
1022-
expression_regex_mapping);
1049+
expression_vocab, expression_regex_mapping);
1050+
ctx->get_expression_tables()->set_flattened(
1051+
m_gstate->get_pkeyed_table(
1052+
ctx->get_expression_tables()->m_master->get_schema(),
1053+
ctx->get_expression_tables()->m_master));
10231054
} break;
10241055
case UNIT_CONTEXT:
10251056
break;

‎cpp/perspective/src/cpp/gnode_state.cpp‎

Lines changed: 12 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -547,33 +547,36 @@ t_gstate::get_pkey_dtype() const {
547547

548548
std::shared_ptr<t_data_table>
549549
t_gstate::get_pkeyed_table() const {
550+
return get_pkeyed_table(m_input_schema, m_table);
551+
}
552+
553+
std::shared_ptr<t_data_table>
554+
t_gstate::get_pkeyed_table(
555+
const t_schema& schema, const std::shared_ptr<t_data_table> table) const {
550556
// If there are no removes, just return the gstate table. Removes would
551557
// cause m_mapping to be smaller than m_table.
552-
if (m_mapping.size() == m_table->size())
553-
return m_table;
558+
if (m_mapping.size() == table->size())
559+
return table;
554560

555561
// Otherwise mask out the removed rows and return the table.
556562
auto mask = get_cpp_mask();
557563

558564
// count = total number of rows - number of removed rows
559565
t_uindex table_size = mask.count();
560566

561-
const auto& schema_columns = m_input_schema.m_columns;
567+
const auto& schema_columns = schema.m_columns;
562568
t_uindex num_columns = schema_columns.size();
563569

564-
// Clone from the gstate master table
565-
const std::shared_ptr<t_data_table>& master_table = m_table;
566-
567570
std::shared_ptr<t_data_table> rval
568-
= std::make_shared<t_data_table>(m_input_schema, table_size);
571+
= std::make_shared<t_data_table>(schema, table_size);
569572
rval->init();
570573
rval->set_size(table_size);
571574

572575
parallel_for(int(num_columns),
573-
[&schema_columns, rval, master_table, &mask](int colidx) {
576+
[&schema_columns, rval, table, &mask](int colidx) {
574577
const std::string& colname = schema_columns[colidx];
575578
rval->set_column(
576-
colname, master_table->get_const_column(colname)->clone(mask));
579+
colname, table->get_const_column(colname)->clone(mask));
577580
}
578581

579582
);

‎cpp/perspective/src/cpp/view.cpp‎

Lines changed: 16 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1580,10 +1580,10 @@ View<t_ctxunit>::to_columns(t_uindex start_row, t_uindex end_row,
15801580
bool get_pkeys, bool get_ids, bool _leaves_only, t_uindex num_sides,
15811581
bool _has_row_path, std::string nidx, t_uindex columns_length,
15821582
t_uindex group_by_length) const {
1583-
1583+
PSP_GIL_UNLOCK();
1584+
PSP_READ_LOCK(get_lock());
15841585
auto slice = get_data(start_row, end_row, start_col, end_col);
1585-
auto col_names = slice->get_column_names();
1586-
auto schema = m_ctx->get_schema();
1586+
auto& col_names = slice->get_column_names();
15871587

15881588
rapidjson::StringBuffer s;
15891589
rapidjson::Writer<rapidjson::StringBuffer> writer(s);
@@ -1628,9 +1628,12 @@ View<t_ctx0>::to_columns(t_uindex start_row, t_uindex end_row,
16281628
bool get_pkeys, bool get_ids, bool _leaves_only, t_uindex num_sides,
16291629
bool _has_row_path, std::string nidx, t_uindex columns_length,
16301630
t_uindex group_by_length) const {
1631+
PSP_GIL_UNLOCK();
1632+
PSP_READ_LOCK(get_lock());
16311633
auto slice = get_data(start_row, end_row, start_col, end_col);
1632-
auto col_names = slice->get_column_names();
1633-
auto schema = m_ctx->get_schema();
1634+
const std::vector<std::vector<t_tscalar>>& col_names
1635+
= slice->get_column_names();
1636+
16341637
rapidjson::StringBuffer s;
16351638
rapidjson::Writer<rapidjson::StringBuffer> writer(s);
16361639
writer.StartObject();
@@ -1671,8 +1674,11 @@ View<t_ctx1>::to_columns(t_uindex start_row, t_uindex end_row,
16711674
bool get_pkeys, bool get_ids, bool leaves_only, t_uindex num_sides,
16721675
bool has_row_path, std::string nidx, t_uindex columns_length,
16731676
t_uindex group_by_length) const {
1677+
PSP_GIL_UNLOCK();
1678+
PSP_READ_LOCK(get_lock());
1679+
16741680
auto slice = get_data(start_row, end_row, start_col, end_col);
1675-
auto col_names = slice->get_column_names();
1681+
const auto& col_names = slice->get_column_names();
16761682
rapidjson::StringBuffer s;
16771683
rapidjson::Writer<rapidjson::StringBuffer> writer(s);
16781684
writer.StartObject();
@@ -1721,8 +1727,10 @@ View<t_ctx2>::to_columns(t_uindex start_row, t_uindex end_row,
17211727
bool get_pkeys, bool get_ids, bool leaves_only, t_uindex num_sides,
17221728
bool has_row_path, std::string nidx, t_uindex columns_length,
17231729
t_uindex group_by_length) const {
1724-
auto slice = get_data(start_row, end_row, start_col, end_col);
1725-
auto col_names = slice->get_column_names();
1730+
PSP_GIL_UNLOCK();
1731+
PSP_READ_LOCK(get_lock());
1732+
const auto slice = get_data(start_row, end_row, start_col, end_col);
1733+
const auto& col_names = slice->get_column_names();
17261734
rapidjson::StringBuffer s;
17271735
rapidjson::Writer<rapidjson::StringBuffer> writer(s);
17281736
writer.StartObject();

‎cpp/perspective/src/include/perspective/context_common_decls.h‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -97,7 +97,6 @@ std::shared_ptr<t_expression_tables> get_expression_tables() const;
9797
// Given shared pointers to data tables from the gnode, use them to
9898
// compute the results of expression columns.
9999
void compute_expressions(std::shared_ptr<t_data_table> master,
100-
std::shared_ptr<t_data_table> flattened_masked,
101100
t_expression_vocab& expression_vocab, t_regex_mapping& regex_mapping);
102101

103102
void compute_expressions(std::shared_ptr<t_data_table> master,

‎cpp/perspective/src/include/perspective/expression_tables.h‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,8 @@ struct t_expression_tables {
5050
// Calculate the `t_transitions` value for each row.
5151
void calculate_transitions(std::shared_ptr<t_data_table> existed);
5252

53+
void set_flattened(std::shared_ptr<t_data_table> flattened);
54+
5355
void reset();
5456

5557
t_data_table* get_table() const;

0 commit comments

Comments
 (0)