From d4626aea82af873c423f43c6ecab572f3e421d1f Mon Sep 17 00:00:00 2001
From: Alex Hall <alex.mojaki@gmail.com>
Date: Fri, 1 Oct 2021 14:13:14 +0200
Subject: [PATCH] (core) Only update changed records with RenameChoices

Summary:
Make _rename_cell_choice return None for unchanged values

The tests actually passs without the implementation changes, because trim_update_action removed the noop updates. So I'm not sure if this is an improvement. It certainly seems that it would be slower in cases where every record is updated, and it's hard to say if it would be better in other cases.

Test Plan:
Checked doc actions in existing test.
Also tested for invalid existing values.

Reviewers: dsagal

Reviewed By: dsagal

Subscribers: jarek

Differential Revision: https://phab.getgrist.com/D3052
---
 sandbox/grist/column.py           | 23 +++++++++++++--------
 sandbox/grist/test_useractions.py | 34 +++++++++++++++++++++++++++++--
 2 files changed, 46 insertions(+), 11 deletions(-)

diff --git a/sandbox/grist/column.py b/sandbox/grist/column.py
index 0c214d43..11eada07 100644
--- a/sandbox/grist/column.py
+++ b/sandbox/grist/column.py
@@ -225,6 +225,15 @@ class BaseColumn(object):
     # pylint: disable=no-self-use, unused-argument
     return values, []
 
+
+class DataColumn(BaseColumn):
+  """
+  DataColumn describes a column of raw data, and holds it.
+  """
+  pass
+
+
+class ChoiceColumn(DataColumn):
   def rename_choices(self, renames):
     row_ids = []
     values = []
@@ -237,15 +246,9 @@ class BaseColumn(object):
     return row_ids, values
 
   def _rename_cell_choice(self, renames, value):
-    return renames.get(value, value)
+    return renames.get(value)
 
 
-class DataColumn(BaseColumn):
-  """
-  DataColumn describes a column of raw data, and holds it.
-  """
-  pass
-
 class BoolColumn(BaseColumn):
   def set(self, row_id, value):
     # When 1 or 1.0 is loaded, we should see it as True, and similarly 0 as False. This is similar
@@ -356,7 +359,7 @@ class PositionColumn(NumericColumn):
     return new_values, [(self._sorted_rows[i], pos) for (i, pos) in adjustments]
 
 
-class ChoiceListColumn(BaseColumn):
+class ChoiceListColumn(ChoiceColumn):
   """
   ChoiceListColumn's default value is None, but is presented to formulas as the empty list.
   """
@@ -376,7 +379,8 @@ class ChoiceListColumn(BaseColumn):
     return () if typed_value is None else typed_value
 
   def _rename_cell_choice(self, renames, value):
-    return tuple(renames.get(choice, choice) for choice in value)
+    if any((v in renames) for v in value):
+      return tuple(renames.get(choice, choice) for choice in value)
 
 
 class BaseReferenceColumn(BaseColumn):
@@ -502,6 +506,7 @@ class ReferenceListColumn(BaseReferenceColumn):
 usertypes.BaseColumnType.ColType = DataColumn
 usertypes.Reference.ColType = ReferenceColumn
 usertypes.ReferenceList.ColType = ReferenceListColumn
+usertypes.Choice.ColType = ChoiceColumn
 usertypes.ChoiceList.ColType = ChoiceListColumn
 usertypes.DateTime.ColType = DateTimeColumn
 usertypes.Date.ColType = DateColumn
diff --git a/sandbox/grist/test_useractions.py b/sandbox/grist/test_useractions.py
index e393af1d..f1a417af 100644
--- a/sandbox/grist/test_useractions.py
+++ b/sandbox/grist/test_useractions.py
@@ -791,6 +791,8 @@ class TestUserActions(test_engine.EngineTestCase):
           [3, "c"],
           [4, "d"],
           [5, None],
+          [6, 5],
+          [7, [[]]],
         ],
         "ChoiceListTable": [
           ["id", "ChoiceListColumn"],
@@ -803,6 +805,8 @@ class TestUserActions(test_engine.EngineTestCase):
           [8, ["b", "c"]],
           [9, ["a", "c"]],
           [10, ["a", "b", "c"]],
+          [11, 5],
+          [12, [[]]],
         ],
       }
     })
@@ -811,11 +815,33 @@ class TestUserActions(test_engine.EngineTestCase):
     # Renames go in a loop to make sure that works correctly
     # a -> b -> c -> a -> b -> ...
     renames = {"a": "b", "b": "c", "c": "a"}
-    self.apply_user_action(
+    out_actions_choice = self.apply_user_action(
       ["RenameChoices", "ChoiceTable", "ChoiceColumn", renames])
-    self.apply_user_action(
+    out_actions_choice_list = self.apply_user_action(
       ["RenameChoices", "ChoiceListTable", "ChoiceListColumn", renames])
 
+    self.assertPartialOutActions(
+      out_actions_choice,
+      {'stored':
+         [['BulkUpdateRecord',
+           'ChoiceTable',
+           [1, 2, 3],
+           {'ChoiceColumn': [u'b', u'c', u'a']}]]})
+
+    self.assertPartialOutActions(
+      out_actions_choice_list,
+      {'stored':
+         [['BulkUpdateRecord',
+           'ChoiceListTable',
+           [1, 2, 3, 7, 8, 9, 10],
+           {'ChoiceListColumn': [['L', u'b'],
+                                 ['L', u'c'],
+                                 ['L', u'a'],
+                                 ['L', u'b', u'c'],
+                                 ['L', u'c', u'a'],
+                                 ['L', u'b', u'a'],
+                                 ['L', u'b', u'c', u'a']]}]]})
+
     self.assertTableData('ChoiceTable', data=[
       ["id", "ChoiceColumn"],
       [1, "b"],
@@ -823,6 +849,8 @@ class TestUserActions(test_engine.EngineTestCase):
       [3, "a"],
       [4, "d"],
       [5, None],
+      [6, 5],
+      [7, [[]]],
     ])
 
     self.assertTableData('ChoiceListTable', data=[
@@ -836,4 +864,6 @@ class TestUserActions(test_engine.EngineTestCase):
       [8, ["c", "a"]],
       [9, ["b", "a"]],
       [10, ["b", "c", "a"]],
+      [11, 5],
+      [12, [[]]],
     ])