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

MLIBZ-2156: Correctly sort fields that have a null value #205

Merged
merged 5 commits into from Jan 26, 2018

Conversation

@thomasconner
Copy link
Contributor

commented Jan 16, 2018

Description

Put docs with the sort field set to null at the beginning of the sorted array.

Tests

  • Added test that checks that fields with a null value are sorted to the beginning of the list.

@thomasconner thomasconner self-assigned this Jan 16, 2018

@thomasconner thomasconner requested a review from georgiwe Jan 16, 2018

@@ -778,9 +778,9 @@ export class Query {
const bField = nested(b, field);

if (isDefined(aField) && isDefined(bField) === false) {

This comment has been minimized.

Copy link
@georgiwe

georgiwe Jan 16, 2018

Contributor

Wouldn't this treat null and undefined the same? I'm not sure what the spec is - what should happen to objects, which do not posses the field? Regardless of that, wouldn't this result in an unpredictable order among entities for which isDefined() returns false?

This comment has been minimized.

Copy link
@thomasconner

thomasconner Jan 16, 2018

Author Contributor

Yes this does treat null and undefined the same (or appears too). When I tested it with null and undefined, null came first in an ascending order and undefined came first in a descending order.

The same order would be returned depending on if the sort is descending or ascending and isDefined() returned false.

This comment has been minimized.

Copy link
@georgiwe

georgiwe Jan 16, 2018

Contributor

Isn't that because of the order in the initial array? Because comparing null and undefined always returns false. And what is the desired behaviour here? We are trying to align it to the server's, right - what is that? Primitives should be considered too.

This comment has been minimized.

Copy link
@thomasconner

thomasconner Jan 18, 2018

Author Contributor

I will have to see what the server returns for these different use cases. I don't think there is a spec somewhere to reference.

This comment has been minimized.

Copy link
@thomasconner

thomasconner Jan 24, 2018

Author Contributor

For a sort on _id in descending order the server returns

[ 
  { _id: 2, customProperty: 'Xlv3a36WdCahHscBdu' },
  { _id: 1, customProperty: 'UbiUgNfxLdiHrX8vOS' },
  { _id: null, customProperty: 'ul29WKATNXhGD9SUwF' }, // _id is null
  { customProperty: 'yRNvuXJ79vC3OTW9HZ' } // _id is undefined
]

For a sort on _id in ascending order the server returns

[ 
  { customProperty: 'uhljZH0VRHUNbW9Ck2' }, // _id is undefined
  { _id: null, customProperty: 'YCtYEPDQYyPtnN8n3E' }, // _id is null
  { _id: 1, customProperty: 'oYYRouQ9eZ026J3pKq' },
  { _id: 2, customProperty: 'gUpApX6uCIjYg8FTx8' } 
]

This comment has been minimized.

Copy link
@georgiwe

georgiwe Jan 25, 2018

Contributor

How did you create an entity without an _id? I tested it with a different field and null always came up before objects which do not posses the field in ascending order - in both mongodb and for requests to Kinvey.

This comment has been minimized.

Copy link
@georgiwe

georgiwe Jan 26, 2018

Contributor

Ah, nevermind, turns out it's just the natural order that's at play here.

This comment has been minimized.

Copy link
@thomasconner

thomasconner Jan 26, 2018

Author Contributor

The natural order is preserved with the latest changes.

const entity4 = { _id: 3, customProperty: randomString() };
const query = new Query().ascending('_id');
query.fields = ['customProperty'];
const result = query.process([entity4, entity1, entity3, entity2]);

This comment has been minimized.

Copy link
@georgiwe

georgiwe Jan 16, 2018

Contributor

It would help to define the spec more thoroughly to add an entity which does not contain a someProperty field at all. Or maybe a primitive even - are those treated the same as if they are objects, which don't have it?

@@ -934,6 +934,20 @@ describe('Query', () => {
expect(result[0].customProperty).to.equal(entity1.customProperty);
expect(result[1].customProperty).to.equal(entity2.customProperty);
});

it('should put docs with null values for sort field at the beginning of the list', () => {

This comment has been minimized.

Copy link
@georgiwe

georgiwe Jan 16, 2018

Contributor

Maybe add a test for descending() too?

@thomasconner

This comment has been minimized.

Copy link
Contributor Author

commented Jan 24, 2018

Changes

  • Changes sorting to return the same result as the server for null and undefined values
const query = new Query().ascending('_id');
// query.fields = ['customProperty'];
const result = query.process([entity4, entity1, entity3, entity2]);
console.log(result);

This comment has been minimized.

Copy link
@georgiwe

georgiwe Jan 25, 2018

Contributor

This should probably be removed. And what about the commented line above?

@thomasconner

This comment has been minimized.

Copy link
Contributor Author

commented Jan 25, 2018

Tests

  • Remove console.log statement and uncommented the line
@thomasconner

This comment has been minimized.

Copy link
Contributor Author

commented Jan 25, 2018

I think all changes requested have been made and this should be ready to go.

@thomasconner

This comment has been minimized.

Copy link
Contributor Author

commented Jan 25, 2018

@tejasranade Can you review this and approve it if everything looks good?

@thomasconner thomasconner merged commit d0d45c1 into master Jan 26, 2018

1 check passed

continuous-integration/travis-ci/push The Travis CI build passed
Details

@thomasconner thomasconner deleted the MLIBZ-2156_Query_Sorting_Fix branch Jan 26, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.