Support for Aggregate Queries #4207

dplewis · 2017-09-24T01:53:34Z

Adds a new endpoint for distinct and aggregate queries.

Basic support for Postgres aggregates (WIP).

codecov · 2017-09-24T02:07:23Z

Codecov Report

Merging #4207 into master will increase coverage by 0.12%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #4207      +/-   ##
==========================================
+ Coverage    92.5%   92.63%   +0.12%     
==========================================
  Files         118      119       +1     
  Lines        8256     8400     +144     
==========================================
+ Hits         7637     7781     +144     
  Misses        619      619

Impacted Files	Coverage Δ
src/RestQuery.js	`95.47% <ø> (ø)`	⬆️
src/Routers/AggregateRouter.js	`100% <100%> (ø)`
src/Adapters/Storage/Mongo/MongoCollection.js	`97.67% <100%> (+0.17%)`	⬆️
src/Adapters/Storage/Mongo/MongoStorageAdapter.js	`93.33% <100%> (+0.37%)`	⬆️
...dapters/Storage/Postgres/PostgresStorageAdapter.js	`96.84% <100%> (+0.33%)`	⬆️
src/Controllers/DatabaseController.js	`94.76% <100%> (+0.08%)`	⬆️
src/ParseServer.js	`94.61% <100%> (+0.04%)`	⬆️
src/RestWrite.js	`93.39% <0%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 842343a...9c866c7. Read the comment docs.

dplewis · 2017-09-24T17:02:06Z

@flovilmart Can you think of any other tests? For Postgres we can add more functionality as it is requested. Also the pipeline is ordered so we don't have to worry about that.

flovilmart

Tests look great, a few nits. I’ll do a larger review later tonight! Awesome job man!

flovilmart · 2017-09-24T17:04:47Z

spec/ParseQuery.Aggregate.spec.js

+  it('group sum query', (done) => {
+    const options = Object.assign({}, masterKeyOptions, {
+      body: {
+        group: { _id: null, total: { $sum: '$score' } },


_id should be transformed (from objectId), which would be true for all keys passed here :)

So group: { objectId: null, total: { $sum: '$score' } }

And it should return [ { objectId: null, total: ... } ] instead of [ { _id: null, total: ... } ]

and group: { objectId: '$name', total: { $sum: '$score' } }

Should return [ { name: ..., total: ... } ]

Yep, in and out, those should not be the internal storage columns but the externals

Should an error be thrown if _id is used?

Yep, most probably!

flovilmart · 2017-09-24T18:48:48Z

src/Adapters/Storage/Postgres/PostgresStorageAdapter.js

@@ -1499,6 +1499,14 @@ export class PostgresStorageAdapter {
      if (countField) {
        results[0][countField] = parseInt(results[0][countField], 10);
      }
+      results.forEach(result => {
+        if (result.hasOwnProperty('_id')) {


Can’t we use the usual transformers here?

I was just about to change it

I don't think we can use the usual transformers like buildWhereClause. We might be able to reuse skip, limit, sort that is in find(). It not complete yet but I could try to refactor it in the second iteration

benishak · 2017-10-01T01:53:08Z

@dplewis great job!
Please add CLP Validation for these operations, default to MasterKey maybe?

You could also add a config attribute for the ALLOWED_KEYS

dplewis · 2017-10-01T02:35:55Z

@benishak Thanks, I don't know much about class level permissions. Are CLP needed here or are they overriden when you use masterKey which is required here?

flovilmart · 2017-10-01T02:38:19Z

CLP don’t really matter for that iteration, as all endpoints are protected by masterKey. It seems unsafe to allow any call to the aggregation pipeline without a masterKey.

benishak · 2017-10-01T03:03:06Z

yeah I fully agree, default behavior should be masterkey only, but I imagine there could be also use cases where non master user would need to run such query like Data analytics dashboards, CRM ... etc. Therefore I thought to put some flexibility but make sure that the default is masterkey only.

dplewis · 2017-10-06T17:22:37Z

@vitaly-t more bad habits, can you look this over?

vitaly-t · 2017-10-06T17:29:52Z

@dplewis same story, in terms of the connections usage it all seems fine, but in terms of using ES6 strings to format queries - it is all over the place :)

montymxb · 2017-11-04T19:14:17Z

spec/ParseQuery.Aggregate.spec.js

+      .then((resp) => {
+        expect(resp.results[0].hasOwnProperty('objectId')).toBe(true);
+        expect(resp.results[0].objectId).toBe(null);
+        expect(resp.results[0].total).toBe(50);


Just curious what happens if one of the score values happens to be unset/null? I'm assuming it just omits it but would be nice to know for sure.

Just tried. If unset/null it gets ignored

montymxb · 2017-11-04T19:33:30Z

spec/ParseQuery.Aggregate.spec.js

+      }).catch(done.fail);
+  });
+
+  it('sort ascending query', (done) => {


~~This looks like it duplicates functionality we already have in standard queries, is this needed for something in particular where the original won't do?~~ Read through the rest, nvm this comment.

As the user builds their aggregate queries they have the option to limit, sort, skip. This test is also there for Postgres support

Yeah I realized that right after I noted this, you're good here.

montymxb · 2017-11-04T19:36:32Z

spec/ParseQuery.Aggregate.spec.js

+      }).catch(done.fail);
+  });
+
+  it('distint query with where', (done) => {


typo on distint

montymxb · 2017-11-04T19:36:44Z

spec/ParseQuery.Aggregate.spec.js

+      }).catch(done.fail);
+  });
+
+  it('distint query with where string', (done) => {


typo on distint again

montymxb · 2017-11-04T19:50:49Z

src/Adapters/Storage/Postgres/PostgresStorageAdapter.js

@@ -1366,6 +1370,140 @@ export class PostgresStorageAdapter {
    });
  }

+  distinct(className, schema, query, fieldName) {
+    debug('distinct', className, query);


~~Just curious what's the debug mark here for?~~ Nvm, logger.

Passing PARSE_SERVER_LOG_LEVEL=debug

It really helps for testing PG

montymxb · 2017-11-04T19:59:01Z

src/Adapters/Storage/Postgres/PostgresStorageAdapter.js

+    debug('distinct', className, query);
+    let field = fieldName;
+    let column = fieldName;
+    if (fieldName.indexOf('.') >= 0) {


Although this will work the value won't ever be 0, just -1 or > 1. Doesn't mean this needs to be changed, but it could also be >= 1 for be more specific about what's expected.

const str = '.'; str.indexOf('.') will return 0;

It will never happen but it covers all cases

montymxb · 2017-11-04T20:01:43Z

src/Adapters/Storage/Postgres/PostgresStorageAdapter.js

+        if (fieldName.indexOf('.') === -1) {
+          return results.map(object => object[field]);
+        }
+        const child = fieldName.split('.')[1];


If I pass a field name like myField. I think this would bug out. Can you check on/add a case to look at nested behavior where the dot is missing following text?

Additionally something like .myField should be invalid. I'm not sure if they are currently prevented though, if they aren't they should probably be screened ahead.

Wouldn't myfield. be an invalid field name?

Yep, just covering bases. Took a look, this is good too.

montymxb · 2017-11-04T20:06:32Z

src/Adapters/Storage/Postgres/PostgresStorageAdapter.js

+          if (sort[key] === 1) {
+            return `"${key}" ASC`;
+          }
+          return `"${key}" DESC`;


Perhaps a bit silly, but you could mark sort with a value of 2 or greater and it would technically fall into the -1 descending category.

montymxb · 2017-11-04T20:10:10Z

src/Routers/AggregateRouter.js

+        body[key]._id = body[key].objectId;
+        delete body[key].objectId;
+      }
+      pipeline.push({ [`$${key}`]: body[key] });


$$? Double checking that this isn't just a working typo...

Scratch this, you're good here as well 👍

montymxb

Changes look good, just a couple typos from what I could tell.

montymxb · 2017-11-07T05:40:12Z

@flovilmart as it stands I think this is ready, but I want to have a second opinion before we go ahead and merge it in.

dplewis · 2017-11-12T18:58:39Z

@flovilmart can we get this one in?

flovilmart · 2017-11-12T19:00:09Z

Yes, let’s go!

montymxb · 2017-11-13T03:27:03Z

@dplewis just a quick thing, looks like we still need to properly clear out _ prefixed values in the return for aggregation, particularly on the _User class. Seeing things like _created_at and such in the results. Need to take care of that before we can consider the next release. @flovilmart letting you know as well.

ranhsd · 2018-03-25T12:36:43Z

Hi @dplewis there are some docs on how to use it? What about the client SDK's? for iOS / Android ?

thanks,
Ran.

dplewis · 2018-03-25T13:44:53Z

@ranhsd To use this master key is required. iOS and Android cant use master key.

ranhsd · 2018-03-25T14:34:27Z

Hi @dplewis yes just noticed that. so I think we need to mention there that it can be achieved via cloud code.

* Support for Aggregate Queries * improve pg and coverage * Mongo 3.4 aggregates and tests * replace _id with objectId * improve tests for objectId * project with group query * typo

Support for Aggregate Queries

9ef32c7

dplewis added 2 commits September 23, 2017 22:21

improve pg and coverage

a138d86

Mongo 3.4 aggregates and tests

a04ede9

flovilmart suggested changes Sep 24, 2017

View reviewed changes

dplewis added 2 commits September 24, 2017 13:04

replace _id with objectId

8b5e858

improve tests for objectId

ea3ec59

flovilmart reviewed Sep 24, 2017

View reviewed changes

project with group query

6b7b2ca

dplewis mentioned this pull request Sep 28, 2017

Support for Aggregate Queries parse-community/parse-php-sdk#355

Merged

dplewis mentioned this pull request Oct 1, 2017

Support for Aggregate Queries parse-community/Parse-SDK-JS#485

Merged

Merge branch 'master' into aggregate

a8d1b24

dplewis requested review from montymxb and flovilmart November 4, 2017 18:40

montymxb reviewed Nov 4, 2017

View reviewed changes

spec/ParseQuery.Aggregate.spec.js Outdated

}).catch(done.fail);

});

it('distint query with where', (done) => {

Copy link

Contributor

montymxb Nov 4, 2017

Choose a reason for hiding this comment

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

typo on distint

montymxb reviewed Nov 4, 2017

View reviewed changes

typo

9c866c7

montymxb reviewed Nov 4, 2017

View reviewed changes

flovilmart merged commit 7223add into parse-community:master Nov 12, 2017

dplewis mentioned this pull request Nov 12, 2017

Find the distinct values for a specified column #2238

Closed

dplewis deleted the aggregate branch November 15, 2017 00:50

cbaker6 mentioned this pull request Jun 27, 2023

perf: Use aggregation pipeline with $graphLookup for recursive role and auth queries #8563

Open

4 tasks

Support for Aggregate Queries #4207

Support for Aggregate Queries #4207

Conversation

dplewis commented Sep 24, 2017 • edited Loading

codecov bot commented Sep 24, 2017 • edited Loading

Codecov Report

dplewis commented Sep 24, 2017

flovilmart left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dplewis Sep 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benishak commented Oct 1, 2017

dplewis commented Oct 1, 2017

flovilmart commented Oct 1, 2017

benishak commented Oct 1, 2017

dplewis commented Oct 6, 2017

vitaly-t commented Oct 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

montymxb Nov 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

montymxb Nov 4, 2017 • edited Loading

Choose a reason for hiding this comment

dplewis Nov 4, 2017 • edited Loading

Choose a reason for hiding this comment

montymxb Nov 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

montymxb Nov 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

montymxb Nov 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

montymxb left a comment

Choose a reason for hiding this comment

montymxb commented Nov 7, 2017

dplewis commented Nov 12, 2017

flovilmart commented Nov 12, 2017

montymxb commented Nov 13, 2017

ranhsd commented Mar 25, 2018

dplewis commented Mar 25, 2018

ranhsd commented Mar 25, 2018

dplewis commented Sep 24, 2017 •

edited

Loading

codecov bot commented Sep 24, 2017 •

edited

Loading

dplewis Sep 24, 2017 •

edited

Loading

montymxb Nov 4, 2017 •

edited

Loading

montymxb Nov 4, 2017 •

edited

Loading

dplewis Nov 4, 2017 •

edited

Loading

montymxb Nov 4, 2017 •

edited

Loading

montymxb Nov 4, 2017 •

edited

Loading

montymxb Nov 4, 2017 •

edited

Loading