From 437d30bd9f17f73c1a25748c01dea4be808e03ad Mon Sep 17 00:00:00 2001 From: Alex Hall Date: Mon, 21 Feb 2022 16:19:11 +0200 Subject: [PATCH] (core) Log number of rows in user tables in data engine Summary: Adds a method Table._num_rows using an empty lookup map column. Adds a method Engine.count_rows which adds them all up. Returns the count after applying user actions to be logged by ActiveDoc. Test Plan: Added a unit test in Python. Tested log message manually. Reviewers: paulfitz Reviewed By: paulfitz Differential Revision: https://phab.getgrist.com/D3275 --- app/common/ActionBundle.ts | 1 + app/server/lib/ActiveDoc.ts | 7 ++++++- sandbox/grist/engine.py | 7 +++++++ sandbox/grist/lookup.py | 7 +++++++ sandbox/grist/main.py | 5 ++++- sandbox/grist/table.py | 11 +++++++++++ sandbox/grist/test_derived.py | 4 +++- sandbox/grist/test_lookups.py | 8 ++++++-- sandbox/grist/test_summary_choicelist.py | 2 ++ sandbox/grist/test_trigger_formulas.py | 9 ++++++--- sandbox/grist/test_useractions.py | 17 +++++++++++++++++ sandbox/grist/testscript.json | 16 +++++++++------- sandbox/grist/useractions.py | 5 +++++ 13 files changed, 84 insertions(+), 15 deletions(-) diff --git a/app/common/ActionBundle.ts b/app/common/ActionBundle.ts index c5ae4ce8..eb35f3a6 100644 --- a/app/common/ActionBundle.ts +++ b/app/common/ActionBundle.ts @@ -61,6 +61,7 @@ export interface SandboxActionBundle { calc: Array>; undo: Array>; // Inverse actions for all 'stored' actions. retValues: any[]; // Contains retValue for each of userActions. + rowCount: number; } // Local action that's been applied. It now has an actionNum, and includes doc actions packaged diff --git a/app/server/lib/ActiveDoc.ts b/app/server/lib/ActiveDoc.ts index 28e1e289..7265652c 100644 --- a/app/server/lib/ActiveDoc.ts +++ b/app/server/lib/ActiveDoc.ts @@ -1216,6 +1216,10 @@ export class ActiveDoc extends EventEmitter { } const user = docSession ? await this._granularAccess.getCachedUser(docSession) : undefined; sandboxActionBundle = await this._rawPyCall('apply_user_actions', normalActions, user?.toJSON()); + log.rawInfo('Sandbox row count', { + ...this.getLogMeta(docSession), + rowCount: sandboxActionBundle.rowCount + }); await this._reportDataEngineMemory(); } else { // Create default SandboxActionBundle to use if the data engine is not called. @@ -1736,7 +1740,8 @@ function createEmptySandboxActionBundle(): SandboxActionBundle { direct: [], calc: [], undo: [], - retValues: [] + retValues: [], + rowCount: 0, }; } diff --git a/sandbox/grist/engine.py b/sandbox/grist/engine.py index 2eda24e3..69fa89d2 100644 --- a/sandbox/grist/engine.py +++ b/sandbox/grist/engine.py @@ -1175,6 +1175,13 @@ class Engine(object): """ self._unused_lookups.add(lookup_map_column) + def count_rows(self): + return sum( + table._num_rows() + for table_id, table in six.iteritems(self.tables) + if useractions.is_user_table(table_id) + ) + def apply_user_actions(self, user_actions, user=None): """ Applies the list of user_actions. Returns an ActionGroup. diff --git a/sandbox/grist/lookup.py b/sandbox/grist/lookup.py index a49b41f1..a2781d0e 100644 --- a/sandbox/grist/lookup.py +++ b/sandbox/grist/lookup.py @@ -111,6 +111,13 @@ class BaseLookupMapColumn(column.BaseColumn): if not self._lookup_relations: self._engine.mark_lookupmap_for_cleanup(self) + def _do_fast_empty_lookup(self): + """ + Simplified version of do_lookup for a lookup column with no key columns + to make Table._num_rows as fast as possible. + """ + return self._row_key_map.lookup_right((), default=()) + def do_lookup(self, key): """ Looks up key in the lookup map and returns a tuple with two elements: the set of matching diff --git a/sandbox/grist/main.py b/sandbox/grist/main.py index b5331413..0f75a514 100644 --- a/sandbox/grist/main.py +++ b/sandbox/grist/main.py @@ -64,7 +64,10 @@ def run(sandbox): @export def apply_user_actions(action_reprs, user=None): action_group = eng.apply_user_actions([useractions.from_repr(u) for u in action_reprs], user) - return eng.acl_split(action_group).to_json_obj() + return dict( + rowCount=eng.count_rows(), + **eng.acl_split(action_group).to_json_obj() + ) @export def fetch_table(table_id, formulas=True, query=None): diff --git a/sandbox/grist/table.py b/sandbox/grist/table.py index 3f00072f..ca5b965e 100644 --- a/sandbox/grist/table.py +++ b/sandbox/grist/table.py @@ -225,6 +225,11 @@ class Table(object): # which are 'flattened' so source records may appear in multiple groups self._summary_simple = None + # For use in _num_rows. The attribute isn't strictly needed, + # but it makes _num_rows slightly faster, and only creating the lookup map when _num_rows + # is called seems to be too late, at least for unit tests. + self._empty_lookup_column = self._get_lookup_map(()) + # Add Record and RecordSet subclasses which fill in this table as the first argument class Record(records.Record): def __init__(inner_self, *args, **kwargs): # pylint: disable=no-self-argument @@ -237,6 +242,12 @@ class Table(object): self.Record = Record self.RecordSet = RecordSet + def _num_rows(self): + """ + Similar to `len(self.lookup_records())` but faster and doesn't create dependencies. + """ + return len(self._empty_lookup_column._do_fast_empty_lookup()) + def _rebuild_model(self, user_table): """ Sets class-wide properties from a new Model class for the table (inner class within the table diff --git a/sandbox/grist/test_derived.py b/sandbox/grist/test_derived.py index b3a49d71..028bc01e 100644 --- a/sandbox/grist/test_derived.py +++ b/sandbox/grist/test_derived.py @@ -112,7 +112,9 @@ class TestDerived(test_engine.EngineTestCase): actions.BulkUpdateRecord("GristSummary_6_Orders", [1,5], {"group": [[1], [10]]}), ], "calls": { - "GristSummary_6_Orders": {'#lookup#year': 1, "group": 2, "amount": 2, "count": 2}, + "GristSummary_6_Orders": { + '#lookup#year': 1, "group": 2, "amount": 2, "count": 2, "#lookup#": 1 + }, "Orders": {"#lookup##summary#GristSummary_6_Orders": 1, "#summary#GristSummary_6_Orders": 1}} }) diff --git a/sandbox/grist/test_lookups.py b/sandbox/grist/test_lookups.py index 335215fe..1ee2304e 100644 --- a/sandbox/grist/test_lookups.py +++ b/sandbox/grist/test_lookups.py @@ -399,7 +399,9 @@ return ",".join(str(r.id) for r in Students.lookupRecords(firstName=fn, lastName self.assertPartialOutActions(out_actions, { "calls": { # No calculations of anything Schools because nothing depends on the incomplete value. - "Students": {"#lookup#firstName:lastName": 1, "schoolIds": 1, "schoolCities": 1} + "Students": { + "#lookup#firstName:lastName": 1, "schoolIds": 1, "schoolCities": 1, "#lookup#": 1 + } }, "retValues": [7], }) @@ -408,7 +410,9 @@ return ",".join(str(r.id) for r in Students.lookupRecords(firstName=fn, lastName out_actions = self.add_record("Students", firstName="Chuck", lastName="Norris") self.assertPartialOutActions(out_actions, { "calls": { - "Students": {"#lookup#firstName:lastName": 1, "schoolIds": 1, "schoolCities": 1}, + "Students": { + "#lookup#firstName:lastName": 1, "schoolIds": 1, "schoolCities": 1, "#lookup#": 1 + }, "Schools": {"bestStudentId": 1} }, "retValues": [8], diff --git a/sandbox/grist/test_summary_choicelist.py b/sandbox/grist/test_summary_choicelist.py index ba113475..202a8d3b 100644 --- a/sandbox/grist/test_summary_choicelist.py +++ b/sandbox/grist/test_summary_choicelist.py @@ -155,6 +155,8 @@ class TestSummaryChoiceList(EngineTestCase): '#summary#GristSummary_6_Source4': column.ReferenceListColumn, "#lookup#_Contains(value='#summary#GristSummary_6_Source4')": lookup.ContainsLookupMapColumn, + + "#lookup#": lookup.SimpleLookupMapColumn, } ) diff --git a/sandbox/grist/test_trigger_formulas.py b/sandbox/grist/test_trigger_formulas.py index 3e47b07d..c196469f 100644 --- a/sandbox/grist/test_trigger_formulas.py +++ b/sandbox/grist/test_trigger_formulas.py @@ -145,7 +145,10 @@ class TestTriggerFormulas(test_engine.EngineTestCase): self.assertEqual(self.call_counts, {}) self.load_sample(sample) - self.assertEqual(self.call_counts, {'Creatures': {'OceanName': 3}}) + self.assertEqual(self.call_counts, { + 'Creatures': {'#lookup#': 3, 'OceanName': 3}, + 'Oceans': {'#lookup#': 4}, + }) def test_recalc_undo(self): @@ -247,7 +250,7 @@ class TestTriggerFormulas(test_engine.EngineTestCase): [2, "Manatee", 2, "Poseidon", "", "Poseidon", "Poseidon", "Atlantic" ], ]) self.assertEqual(out_actions.calls, - {"Creatures": {"BossDef": 1, "BossUpd": 1, "BossAll": 1, "OceanName": 1}}) + {"Creatures": {"BossDef": 1, "BossUpd": 1, "BossAll": 1, "OceanName": 1, "#lookup#": 1}}) def test_recalc_trigger_off(self): @@ -272,7 +275,7 @@ class TestTriggerFormulas(test_engine.EngineTestCase): [2, "Manatee", 2, "Poseidon", "", "", "Poseidon", "Atlantic" ], ]) self.assertEqual(out_actions.calls, - {"Creatures": {"BossDef": 1, "BossAll": 1, "OceanName": 1}}) + {"Creatures": {"BossDef": 1, "BossAll": 1, "OceanName": 1, "#lookup#": 1}}) def test_renames(self): diff --git a/sandbox/grist/test_useractions.py b/sandbox/grist/test_useractions.py index 94e8a5cd..33db7d80 100644 --- a/sandbox/grist/test_useractions.py +++ b/sandbox/grist/test_useractions.py @@ -1172,6 +1172,23 @@ class TestUserActions(test_engine.EngineTestCase): "999", ]}]]}) + def test_num_rows(self): + self.load_sample(testutil.parse_test_sample({ + "SCHEMA": [ + [1, "Address", [ + [21, "city", "Text", False, "", "", ""], + ]], + ], + "DATA": { + } + })) + + table = self.engine.tables["Address"] + for i in range(20): + self.add_record("Address", None) + self.assertEqual(i + 1, table._num_rows()) + self.assertEqual(i + 1, self.engine.count_rows()) + def test_raw_view_section_restrictions(self): # load_sample handles loading basic metadata, but doesn't create any view sections self.load_sample(self.sample) diff --git a/sandbox/grist/testscript.json b/sandbox/grist/testscript.json index 1a6190d8..10d0133a 100644 --- a/sandbox/grist/testscript.json +++ b/sandbox/grist/testscript.json @@ -105,7 +105,7 @@ "retValue": [ 11 ] }, "CHECK_CALL_COUNTS": { - "Address" : {"region" : 1} + "Address" : {"region" : 1, "#lookup#": 1} } }], ["APPLY", { @@ -200,7 +200,7 @@ ] }, "CHECK_CALL_COUNTS": { - "Schools": { "name": 1 }, + "Schools": { "name": 1, "#lookup#": 2 }, "Students" : { "schoolShort" : 7 } } }], @@ -239,7 +239,7 @@ ] }, "CHECK_CALL_COUNTS" : { - "Schools": { "name": 1 }, + "Schools": { "name": 1, "#lookup#": 1 }, "Students" : { "schoolShort" : 7 } } }], @@ -357,7 +357,7 @@ "retValue": [ [11, 12] ] }, "CHECK_CALL_COUNTS" : { - "Address" : { "country": 2, "region" : 2 } + "Address" : { "country": 2, "region" : 2, "#lookup#": 2 } } }], ["CHECK_OUTPUT", { @@ -1690,7 +1690,7 @@ ] }, "CHECK_CALL_COUNTS" : { - "Address" : { "city": 1, "region" : 1 }, + "Address" : { "city": 1, "region" : 1, "#lookup#": 1 }, "Students": { "fullName" : 1, "fullNameLen" : 1 } } }], @@ -2563,7 +2563,8 @@ ] }, "CHECK_CALL_COUNTS" : { - "People" : { "fullName" : 7, "schoolRegion" : 7, "schoolShort" : 7, "fullNameLen" : 7 } + "People" : { "fullName" : 7, "schoolRegion" : 7, "schoolShort" : 7, "fullNameLen" : 7, "#lookup#": 7 }, + "School": {"#lookup#": 6} } }], @@ -2677,7 +2678,8 @@ ] }, "CHECK_CALL_COUNTS" : { - "People" : { "fullName" : 7, "schoolRegion" : 7, "schoolShort" : 7, "fullNameLen" : 7 } + "People" : { "fullName" : 7, "schoolRegion" : 7, "schoolShort" : 7, "fullNameLen" : 7, "#lookup#": 7 }, + "School": {"#lookup#": 6} } }], diff --git a/sandbox/grist/useractions.py b/sandbox/grist/useractions.py index 78166224..06840883 100644 --- a/sandbox/grist/useractions.py +++ b/sandbox/grist/useractions.py @@ -8,6 +8,7 @@ import six from six.moves import xrange import acl +import gencode from acl_formula import parse_acl_formula_json import actions import column @@ -83,6 +84,10 @@ def is_hidden_table(table_id): return table_id.startswith('GristHidden_') +def is_user_table(table_id): + return not (is_hidden_table(table_id) or gencode._is_special_table(table_id)) + + def useraction(method): """ Decorator for a method, which creates an action class with the same name and arguments.