balderdashy/waterline-adapter-tests

why groupBy with no calculations has to raise an error ?

postsql opened this issue · 5 comments

queries/modifiers/groupBy.query.modifier.test.js has this test :

it('should error if not given any calculations to do', function(done) {
      User.find({
        groupBy: ['type']
      }, function(err, summed) {
        assert(err);
        done();
      });
    });

Why is this defined to raise an error
IMO this is simple DISTINCT query and not an error.

SELECT "type" FROM "user" GROUP BY "type";

or

SELECT DISTINCT "type" FROM "user";

@postsql good point!

groupBy / aggregations API in waterline is still experimental (not supported by Redis, for example, so we have to make sure we can gracefully handle that case). If you'd be down to do a PR changing the test, I'm down to merge. We really ought to expose a separate "aggregatable" interface so the tests can be switched on/off for adapters who choose to support it. Then, at some point, we can combine it w/ waterline-criteria + the query integrator built in to waterline core to offer naive support automatically (like we do for joins) for adapters which don't explicitly implement the methods.

Thanks!

Any updates on this?

I think this is primarily a waterline-sequel thing: https://github.com/balderdashy/waterline-sequel/blob/4c084466d2af3ab5c64d98327fd4b65c7581200e/sequel/select.js#L127-L130

I don't believe it's enforced in waterline core.

Good catch @devinivy, I think we can close this for now then.

I've moved this over to balderdashy/waterline-sequel#47 for now. @dmarcelino we should think about how the tests should deal with adapters that do/do not support groupBy and especially aggregates. And on the waterline core end, will that functionality have to go into the integrator? Any pertinent issues should be split-off from this one. Thanks for bringing this up @postsql !