subsquid / squid-sdk

The main repo of the squid SDK
Apache License 2.0
1.23k stars 151 forks source link

fix: NULL appears first when order By DESC #191

Closed justraman closed 1 year ago

eldargab commented 1 year ago

Hey, this is a breaking change and it's non-obvious that it is always what users want!

I'm not against CLI flag enabling such behaviour, also would be nice to know how Hasura and postgraphile behave...

justraman commented 1 year ago

Hey @eldargab , I see your point. I just checked the Hasura and Postgraphile as mentioned

  1. Postgraphile allows it by using an argument when initializing the engine orderByNullsLast: true
  2. On the other hand, Hasura handles nullable fields differently by creating additional OrderBy options desc_nulls_last, desc_nulls_first and default desc

Which one would you prefer?

eldargab commented 1 year ago

Can we agree on

query {
  list(orderBy: [foo_DESC_NULLS_LAST, bar_DESC, baz_ASC_NULLS_FIRST, qux_ASC]) {
    foo
    bar
  }
}

?

eldargab commented 1 year ago

To elaborate:

With the flag option it is possible to have 2 similarly looking servers behaving completely differently. Also that approach is not flexible.

That leaves us with the need to extend the interface...

justraman commented 1 year ago

Absolutely!, I will work on it this week.

eldargab commented 1 year ago

Great!

The very brief implementation guide will be

  1. extend the SortOrder
  2. check and fix places where it is used :)
justraman commented 1 year ago

@eldargab it's ready for review, i tried and tested locally.