pilagod / gorm-cursor-paginator

A paginator doing cursor-based pagination based on GORM
https://github.com/pilagod/gorm-cursor-paginator
MIT License
188 stars 44 forks source link

Order with NULL FIRST/LAST implicitly. #54

Closed emfomy closed 11 months ago

emfomy commented 1 year ago

How to order with NULL FIRST/LAST implicitly?

I need this feature for indices.

COALESCE and NULL FIREST/LAST are treated differently while choosing indices.

pilagod commented 1 year ago

Hi @emfomy, not sure if I understand you correctly, I think the NULLReplacement config may help you to achieve the goal.

In the Specification section in the README, it describes the rationale about this setting, in the paginator.Rule:

Example usage in the test case: https://github.com/pilagod/gorm-cursor-paginator/blob/f807e66213b074b491dff8cf5e7fc3040f9768df/paginator/paginator_paginate_test.go#L523-L565

emfomy commented 1 year ago

Hi @pilagod! Thanks for your response.

I'm currently using NULLReplacement as you mentioned in my project.

Here is a simple example (with PostgreSQL) of what I'm doing now:

type Foo struct {
    ID int `gorm:"primaryKey"`
}

pgn := paginator.New(
    &paginator.Config{
        Rules: []paginator.Rule{{
            Key:             "ID",
            NULLReplacement: 0,
        }},
        Order: paginator.DESC,
    },
)

var foos []model.Foo
pgn.Paginate(db, &foos)

And the code generates

SELECT * FROM "foos" ORDER BY COALESCE(foos.id, '0') DESC LIMIT 11

However, by applying EXPLAIN on above SQL query with different indices listed below, I found that this query uses idx_foo_coallesce index but not idx_foo_nulls_last (which is the index defined in my project)

CREATE INDEX idx_foo_nulls_last ON foos (foos.id DESC NULLS LAST);
CREATE INDEX idx_foo_coallesce ON foos (COALESCE(foos.id, '0') DESC);

It would be great if you can add an option to make the SQL query to

SELECT * FROM "foos" ORDER BY foos.id DESC NULLS LAST LIMIT 11
pilagod commented 1 year ago

This is a good feedback. I understand and agree with you that using native mechanism to handle NULL ordering would be a more ideal solution.

One critical obstacle makes this package implement NULLReplacement instead of native NULL ordering is that, each vendor of database uses different ways to handle NULL ordering, as you can see in this article:

https://learnsql.com/blog/how-to-order-rows-with-nulls/

And if we take this way, we must have to manually mapping the SQL query case by case for every vendor, and it would cause high maintenance cost for this package. That's why for what it looks like now 🥲

emfomy commented 1 year ago

How about leaving this obstacle to the end user?

We may add an extra option to Rule, and put this value in Paginator.buildOrderSQL after order?

pilagod commented 12 months ago

@emfomy Thanks for your idea, it seems reasonable to push the customization to the client side, to let client configure based on their database.

The PR impl doesn't look general enough to me. I think maybe we could have a new config NullOrderBuilder at the global or option level, and introduce a new field NullOrder in the Rule, such as

// package paginator
// A global level NullOrderBuilder, imaging a function like `func (rule Rule, order Order) string`
func SetNullOrderBuilder(func (rule Rule, order Order) string ) { ... }

type NullOrder string

const (
    First NullOrder = "FIRST"
    Last NullOrder = "LAST"
)

// Postgres
func PostgresNullOrderBuilder(rule Rule, order Order) {
    return fmt.Sprintf("%s %s %s", rule.SQLRepr, order, rule.NullOrder)
}

// MySQL
func MySQLNullOrderBuilder(rule Rule, order Order) {
    stmt :=  fmt.Sprintf("%s %s", rule.SQLRepr, order)
    if rule.NullOrder == First {
        return fmt.Sprintf("%s IS NULL, %s", rule.SQLRepr, stmt)
    }
    return fmt.Sprintf("%s IS NOT NULL, %s", rule.SQLRepr, stmt)
}

// In project entry point
paginator.SetNullOrderBuilder(PostgresNullOrderBuilder)

Just draft my idea by code, and it needs to be polished further when putting into impl. What do you think about this direction?

emfomy commented 12 months ago

@pilagod Thanks for your response. I have refined the PR based on your idea. Please take a look, thanks!

emfomy commented 12 months ago

BTW, during implementation, I found that NullOrderBuilder doesn't need to be limited to null order only. It can be used for general order.

pilagod commented 11 months ago

I really appreciate your contribution 🙌

I was trying to help you to add tests, but found that this problem might be more complicate than we realized. When using null first/last to order records, it will also affect the where clause during pages.

For instance, suppose we have 3 records like:

id value
1 1
2 NULL
3 3

When we want to order them by value in DESC, with NULLS LAST and 2 items in a page. The first query will be (we need additional 1 item to know if we have next page or not, so the limit will be 3):

SELECT * FROM `records` ORDER BY value DESC NULLS LAST LIMIT 3;

This query can produce expected outcome, which contains record 3 and record 1. But if we keep paginating to the next page, it will produce an unexpected result:

SELECT * FROM `records` WHERE value < 1 ORDER BY value DESC NULLS LAST LIMIT 3;

Paginator will use the last record's column value in the previous page as the where condition for the next page query. In this example, the last item in first page has value 1, so the next query will append WHERE value < 1 condition. But this condition will EXCLUDE all nulls in advance, causing the second query to actually output no records, instead of record 2 in our expectation.

I think this is another reason we chose null replacement instead of null first/last. Sorry about that, I totally forgot this problem during our discussion 🫠

emfomy commented 11 months ago

I see. It seems like null replacement is the better solution. Thanks for you help again!