Sorting order in lists with equal sorting criteria #3419

laevandus · 2024-09-11T13:11:52Z

🔗 Issue Links

Resolves: PBE-5993

🎯 Goal

When sorting by key(s) with equal values the sort order in SDK models is kept unchanged

📝 Summary

Recommendation: Always try to add sorting criteria what contains one sorting key with unique values.

Fix an issue where microseconds in timestamps were ignored causing channel list sorting to have many channels with the same lastMessageAt timestamp (triggers active reordering)
Fix an issue where just sorting by unreadCount or other Int or Bool keys led to unexpected reordering in the channel list (many channels with equal sorting value). Break this by adding updatedAt sorting key internally.
Fix an issue of runtime sorting (post FRC sorting used when FRC does not support sorting key) returning incorrect order
Add ChannelMemberListSortingKey.userId for sorting channel members by id

🛠 Implementation

Microseconds parsing

DateFormatter supports milliseconds and ignores microseconds and nanoseconds. This led to date properties to lose precision and causing many equal values when sorting the list.

Equal values and sorting

Our SDK returns items sorted by Core Data sort descriptors (or by runtime sorting if custom sorting keys are used). When we sort over non-unique values and there are many items with the same value, the order returned by the SDK is inconsistent: channel1, channel2, channel3 can change to channel2, channel3, channel1 when DB changes happen. API responses, on the other hand, return consistent order even in cases like these. If we paginate, then we also do not want to see any reordering happening when the next page of channels is inserted into the DB. One way is to internally add a tie breaker.

The issue with runtime sorting

Runtime sorting is used when we can't sort on the Core Data model layer (DTO does not have a key for that field). I found that runtime sorting does not match with how NSSortDescriptor sorts items (nil values, multiple sort criteria). This PR fixes these issues.

🎨 Showcase

Channel list sorted by unread count

Before	After

🧪 Manual Testing Notes

Case: sort channel list by unread message count

Prerequisites

Open DemoAppCoordinator+DemoApp.swift:L105 and change default sorting to sort by unread count (gives many equal values)

channelListQuery = .init(
  filter: .containMembers(userIds: [userCredentials.id]),
  sort: [.init(key: .unreadCount, isAscending: false)]
)

Steps

Open the demo app and log in with an user with many channels
Scroll down to load more channels
Result: Channels do not reorder

☑️ Contributor Checklist

I have signed the Stream CLA (required)
This change should be manually QAed
Changelog is updated with client-facing changes
Changelog is updated with new localization keys
New code is covered by unit tests
Comparison screenshots added for visual changes
Affected documentation updated (docusaurus, tutorial, CMS)

🎁 Meme

Provide a funny gif or image that relates to your work on this pull request. (Optional)

Stream-SDK-Bot · 2024-09-11T13:24:46Z

SDK Size

`title`	`develop`	`branch`	`diff`	`status`
StreamChat	6.83 MB	6.85 MB	+17 KB	🟢
StreamChatUI	4.42 MB	4.42 MB	0 KB	🟢

Stream-SDK-Bot · 2024-09-11T13:32:32Z

SDK Performance

`target`	`metric`	`benchmark`	`branch`	`performance`	`status`
MessageList	Hitches total duration	10 ms	4.17 ms	58.3% 🔼	🟢
	Duration	2.6 s	2.54 s	2.31% 🔼	🟢
	Hitch time ratio	4 ms per s	1.65 ms per s	58.75% 🔼	🟢
	Frame rate	75 fps	78.06 fps	4.08% 🔼	🟢
	Number of hitches	1	0.4	60.0% 🔼	🟢
ChannelList	Hitches total duration	12.5 ms	14.19 ms	-13.52% 🔽	🔴
	Duration	2.6 s	2.54 s	2.31% 🔼	🟢
	Hitch time ratio	5 ms per s	5.59 ms per s	-11.8% 🔽	🔴
	Frame rate	72 fps	74.49 fps	3.46% 🔼	🟢
	Number of hitches	1.2	1.4	-16.67% 🔽	🔴

Stream-SDK-Bot · 2024-09-13T06:38:27Z

SDK Size

`title`	`develop`	`branch`	`diff`	`status`
StreamChat	6.83 MB	6.85 MB	+17 KB	🟢
StreamChatUI	4.42 MB	4.42 MB	0 KB	🟢

laevandus · 2024-09-16T13:55:49Z

Sources/StreamChat/Database/DatabaseSession.swift

@@ -303,6 +305,26 @@ protocol ChannelDatabaseSession {
    func removeChannels(cids: Set<ChannelId>)
 }

