-
-
Notifications
You must be signed in to change notification settings - Fork 438
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
Add optimized selects #1626
base: master
Are you sure you want to change the base?
Add optimized selects #1626
Conversation
First, I suggest to implement the trivial case without {
posts {
id
title
}
} select id, title from posts; Next, consider the implementation of https://github.com/nuwave/lighthouse/blob/master/src/Schema/Directives/CacheDirective.php, that should give you an idea how you can grab The actual Since many resolver directives can benefit from this optimization, it should be abstracted and made reusable. Maybe in a similar way as |
Also, see #1253 |
0f03dc4
to
326b6ab
Compare
f8b749e
to
4adbee0
Compare
""" | ||
SQL columns names to pass to the Eloquent query builder | ||
""" | ||
columns: [String!] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
columns: [String!] | |
columns: [String!]! |
This directive has no use without this argument.
if ($fieldDefinition) { | ||
$directivesRequiringLocalKey = ['hasOne', 'hasMany', 'count']; | ||
$directivesRequiringForeignKey = ['belongsTo', 'belongsToMany', 'morphTo']; | ||
$directivesRequiringKeys = array_merge($directivesRequiringLocalKey, $directivesRequiringForeignKey); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we are trying to optimize performance here: Those should all be const
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice to see you pick up work on this branch again, this will be a big improvement when it lands. The code looks pretty good so far.
This feature seems like something that can have a lot of weird corner cases, so don't forget to add comprehensive tests.
Hey, @spawnia, thanks for checking in. I tried to get some work done on it this weekend, but I hit a wall with some edge cases and Eloquent limitations surrounding polymorphic relationships (example), so I decided to push what I had. The app that utilizes this only employs the all, paginate, and one-to-one and one-to-many relationship directives, so those use cases are working. I'm not sure how many more hours I can put into this to address all of the edge cases. It would be an amazing edition though, nonetheless. |
A partial solution is better than no solution at all, if there are shortcomings we can document them and offer users the choice. Let's just make sure that whatever is implemented actually works. |
} | ||
} | ||
|
||
return array_unique($selectColumns); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure, but if I need an accessor that is not in the table columns, we will get a "Column not found" SQL error.
return array_unique($selectColumns); | |
$allColumns = Schema::getColumnListing($model->getTable()); | |
return array_intersect($allColumns, array_unique($selectColumns)); |
@DDynamic |
Resolves #1356
Changes
This MR optimizes queries generated by eloquent by only including the requested fields.
The above schema definition and query cause
SELECT * FROM posts
to be executed. This MR adds adds in an opt-in configuration option to change this behavior to select only the fields required,SELECT id, title FROM posts
. It adds a select directive to specify field dependencies, like in the case of uppercaseTitle.@spawnia what do you think of this implementation idea? I could update the
AllDirecrive
to get the field selection with$resolveInfo
, but I am not sure how to "look ahead" to field selection to see if there is a select directive present.