Archives/gristlabs_grist-core
1
0
Fork 0
mirror of https://github.com/gristlabs/grist-core.git synced 2024-10-27 20:44:07 +00:00
Code Issues Projects Releases Wiki Activity

(core) Make lookups depend on all rows

Browse Source 
Summary:
This is a fix for a bug discussed in https://grist.slack.com/archives/C069RUP71/p1645138610722889

I still haven't completely wrapped my head around it or figured out how to make a simple reproducible example, but the problem seems to be that a lookup can happen before the column(s) being looked up (the summary helper column in this case) have been computed fully (I think it got interrupted halfway by an OrderError). `do_lookup` would check via `engine._use_node` that the row IDs it found had all been computed already, but there might still be other rows that hadn't been computed yet and would also have values matching the lookup key, so it missed those.

This diff instead calls `_use_node` with no `row_ids` argument, which should ensure that all rows have already been computed.

At first I was worried about how this would affect performance, which led me down an optimisation rabbit hole, hence a bit of unrelated cleanup here and also https://phab.getgrist.com/D3310 . But it doesn't seem to be a problem, and IIUC it should actually make things better, although this code is pretty confusing.

Test Plan: Tested manually that the doc no longer behaves weirdly

Reviewers: dsagal

Reviewed By: dsagal

Subscribers: dsagal, paulfitz

Differential Revision: https://phab.getgrist.com/D3308
This commit is contained in:
Alex Hall 2022-03-07 19:28:00 +02:00
parent b2715ae9ef
commit d154b9afa7
2 changed files with 5 additions and 15 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

14
sandbox/grist/engine.py
View File

@ -824,19 +824,11 @@ class Engine(object):
exclude.add(row_id)
cleaned.append(row_id)
self._recompute_done_counter += 1
# If no particular rows were requested, and we arrive here,
# that means we made it through the whole column! For long
# columns, it is worth deleting dirty_rows in one step rather
# than discarding one cell at a time.
if require_rows is None:
cleaned = []
dirty_rows = None
finally:
self._current_node = previous_current_node
for row_id in cleaned:
# Usually dirty_rows refers to self.recompute_map[node], so this modifies both
dirty_rows.discard(row_id)
# Usually dirty_rows refers to self.recompute_map[node], so this modifies both
dirty_rows -= cleaned
# However it's possible for them to be different
# (see above where `exempt` is nonempty and allow_evaluation=True)
# so here we check self.recompute_map[node] directly

6
sandbox/grist/lookup.py
View File

@ -134,11 +134,9 @@ class BaseLookupMapColumn(column.BaseColumn):
# The _use_node call both brings LookupMapColumn up-to-date, and creates a dependency on it.
# Relation of None isn't valid, but it happens to be unused when there is no current_frame.
row_ids = self._row_key_map.lookup_right(key, set())
engine._use_node(self.node, rel, row_ids)
if not row_ids:
row_ids = self._row_key_map.lookup_right(key, set())
engine._use_node(self.node, rel)
row_ids = self._row_key_map.lookup_right(key, set())
return row_ids, rel
# Override various column methods, since LookupMapColumn doesn't care to store any values. To