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

[5.8][WIP] Support eager loading with limit #26035

Closed
wants to merge 1 commit into from
Closed

[5.8][WIP] Support eager loading with limit #26035

wants to merge 1 commit into from

Conversation

staudenmeir
Copy link
Contributor

There is currently no way to limit eager loading results per parent:

class User extends Model {
    public function posts() {
        return $this->hasMany(Post::class)->latest()->limit(5);
    }
}

User::with('posts')->get(); 
# Loads 5 posts for all users combined.
select * from "posts" where "posts"."user_id" in (1, 2, 3) order by "created_at" desc limit 5

There are multiple issues about this (#16217, #4835, #18014) and it also comes up regularly on StackOverflow etc.

We can support this feature with a window function (we are already using this technique to implement OFFSET queries on SQL Server):

select * from (
  select *, row_number() over (partition by "user_id" order by "created_at" desc) as row_num
  from "posts"
  where "posts"."user_id" in (1, 2, 3)
) as temp_table
where row_num <= 5
order by row_num

The query is supported by all four databases, but requires very recent versions of MySQL and SQLite:

  • MySQL 8.0 (2018-07-27): We could support older versions with this workaround.
  • PostgreSQL 9.3 (2013)
  • SQLite 3.25 (2018-09-15)
  • SQL Server 2008

I wrote a sample implementation for PostgreSQL because it's the only supported database in Homestead at the moment.

The relationships override the query builder's limit() and call the new partitionLimit() method. By checking the parent model's exists property, we can detect eager loading. This is necessary to support lazy loading ($user->posts). The window function would also work for lazy loading but the database might not support it.

Before I implement the other databases: Would you consider this for the core?

@deleugpn
Copy link
Contributor

deleugpn commented Oct 10, 2018

Promising. Great job.

@X-Coder264
Copy link
Contributor

This looks very nice. It'd be great if this would be in the core.

@staudenmeir staudenmeir changed the base branch from 5.7 to master October 11, 2018 04:09
@staudenmeir staudenmeir changed the title [5.7][WIP] Support eager loading with limit [5.8][WIP] Support eager loading with limit Oct 11, 2018
@Lelectrolux
Copy link
Contributor

Lelectrolux commented Oct 11, 2018

Currently using my own version of the mysql workaround, happy to see that mysql 8.0 helps to clean the madness even if I'm on my own, but I would love that in laravel's core.

For an example of use in real life, it's a needs that comes often when doing travelling best offers (like find the 3 best price for this plane/train in the next week). My current work is full of that.

@staudenmeir
Copy link
Contributor Author

@Lelectrolux Are you using the MySQL workaround with BelongsToMany or HasManyThrough relationships?

@Lelectrolux
Copy link
Contributor

@staudenmeir Currently, I'm using it on a hasMany relationship, but I'm hotswapping the table with a SQL view to have some computed prices on a join.

@staudenmeir
Copy link
Contributor Author

staudenmeir commented Oct 12, 2018

I added the remaining implementations. We can use the same SQL in all four databases. The only exception is that SQL Server always requires an ORDER BY clause (we use a dummy clause if necessary).

  • MySQL:
    I implemented the mentioned workaround for MySQL < 8.0. It works for HasMany relationships but not for BelongsToMany or HasManyThrough. For some reason, MySQL ignores the ORDER BY clause and always uses the row insertion order (fiddle). Does anyone have an idea on how to fix this? Unfortunately, this is quite hacky for BelongsToMany relationships.

  • SQLite:
    SQLite 3.25.0 has only been released a month ago, so this feature doesn't work with the current PHP distributions. You have to compile the latest pdo_sqlite extension yourself (I tested it on Windows). So, no integration tests.
    Since SQLite is mainly used for testing, I suggest that we completely ignore the partition limit on unsupported versions. This PR is "only" a performance improvement and I don't think it's worth breaking the users' tests.

  • SQL Server:
    I moved compileOver() and compileTableExpression() up to Grammar and refactored them to work with the new features.

@miki131
Copy link

miki131 commented Oct 15, 2018

@staudenmeir what about some LimitTrait and move there take and limit functions?

@staudenmeir
Copy link
Contributor Author

staudenmeir commented Oct 15, 2018

@miki131 That's not possible for limit(): All three methods are different from each other.

@miki131
Copy link

miki131 commented Oct 15, 2018

@staudenmeir take a look on getExistenceCompareKey() method. It can be useful for BelongsToMany.php and HasOneOrMany.php. Later i'll try find out about third relation.

@staudenmeir
Copy link
Contributor Author

Yes, but we still need a separate implementation for HasManyThrough. I don't think it makes sense to create a trait for two relationships and override it in the third.

@miki131
Copy link

miki131 commented Oct 16, 2018

@staudenmeir You can create getExistenceCompareKey method for HasManyThrough. Also override getParent method. I think it can be useful in future and obviously help you separate code into trait.

@staudenmeir
Copy link
Contributor Author

@miki131 The HasManyThrough relationship does have a $parent property, so getParent() can't just return the $farParent.

@ludo237
Copy link

ludo237 commented Oct 16, 2018

What about MariaDB?

Promising feature tho

@staudenmeir
Copy link
Contributor Author

staudenmeir commented Oct 17, 2018

@ludo237 MariaDB has been supporting window functions since 10.2, but they don't work in strict mode (bug report).

@taylorotwell
Copy link
Member

Do any other frameworks support this?

@staudenmeir
Copy link
Contributor Author

Rails doesn't support it:

If you eager load an association with a specified :limit option, it will be ignored, returning all the associated objects.

I don't know about Symfony, I couldn't find anything. So probably not.

@taylorotwell
Copy link
Member

Honestly I don't see a big incentive to take on the maintenance burden of maintaining this functionality. If it's not even supported by Rails that indicates to me it's not that widely useful.

@staudenmeir staudenmeir deleted the partition-limit branch October 24, 2018 04:35
@staudenmeir
Copy link
Contributor Author

I released the code as a package: https://github.com/staudenmeir/eloquent-eager-limit

@yaquawa
Copy link
Contributor

yaquawa commented Oct 25, 2018

Rejected as usual.

Tips: To get this into the core, the best way is send a PR to Rails.

@atrauzzi
Copy link
Contributor

atrauzzi commented Jun 6, 2019

Just hit this today, a bit confused as to why it was rejected. This would be a great - if not essential - addition to Eloquent. If people have the syntax available, it stands to reason they'll assume this is possible.

@driesvints
Copy link
Member

Hey @atrauzzi, there's a package for this which you may use: https://github.com/staudenmeir/eloquent-eager-limit

@tillkruss
Copy link
Contributor

When using PostgreSQL use DISTINCT ON: #4835 (comment)

@staudenmeir
Copy link
Contributor Author

Eager loading with limit will be supported natively in Laravel 11 (#49695).

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

Successfully merging this pull request may close these issues.