From 267640c277d20004adc8d7779956b871f346cb11 Mon Sep 17 00:00:00 2001 From: Alex Hall Date: Tue, 9 Nov 2021 18:07:58 +0200 Subject: [PATCH] (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 | 37 ++++++++++++++++++++++++++--------- sandbox/grist/grist.py | 1 + sandbox/grist/table.py | 3 ++- sandbox/grist/test_lookups.py | 27 +++++++++++++++++++++++++ 4 files changed, 58 insertions(+), 10 deletions(-) diff --git a/sandbox/grist/column.py b/sandbox/grist/column.py index 48865cb8..a26bb2ed 100644 --- a/sandbox/grist/column.py +++ b/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 diff --git a/sandbox/grist/grist.py b/sandbox/grist/grist.py index 15c5cb75..7343eb42 100644 --- a/sandbox/grist/grist.py +++ b/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))] diff --git a/sandbox/grist/table.py b/sandbox/grist/table.py index 483d4006..ea30db4b 100644 --- a/sandbox/grist/table.py +++ b/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) diff --git a/sandbox/grist/test_lookups.py b/sandbox/grist/test_lookups.py index b7d577fe..4c94b80b 100644 --- a/sandbox/grist/test_lookups.py +++ b/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']], + ])