+extension ChannelDatabaseSession {


Without it, there are so many errors because of missing the offset argument (most of it is in tests). If we want to avoid this extension, I can take the time to fill it in.

can't we provide a default value for the offset?

or maybe move these to the test target?

Unfortunately can't set a default value in protocol but moving the extension to test target is a good idea.

Sources/StreamChat/Query/Sorting/ChannelListSortingKey.swift

laevandus · 2024-09-16T13:57:24Z

Sources/StreamChat/Query/Sorting/SortValue.swift

+        func isAnyNil(_ value: Any) -> Bool {
+            let mirror = Mirror(reflecting: value)
+            return mirror.displayStyle == .optional && mirror.children.isEmpty
        }


Is there a better way?

can't we do something like this instead:

func isAnyNil(_ value: Any) -> Bool { return (value as Any?) == nil }

This also does not work.

func test() { struct Item { let index: Int let date: Date? } let item1 = Item(index: 1, date: nil) let item2 = Item(index: 2, date: Date()) let keyPath: PartialKeyPath<Item> = \.date let lhsValue = item1[keyPath: keyPath] let rhsValue = item2[keyPath: keyPath] print(lhsValue, rhsValue) print(isAnyNil(lhsValue), isAnyNil(rhsValue)) print((lhsValue as Any?) == nil, (rhsValue as Any?) == nil) } func isAnyNil(_ value: Any) -> Bool { let mirror = Mirror(reflecting: value) return mirror.displayStyle == .optional && mirror.children.isEmpty }

gives

nil Optional(2024-09-17 10:18:17 +0000) true false false false

maybe String(describing: value) == "nil"? 😅

@laevandus You can check DebugObjectViewController, we do nil comparison there.

I want to avoid reflection though, this will be run for every channel so it might impact performance.

laevandus · 2024-09-16T13:57:58Z

Sources/StreamChat/Utils/Sequence+CompactMapLoggingError.swift

-extension Collection {
+extension Sequence {


Needed it for sorting .enumerated()

laevandus · 2024-09-16T13:58:42Z

Sources/StreamChat/Query/Sorting/SortValue.swift

@@ -10,49 +10,54 @@ struct SortValue<T> {
 }

 extension Array {
-    func sort(using sorting: [SortValue<Element>]) -> [Element] {


Renamed it to sorted because sort means in-place in the Foundation APIs.

Tests/StreamChatTests/Query/Sorting/SortingValue_Tests.swift

martinmitrevski

I tested it also on SwiftUI and I still get an occasional jump. Have you tried it there?

martinmitrevski · 2024-09-17T08:04:12Z

Sources/StreamChat/Database/DatabaseSession.swift

@@ -303,6 +305,26 @@ protocol ChannelDatabaseSession {
    func removeChannels(cids: Set<ChannelId>)
 }

+extension ChannelDatabaseSession {


can't we provide a default value for the offset?

martinmitrevski · 2024-09-17T08:06:41Z

Sources/StreamChat/Database/DTOs/ChannelDTO.swift

+    // Note: When the channel is part of multiple channel list controllers,
+    // it might still cause some movement because the property is shared.
+    // Recommendation: use at least one sorting parameter with unique values.
+    @NSManaged var sortIndex: Int


another option can be to keep a map of the queries and the indexes, but I guess it's not worth it

I considered it but it felt like it gets too complex and also sort descriptor wants a key-path.

martinmitrevski · 2024-09-17T08:07:11Z

Sources/StreamChat/Database/DatabaseSession.swift

@@ -303,6 +305,26 @@ protocol ChannelDatabaseSession {
    func removeChannels(cids: Set<ChannelId>)
 }

+extension ChannelDatabaseSession {


or maybe move these to the test target?

martinmitrevski · 2024-09-17T08:12:03Z

Sources/StreamChat/Query/Sorting/SortValue.swift

+        func isAnyNil(_ value: Any) -> Bool {
+            let mirror = Mirror(reflecting: value)
+            return mirror.displayStyle == .optional && mirror.children.isEmpty
        }


can't we do something like this instead:

func isAnyNil(_ value: Any) -> Bool { return (value as Any?) == nil }

github-actions · 2024-09-19T12:15:00Z

	1 Warning
⚠️	Big PR

Generated by 🚫 Danger

nuno-vieira · 2024-09-19T15:11:30Z

Sources/StreamChat/Query/Sorting/SortValue.swift

+    /// The first `SortValue` is the primary key and subsequent `SortValue`s are for breaking the tie.
+    func sorted(using sortValues: [SortValue<Element>]) -> [Element] {
+        guard !sortValues.isEmpty else { return self }
+        return sorted { lhs, rhs in
+            for sortValue in sortValues {
+                let lhsValue = lhs[keyPath: sortValue.keyPath]
+                let rhsValue = rhs[keyPath: sortValue.keyPath]
+                let isAscending = sortValue.isAscending
+                // These are type-erased, therefore we need to manually cast them
+                if let result = nilComparison(lhs: lhsValue, rhs: rhsValue, isAscending: isAscending) {
+                    return result
+                } else if let result = areInIncreasingOrder(lhs: lhsValue, rhs: rhsValue, type: Date.self, isAscending: isAscending) {
+                    return result
+                } else if let result = areInIncreasingOrder(lhs: lhsValue, rhs: rhsValue, type: String.self, isAscending: isAscending) {
+                    return result
+                } else if let result = areInIncreasingOrder(lhs: lhsValue, rhs: rhsValue, type: Int.self, isAscending: isAscending) {
+                    return result
+                } else if let result = areInIncreasingOrder(lhs: lhsValue, rhs: rhsValue, type: Double.self, isAscending: isAscending) {
+                    return result
+                } else if let lBool = lhsValue as? Bool, let rBool = rhsValue as? Bool, lBool != rBool {
+                    // Backend considers boolean sorting in reversed order.
+                    return isAscending ? lBool && !rBool : !lBool && rBool


Do we still need these changes, after we changed the strategy to use an additional sort descriptor for the tie breaker?

We still need, this code-path is triggered when someone uses a custom sorting key which can't be represented by the FRC's sort descriptors. Most of the time, this code never runs, only these specific cases when someone goes for custom sorting keys.

Sorting keys what have localKey nil. Nil means we can't use sort descriptor and need to resort post-FRC sorting.

public static let hasUnread = Self( keyPath: \.hasUnread, localKey: nil, remoteKey: "has_unread" ) public static func custom<T>(keyPath: KeyPath<ChatChannel, T>, key: String) -> Self { .init(keyPath: keyPath, localKey: nil, remoteKey: key) }

nuno-vieira

The new approach sounds much better 👍 Just added minor comments but overall LGTM!

… with NSSortDescriptor order

…as API responses for breaking ties in the fetched results controller

…ey for sorting by user id, and add sorting tie breakers to controllers

laevandus · 2024-09-20T06:59:03Z

Sources/StreamChat/Query/Sorting/ChannelListSortingKey.swift

@@ -62,7 +62,7 @@ public struct ChannelListSortingKey: SortingKey, Equatable {

    /// Sort channels by their unread count.
    public static let unreadCount = Self(
-        keyPath: \.unreadCount,
+        keyPath: \.unreadCount.messages,


unreadCount is a struct, not Int, this is used for runtime sorting

laevandus · 2024-09-20T07:03:40Z

Sources/StreamChat/Utils/Codable+Extensions.swift

+        guard let index = string.lastIndex(of: ".") else { return date }
+        let range = string.suffix(from: index)
+            .dropFirst(4) // . and ms part
+            .dropLast() // Z
+        var fractionWithoutMilliseconds = String(range)
+        if fractionWithoutMilliseconds.count < 3 {
+            fractionWithoutMilliseconds = fractionWithoutMilliseconds.padding(toLength: 3, withPad: "0", startingAt: 0)
+        }
+        guard let microseconds = TimeInterval("0.000".appending(fractionWithoutMilliseconds)) else { return date }
+        return date.addingTimeInterval(microseconds)


Maybe not the most beautiful but it needs to handle cases like

2024-09-18T13:49:11.3241Z 2024-09-18T13:49:11.32412Z 2024-09-18T13:49:11.324123Z 2024-09-18T13:49:11.3241234Z 2024-09-18T13:49:11.32412345Z 2024-09-18T13:49:11.324123456Z

sonarcloud · 2024-09-20T10:35:37Z

Quality Gate passed

Issues
161 New issues
0 Accepted issues

Measures
0 Security Hotspots
84.1% Coverage on New Code
0.1% Duplication on New Code

See analysis details on SonarCloud

laevandus added 🐞 Bug An issue or PR related to a bug 🌐 SDK: StreamChat (LLC) Tasks related to the StreamChat LLC SDK labels Sep 11, 2024

laevandus requested a review from a team as a code owner September 11, 2024 13:11

laevandus changed the title ~~Runtime sorting gave inconsistent sort order and did not always match with NSSortDescriptor order~~ [WIP] Runtime sorting gave inconsistent sort order and did not always match with NSSortDescriptor order Sep 11, 2024

laevandus added the 🔧 WIP A PR that is Work in Progress label Sep 11, 2024

laevandus force-pushed the fix/sorting-consistency branch from a087116 to 20c8f56 Compare September 13, 2024 06:31

laevandus force-pushed the fix/sorting-consistency branch from 20c8f56 to 2e5c54e Compare September 16, 2024 12:11

laevandus changed the title ~~[WIP] Runtime sorting gave inconsistent sort order and did not always match with NSSortDescriptor order~~ Sorting order in lists with equal sorting criterias Sep 16, 2024

laevandus changed the title ~~Sorting order in lists with equal sorting criterias~~ Sorting order in lists with equal sorting criteria Sep 16, 2024

laevandus commented Sep 16, 2024

View reviewed changes

Sources/StreamChat/Query/Sorting/ChannelListSortingKey.swift Show resolved Hide resolved

laevandus commented Sep 16, 2024

View reviewed changes

Tests/StreamChatTests/Query/Sorting/SortingValue_Tests.swift Show resolved Hide resolved

laevandus removed the 🔧 WIP A PR that is Work in Progress label Sep 17, 2024

laevandus changed the title ~~Sorting order in lists with equal sorting criteria~~ [WIP] Sorting order in lists with equal sorting criteria Sep 17, 2024

laevandus changed the title ~~[WIP] Sorting order in lists with equal sorting criteria~~ Sorting order in lists with equal sorting criteria Sep 17, 2024

martinmitrevski reviewed Sep 17, 2024

View reviewed changes

laevandus changed the title ~~Sorting order in lists with equal sorting criteria~~ [WIP] Sorting order in lists with equal sorting criteria Sep 17, 2024

laevandus force-pushed the fix/sorting-consistency branch 2 times, most recently from 2820a2a to d56904c Compare September 18, 2024 12:36

laevandus changed the title ~~[WIP] Sorting order in lists with equal sorting criteria~~ Sorting order in lists with equal sorting criteria Sep 19, 2024

laevandus force-pushed the fix/sorting-consistency branch from b12a9be to 25db257 Compare September 19, 2024 12:24

nuno-vieira reviewed Sep 19, 2024

View reviewed changes

laevandus added 4 commits September 20, 2024 09:57

Runtime sorting gave inconsistent sort order and did not always match…

ae70997

… with NSSortDescriptor order

Add additional sorting descriptor which roughly gives the same order …

3f1c9a9

…as API responses for breaking ties in the fetched results controller

Support microsecond precision for dates, add channel member sorting k…

0c99cba

…ey for sorting by user id, and add sorting tie breakers to controllers

Add test for storing microseconds date in DB

17802e2

laevandus commented Sep 20, 2024

View reviewed changes

Fix unit-tests after timestamp precision increase

c11edf4

laevandus force-pushed the fix/sorting-consistency branch from 418cc33 to c11edf4 Compare September 20, 2024 08:26

Fix remaining tests

e4f3dda

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sorting order in lists with equal sorting criteria #3419

Sorting order in lists with equal sorting criteria #3419

laevandus commented Sep 11, 2024 •

edited

Loading

Stream-SDK-Bot commented Sep 11, 2024

Stream-SDK-Bot commented Sep 11, 2024

Stream-SDK-Bot commented Sep 13, 2024 •

edited

Loading

laevandus Sep 16, 2024

martinmitrevski Sep 17, 2024

martinmitrevski Sep 17, 2024

laevandus Sep 17, 2024

laevandus Sep 17, 2024

laevandus Sep 16, 2024

martinmitrevski Sep 17, 2024

laevandus Sep 17, 2024

martinmitrevski Sep 17, 2024

nuno-vieira Sep 17, 2024

martinmitrevski Sep 17, 2024

laevandus Sep 16, 2024

laevandus Sep 16, 2024

martinmitrevski left a comment

martinmitrevski Sep 17, 2024

martinmitrevski Sep 17, 2024

laevandus Sep 17, 2024

martinmitrevski Sep 17, 2024

martinmitrevski Sep 17, 2024

github-actions bot commented Sep 19, 2024

nuno-vieira Sep 19, 2024

laevandus Sep 20, 2024

laevandus Sep 20, 2024

nuno-vieira left a comment •

edited

Loading

laevandus Sep 20, 2024

laevandus Sep 20, 2024

sonarcloud bot commented Sep 20, 2024

Sorting order in lists with equal sorting criteria #3419

Are you sure you want to change the base?

Sorting order in lists with equal sorting criteria #3419

Conversation

laevandus commented Sep 11, 2024 • edited Loading

🔗 Issue Links

🎯 Goal

📝 Summary

🛠 Implementation

Microseconds parsing

Equal values and sorting

The issue with runtime sorting

🎨 Showcase

Channel list sorted by unread count

🧪 Manual Testing Notes

Case: sort channel list by unread message count

Prerequisites

Steps

☑️ Contributor Checklist

🎁 Meme

Stream-SDK-Bot commented Sep 11, 2024

SDK Size

Stream-SDK-Bot commented Sep 11, 2024

SDK Performance

Stream-SDK-Bot commented Sep 13, 2024 • edited Loading

SDK Size

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinmitrevski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Sep 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nuno-vieira left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented Sep 20, 2024

Quality Gate passed

laevandus commented Sep 11, 2024 •

edited

Loading

Stream-SDK-Bot commented Sep 13, 2024 •

edited

Loading

nuno-vieira left a comment •

edited

Loading