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

Smarter filter and orderBy index optimizations #499

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

Conversation

marshall007
Copy link
Contributor

@marshall007 marshall007 commented May 12, 2016

This PR exposes a new method Model::ensureCompoundIndex and in using it, allows thinky to optimize multi-field filter and orderBy queries using compound indexes.

let Person = thinky.createModel('person', {
  id: String,
  first_name: String,
  last_name: String,
  active: Boolean
})

Person.ensureCompoundIndex('full_name', [ 'first_name', 'last_name' ])

Person.filter({ first_name: 'Foo', last_name: 'Bar', active: true })
// -> .getAll([ 'Foo', 'Bar' ], { index: 'full_name' }).filter({ active: true })

Person.orderBy('first_name', 'last_name')
// -> .orderBy({ index: 'full_name' })

It might be nice if we detected implicit vars passed to the ensureIndex and automatically pulled the field name(s) from the query object. That way we wouldn't need a separate method and index optimization would automatically work for index functions like r.row('field') and [ r.row('field1'), r.row('field2') ].

@neumino
Copy link
Owner

neumino commented May 14, 2016

The PR looks good to me. I'm a bit worried about the direction we are going though.

One of the nice thing about ReQL is that you know what RethinkDB is using. Indexes have to be explicitly used. I added this filter optimization at the beginning, but I'm not sure if someone is actually using it.

@marshall007 -- would you use filter instead of getAll knowing that filter wiill be transformed in a getAll?

@fenos
Copy link

fenos commented Jul 6, 2016

I think this PR is important, if thinky can help optimize queries then the developer has less to think about at least for filters and order by. Of course this should be explicitly requested, toggling a parameter / or calling a specific function like ensureCompoundIndex. I would give it a go! :)

@marshall007
Copy link
Contributor Author

would you use filter instead of getAll knowing that filter will be transformed in a getAll?

@neumino sorry for the delay in following up with this, but yes absolutely! I have several apps where I end up duplicating similar logic for building an efficient query based on the specified query string parameters.

@marshall007
Copy link
Contributor Author

Also, in terms of the overall direction here I think optimizations like these are a completely reasonable and often expected feature of ORMs. I agree with @fenos that by explicitly defining by indices in the ORM I should reasonably expect it to optimize my queries automatically whenever possible.

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.

3 participants