Skip to content

Conversation

@wojcikstefan
Copy link
Member

Closes #569

@MRigal here's the implementation of sum & average using the aggregation framework. I haven't tested it in a sharded environment, but if it works, I believe it could fully replace the slower, map-reduce-based BaseQuerySet.sum and BaseQuerySet.average.

Review on Reviewable

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.02%) to 91.17% when pulling 0869bdf on elasticsales:aggregate-sum-and-avg into 6d3bc43 on MongoEngine:master.

@landscape-bot
Copy link

Code Health
Repository health decreased by 0.04% when pulling 0869bdf on elasticsales:aggregate-sum-and-avg into 6d3bc43 on MongoEngine:master.

@wojcikstefan
Copy link
Member Author

Thanks @thedrow, the latest commit fixes it.

@landscape-bot
Copy link

Code Health
Repository health decreased by 0.05% when pulling ce8f960 on elasticsales:aggregate-sum-and-avg into 6d3bc43 on MongoEngine:master.

@MRigal
Copy link
Member

MRigal commented Jun 11, 2015

Thanks @wojcikstefan !
You will find me very picky, but for readability and to minimise the "PyMongo3-related code conditionality", could you eventually rewrite the followings:

   if IS_PYMONGO_3:
        result = list(result)
        if result:
            return result[0]['total']
    else:
        if result['result']:
            return result['result'][0]['total']
    return 0

to:

   if IS_PYMONGO_3:
        result = list(result)
    else:
        result = result.get('result')
    if result:
        return result[0]['total']
    return 0

Thanks !

@MRigal
Copy link
Member

MRigal commented Jun 11, 2015

To all:
should we also add some DeprecationWarning to the classical sum() and average() methods?

@MRigal
Copy link
Member

MRigal commented Jun 11, 2015

One more point before merge: @wojcikstefan please add this to the changelog !

@wojcikstefan
Copy link
Member Author

@MRigal thanks - glad you pay attention! I added a changelog entry and simplified the code.

Not sure about deprecating sum and average. Ideally, we would just drop the aggregate_ prefix from the methods I implemented here. In such case, the methods themselves wouldn't be deprecated, but their original contents would.

@landscape-bot
Copy link

Code Health
Repository health decreased by 0.04% when pulling 4e0b54d on elasticsales:aggregate-sum-and-avg into 6d3bc43 on MongoEngine:master.

@MRigal
Copy link
Member

MRigal commented Jun 18, 2015

@thedrow @DavidBord why the fuck did Travis didn't start a build Job here???!?!? I have no clue how to start one now...

@MRigal
Copy link
Member

MRigal commented Jun 18, 2015

@wojcikstefan I would have eventually solved the conflict by hand, but since the tests didn't started, could you please rebase the branch and push, with the hope it will launch a test run on travis...

@wojcikstefan wojcikstefan force-pushed the aggregate-sum-and-avg branch from 4e0b54d to b7ef82c Compare June 18, 2015 18:02
@wojcikstefan
Copy link
Member Author

@MRigal I rebased against the latest master. It seems to have triggered a Travis build.

@landscape-bot
Copy link

Code Health
Repository health decreased by 0.04% when pulling b7ef82c on elasticsales:aggregate-sum-and-avg into 4d5200c on MongoEngine:master.

MRigal added a commit that referenced this pull request Jun 18, 2015
@MRigal MRigal merged commit 5fa5284 into MongoEngine:master Jun 18, 2015
@MRigal
Copy link
Member

MRigal commented Jun 18, 2015

Thanks @wojcikstefan !

@lafrech
Copy link
Member

lafrech commented Apr 28, 2016

Thanks @wojcikstefan for this contribution. I found those methods by accident while looking in the code. They are not documented in the doc.

@MongoEngine/mongoengine, what would make more sense? Adding a note to the docs? Replace old sum() and average() and drop old map/reduce methods?

Are there cases not covered by those aggregate_* methods? (Old MongoDB, old pymongo, specific configuration,...)

@wojcikstefan
Copy link
Member Author

I think it would make sense to replace the original methods with the new ones. I believe it's supported by all versions of MongoDB >= 2.2 when the aggregation framework was introduced. Also, the less raw JS code in MongoEngine, the better IMHO :)

Curious what @thedrow thinks about this!

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants