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

current sorting on multiple values is broken by design #12

Open
postsql opened this issue Jan 13, 2014 · 6 comments
Open

current sorting on multiple values is broken by design #12

postsql opened this issue Jan 13, 2014 · 6 comments
Milestone

Comments

@postsql
Copy link

postsql commented Jan 13, 2014

the test says:

it('should sort records using multiple sort criteria, with first name asc', function(done) {
User.find({ where: { type: 'sort test multi' }, sort: { last_name: 1, first_name: 1 } }

but as there is no defined order for object fields (hash/dictionary keys), then you can not meaningfully specify multi-key sort order using dictionaries. It has to be an Array, so the test (and implementations) should do:

User.find({ where: { type: 'sort test multi' }, sort: [{ last_name: 1}, {first_name: 1 }] }

to guarantee that result is sorted on last_name first and then on first_name

@mikermcneil
Copy link
Member

Currently, the version of V8 in Node keeps track of key order, so it would work for now, but I'd rather not rely on that too much.

originally, we went with that format for familiarity from Mongo syntax. But looks like they actually want you to use compound indexes for multiple sort criteria (see http://docs.mongodb.org/manual/tutorial/sort-results-with-indexes/)

What your'e suggesting is good-- but I think we'd need to make some changes in core. It could be done in a backwards-compatible way (if you pass in an object, it'll do it the current way-- if you pass in an array, it'll do it the new way)

Either way, to make it easier to standardize the API for multi-criteria sort across all of our adapters, I think it would make sense to pull out a separate interface, i.e. multisortable (similar to the aggregatable issue)

@postsql @particlebanana @sgress454 What do you guys think?

@dmarcelino
Copy link
Member

Any update on this?

@atiertant
Copy link

ECMAScript spec says :
4.3.3 Object
An object is a member of the type Object. It is an unordered collection of properties each of which contains a primitive value, object, or function. A function stored in a property of an object is called a method.

i think using a V8 current specificity is not a good way.

@mikermcneil you are right,using interface to detect adapter functionality but is there some doc about adapter function and interface spec ?

@dmarcelino
Copy link
Member

i think using a V8 current specificity is not a good way.

Agreed, balderdashy/waterline#694 should fix this.

is there some doc about adapter function and interface spec ?

The best source and probably a bit dated is sails-docs - adapter-specification. There are other references in waterline-docs - adapters that may help.

@particlebanana
Copy link
Contributor

@atiertant @dmarcelino let's add this in the 0.11 milestone on Waterline.

@dmarcelino dmarcelino added this to the 0.11 milestone Jul 8, 2015
@dmarcelino
Copy link
Member

balderdashy/waterline#694 and this are now added to milestone 0.11.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

5 participants