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

Add optimized selects #1626

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

DDynamic
Copy link

  • Added or updated tests
  • Documented user facing changes
  • Updated CHANGELOG.md

Resolves #1356

Changes
This MR optimizes queries generated by eloquent by only including the requested fields.

type Query {
  posts: [Post] @all
}

type Post {
  id: ID
  likes: Int
  title: String
  uppercaseTitle: String @select(fields: ["title"]) @method(name: "uppercaseTitle")
}
{
  posts {
    id
    uppercaseTitle
  }
}

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.

@DDynamic DDynamic marked this pull request as draft November 27, 2020 04:55
@spawnia
Copy link
Collaborator

spawnia commented Nov 27, 2020

First, I suggest to implement the trivial case without @select:

{
  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 @select. You will have to combine the field selection from $resolveInfo with the type information from the schema. Tricky, but definitely doable.

The actual @select directive probably won't be much more than a marker and probably not contain much code.

Since many resolver directives can benefit from this optimization, it should be abstracted and made reusable. Maybe in a similar way as ArgumentSet::enhanceBuilder().

@spawnia spawnia added enhancement A feature or improvement performance Fix or optimization of performance labels Nov 27, 2020
@spawnia
Copy link
Collaborator

spawnia commented Nov 27, 2020

Also, see #1253

@DDynamic DDynamic force-pushed the feat-optimized-selects branch 10 times, most recently from 0f03dc4 to 326b6ab Compare November 29, 2020 20:36
"""
SQL columns names to pass to the Eloquent query builder
"""
columns: [String!]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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);
Copy link
Collaborator

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.

Copy link
Collaborator

@spawnia spawnia left a 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.

@DDynamic
Copy link
Author

DDynamic commented Jan 11, 2021

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.

@spawnia
Copy link
Collaborator

spawnia commented Jan 11, 2021

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);
Copy link

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.

Suggested change
return array_unique($selectColumns);
$allColumns = Schema::getColumnListing($model->getTable());
return array_intersect($allColumns, array_unique($selectColumns));

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement A feature or improvement performance Fix or optimization of performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Select only necessary columns for optimized queries
2 participants