(core) Use MixedTypesKey for sort_by arg of lookupRecords to avoid errors in Python 3

Summary: title

Test Plan: Added python unit test. Seems like the first test of sort_by in the whole codebase.

Reviewers: dsagal

Reviewed By: dsagal

Differential Revision: https://phab.getgrist.com/D3124

sandbox/grist/column.py

@@ -1,6 +1,7 @@
 import json
 import types
 from collections import namedtuple
+from numbers import Number
 
 import six
 
@@ -303,12 +304,13 @@ class DateTimeColumn(NumericColumn):
     return _sample_datetime
 
 
-class MixedTypesKey(object):
+class SafeSortKey(object):
   """
-  Sort key that can contain different types.
-  This mimics Python 2 where values of different types can be compared,
-  falling back on some comparison of the types when the values
-  can't be compared normally.
+  Sort key that deals with errors raised by normal comparison,
+  in particular for values of different types or values that can't
+  be compared at all (e.g. None).
+  This somewhat mimics the way that Python 2 compares values.
+  This is only needed in Python 3.
   """
 
   __slots__ = ("value",)
@@ -326,19 +328,36 @@ class MixedTypesKey(object):
     try:
       return self.value < other.value
     except TypeError:
-      return type(self.value).__name__ < type(other.value).__name__
+      if type(self.value) is type(other.value):
+        return id(self.value) < id(other.value)
+      else:
+        return self.type_position() < other.type_position()
+
+  def type_position(self):
+    # Fallback order similar to Python 2:
+    # - None is less than everything else
+    # - Numbers are less than other types
+    # - Other types are ordered by type name
+    # The first two elements use the fact that False < True (because 0 < 1)
+    return (
+      self.value is not None,
+      not isinstance(self.value, Number),
+      type(self.value).__name__,
+    )
 
 
 if six.PY2:
-  def MixedTypesKey(x):
+  _doc = SafeSortKey.__doc__
+  def SafeSortKey(x):
     return x
+  SafeSortKey.__doc__ =_doc
 
 
 class PositionColumn(NumericColumn):
   def __init__(self, table, col_id, col_info):
     super(PositionColumn, self).__init__(table, col_id, col_info)
     # This is a list of row_ids, ordered by the position.
-    self._sorted_rows = SortedListWithKey(key=lambda x: MixedTypesKey(self.raw_get(x)))
+    self._sorted_rows = SortedListWithKey(key=lambda x: SafeSortKey(self.raw_get(x)))
 
   def set(self, row_id, value):
     self._sorted_rows.discard(row_id)
@@ -349,7 +368,7 @@ class PositionColumn(NumericColumn):
   def copy_from_column(self, other_column):
     super(PositionColumn, self).copy_from_column(other_column)
     self._sorted_rows = SortedListWithKey(other_column._sorted_rows[:],
-                                          key=lambda x: MixedTypesKey(self.raw_get(x)))
+                                          key=lambda x: SafeSortKey(self.raw_get(x)))
 
   def prepare_new_values(self, values, ignore_data=False, action_summary=None):
     # This does the work of adjusting positions and relabeling existing rows with new position

sandbox/grist/grist.py

@@ -10,6 +10,7 @@ from usertypes import Any, Text, Blob, Int, Bool, Date, DateTime, \
 from usertypes import PositionNumber, ManualSortPos, Reference, ReferenceList, formulaType
 from table import UserTable
 from records import Record, RecordSet
+from column import SafeSortKey
 
 DOCS = [(__name__, (Record, RecordSet, UserTable)),
         ('lookup', (UserTable.lookupOne, UserTable.lookupRecords))]

sandbox/grist/table.py

@@ -446,7 +446,8 @@ class Table(object):
     lookup_map = self._get_lookup_map(col_ids)
     row_id_set, rel = lookup_map.do_lookup(key)
     if sort_by:
-      row_ids = sorted(row_id_set, key=lambda r: self._get_col_value(sort_by, r, rel))
+      row_ids = sorted(row_id_set,
+                       key=lambda r: column.SafeSortKey(self._get_col_value(sort_by, r, rel)))
     else:
       row_ids = sorted(row_id_set)
     return self.RecordSet(row_ids, rel, group_by=kwargs, sort_by=sort_by)

sandbox/grist/test_lookups.py

@@ -746,3 +746,30 @@ return ",".join(str(r.id) for r in Students.lookupRecords(firstName=fn, lastName
           [102,  [102, 103],  [101, 103],  [103],           [102]],
           [103,  [],          [],          [],              []],
     ])
+
+  def test_sort_by(self):
+    self.load_sample(testutil.parse_test_sample({
+      "SCHEMA": [
+        [1, "Table1", [
+          [1, "num", "Numeric", False, "", "", ""],
+          [2, "lookup", "Any", True, "Table1.lookupRecords(sort_by='num').num", "", ""],
+        ]]
+      ],
+      "DATA": {
+        "Table1": [
+          ["id", "num"],
+          [1, 2],
+          [2, 1],
+          [3, 'foo'],
+          [4, 3],
+          [5, None],
+          [6, 0],
+        ]
+      }
+    }))
+
+    self.assertTableData(
+      "Table1", cols="subset", rows="subset", data=[
+        ["id", "lookup"],
+        [1, [None, 0, 1, 2, 3, 'foo']],
+      ])