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

Implement limit method for MongoDB driver #499

Merged
merged 1 commit into from
Feb 6, 2014

Conversation

nazriel
Copy link
Contributor

@nazriel nazriel commented Feb 6, 2014

My first naive implementation.
Seems to work good but I am still new to vibe.d internals so I am not sure if it's the best approach.

@s-ludwig
Copy link
Member

s-ludwig commented Feb 6, 2014

If you have the time, I'd like to suggest some improvements. In particular, the amount of returned documents per chunk should also be limited to the given limit and the cursor should be discarded when the limit is reached. I'll make some inline comments for this.

@@ -196,6 +217,10 @@ private class MongoCursorData {
addSpecial("$orderby", order);
}

void limit(size_t count) {
m_limit = count;
}
Copy link
Member

Choose a reason for hiding this comment

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

An additional if (m_nret <= 0 || count < m_nret) m_nret = min(count, 1024); should limit the amount of results per chunk here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I might be wrong but I think it should be:

if (count != m_nret) 
    m_nret = min(count, 1024);

    m_limit = count;

Because according to MongoDB docs:
"A limit() value of 0 (e.g. “.limit(0)”) is equivalent to setting no limit."

So such code:

db["users"].find().sort(Bson(["title": Bson(-1)])).limit(3).limit(0);

Should reset limit and also rising limit should be possible, for example:

db["paste"].find().sort(Bson(["title": Bson(-1)])).limit(3).limit(10);

@s-ludwig what you think?

Copy link
Member

Choose a reason for hiding this comment

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

I don't know about the last two use cases, I think it's ok and expected if limit... limits (i.e. it can only lower the limit and not raise it). And it should still be possible to specify a chunk size that is lower than the limit somehow, which cannot work if limit always adjusts m_nret.

But you are definitely right about the zero case (note that the if statement in the empty method also needs to be adjusted to cope with m_limit == 0.

@nazriel
Copy link
Contributor Author

nazriel commented Feb 6, 2014

@s-ludwig updated, hope I understood your correctly.

@s-ludwig
Copy link
Member

s-ludwig commented Feb 6, 2014

Looks good, except the zero case, which I would rather handle like this:

if (count > 0) {
    if (m_nret == 0 || m_nret > count)
         m_nret = count;
}

This would be more in line with the tought that each limit call successively limits the output further.

@nazriel
Copy link
Contributor Author

nazriel commented Feb 6, 2014

@s-ludwig updated. Hope I got it correctly this time 👻

@s-ludwig
Copy link
Member

s-ludwig commented Feb 6, 2014

Thanks a lot! Merging now.

s-ludwig added a commit that referenced this pull request Feb 6, 2014
Implement limit method for MongoDB driver
@s-ludwig s-ludwig merged commit 26fd18f into vibe-d:master Feb 6, 2014
s-ludwig added a commit that referenced this pull request Feb 6, 2014
@s-ludwig
Copy link
Member

s-ludwig commented Feb 6, 2014

Just discovered that m_limit was still handled differently. But now everything should be good.

@nazriel
Copy link
Contributor Author

nazriel commented Feb 6, 2014

Yeah I left limit(0) case as "limit-reset" option.
But I think your approach is more consistant so yeah looks great now!

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

Successfully merging this pull request may close these issues.

2 participants