Equatable vs EncodableRecord for removeDuplicates()

[oddanderson]

In the documentation, there is the advice: Note: The comparison is performed on the database representation of records. As long as your record type adopts the EncodableRecord protocol, you don't need to care about Equatable in reference to being able to use databaseEquals().

I am trying to use the function removeDuplicates() but it looks like the function is expecting you to have explicitly implemented the Equatable protocol, and my class objects only extend Record and therefore EncodableRecord

Is this intended? Should I expect to implement Equatable whenever I want to use removeDuplicates()?

[groue]

Should I expect to implement Equatable whenever I want to use removeDuplicates()?

In order to compare values that do not conform to Equatable, you have removeDuplicates(by:). You can call databaseEquals there:

ValueObservation
    .tracking { db in try Player.fetchOne(db, id: 42) }
    .removeDuplicate { prev, current in
        switch (prev, current) {
        case (nil, nil):
            return true
        case (let prev?, let current?):
            return prev.databaseEquals(current)
        default:
            return false
        }
    }

[oddanderson]

I don't want to get into a huge stylistic debate and it sounds like you probably already disagree. I was throwing out there that:

• extending a Record object is common
• observing a fetchAll is common
• removing duplicates on observation is common
  with the idea that if all three are common, perhaps it makes sense to think about removeDuplicates() also working automatically for Record objects.

That said, the other comparison technique works for me, and since I need to do observe/query joins in parts of my code, it looks like that style should be fine.

[groue]

We are on the same line: removing duplicates is common enough. Performing this check efficiently is not as trivial as it sounds, and by that I mean that most apps don't want this check to run on the main thread. Those two two reasons, combined, are exactly why there exists a dedicated method: removeDuplicates.

Now, it doesn't look like this is enough in your opinion. Is it that you think the databaseEquals method should be extended to other types such as Optionals, Sequences, and Collections of records, in order to streamline your use case?

With those extensions, we'd write the same code for records, optional records, and collections of records:

ValueObservation
    .tracking { ... }
    .removeDuplicates(by: { $0.databaseEquals($1) })

This is an interesting idea!

Maybe we'd need to make some performance checks, though, and compare with the row comparison technique:

Removing duplicates from rows is:

1. Copy database results into rows
2. Compare with previous rows
3. Convert new rows into records

Comparing duplicates from records is:

1. Convert database results into records
2. Compare with the database representation of previous records

Naively, it looks like the second technique performs more work. If it this intuition is exact, and comparing records is noticeably less efficient, we'd still prefer to foster the first technique (compare rows before they are converted to records).

But even this would not be a reason to not ship those new databaseEquals extensions. The performance checks would only impact the documentation.

[groue]

Is it that you think the databaseEquals method should be extended to other types such as Optionals, Sequences, and Collections of records, in order to streamline your use case?

With those extensions, we'd write the same code for records, optional records, and collections of records:

ValueObservation
    .tracking { ... }
    .removeDuplicates(by: { $0.databaseEquals($1) })

This is an interesting idea!

After giving this idea a little more thoughts, I no longer think it is a good idea 😅

databaseEquals was initially introduced as a way to compare two records, and two records only. It was one if a family of methods dedicated to records diffing, and efficient (minimal) database updates.

I have erred on the side of extending this method into a general "Database Equality" concept that would apply to optionals and collections as well. But I don't see what "Database Equality" brings that plain Equatable equality doesn't already have.

So I'm not sure this discussion has shown any clear direction for improving ValueObservation.removeDuplicates. It comes with built-in support for Equatable (which allows it to deal with Equatable records, and also raw database rows). And it comes with removeDuplicates(by:) for more customization. We're not worse than Combine's Publisher.

perhaps it makes sense to think about removeDuplicates() also working automatically for Record objects.

Maybe just have your records conform to Equatable, if that's what you want?

[oddanderson]

Sorry - I don't think my previous message was clear. Relying on Row implementing equality works for me, such that I can write:

let request = Event
    .annotated(withRequired: Event.contact.select(
        Column("id").forKey("contactId"),
        Column("name").forKey("contactName")))
    .asRequest(of: EventInfo.self)
let observation = ValueObservation.tracking { db -> [Row] in
    let eventRows = try Row.fetchAll(db, request)
    return eventRows
 }.removeDuplicates()
 .map { rows in rows.map(EventInfo.init(row:))}

This feels better to me than re-implementing equality check everywhere, even if both might be roughly the same number of lines of code. The other reason why I think it's fine is that in many cases, I need to query joining info, and the only way to realistically do that is by querying rows directly. Generally I prefer to use the same style everywhere, so if I have to query using rows in many places, I think it's generally better to do that everywhere even if I could shortcut and just query with the Record object directly. Having removeDuplicates() work implicitly for rows seems sufficient.

[groue]

Generally I prefer to use the same style everywhere

I could not agree more 💯 This really nails it. But thanks for giving the opportunity for a small exploration of possible alternatives 🙂

[oddanderson]

Wanted to follow-up on this. Does removeDuplicates() work if you are returning an array of rows via a fetchAll or only if you are returning an individual row via fetchOne? In my tests, it looks like it doesn't properly handle an array of rows

[groue]

removeDuplicates works, yes. This library is very tested. I don't mean that it is bug-free, but the responsibility of exhibiting bugs in on you.