Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ord instance appears to be broken #15

Open
lgastako opened this issue Oct 5, 2023 · 3 comments
Open

Ord instance appears to be broken #15

lgastako opened this issue Oct 5, 2023 · 3 comments

Comments

@lgastako
Copy link

lgastako commented Oct 5, 2023

λ> accountId0
Id {unId = 00000000000000000000000000}
λ> accountId1
Id {unId = 00000000000000000000000001}
λ> hash accountId0
5618888146721150863
λ> hash accountId1
5618887047209522684
λ> accountId0 == accountId1
False
λ> -- So far so good, but...
λ> accountId0 < accountId1
False
λ> -- and...
λ> accountId0 > accountId1
False
@TheInnerLight
Copy link

I Just spotted this too, this is because:

instance Ord ULID where
    compare (ULID ts1 _) (ULID ts2 _) = compare ts1 ts2

This means the random component is ignored and breaks lexicographic ordering.

I don't see any reason that the ULIDRandom type shouldn't derive Ord and then ULID can also derive Ord.

@ad-si
Copy link
Member

ad-si commented Oct 21, 2024

I know it seems weird, but I'd argue that this is the correct behavior:
It's not exactly the same thing, but neither one happens before the other one as they have the same timestamp.

Can you give an example for code that should work, but doesn't because of this behavior?

@TheInnerLight
Copy link

TheInnerLight commented Oct 21, 2024

My use case that is broken involves putting ULIDs in a Map or Set. The result is that not all the ULIDs will be stored in the map, only the first one with a unique timestamp because the data structure will assume that the ULIDs with the same timestamp but different random components are actually equal.

This is an issue with any data type where the definitions of Ord and Eq don't align. In the docs (https://hackage.haskell.org/package/base-4.20.0.1/docs/Data-Ord.html) property 6 (x == y = compare x y == EQ) is supposed to hold for all Ord instances but it doesn't here.

Ord instances are for types with total orders and that indeed should be the case as lexicographic ordering is a core component of the ULID data type. Indeed, one of the key motivating use cases for ULIDs is their sequential nature for efficient database insert and indexing.

The way the ordering is currently implemented in this library is currently not lexicographic and is a partial order rather than the total order implied by the Ord instance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants