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

Added range for MongoCursor #172

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
2 participants
@mihails-strasuns
Contributor

mihails-strasuns commented Jan 27, 2013

One of enhancement I have long wanted - working with MongoCursor with InputRange interface. Allows for more functional way to process database data into final output with all std.algorithm & friends help.

Motivating example from tests:

coll.insert(["key1" : "value1"]);
coll.insert(["key1" : "value2"]);
coll.insert(["key1" : "value2"]);
auto data1 = coll.find(["key1" : "value1"]);
auto data2 = coll.find(["key1" : "value2"]);

import std.range, std.algorithm;
auto converted = map!
    (a => a[0].key1.get!string() ~ a[1].key1.get!string())(
        zip(data1.range, data2.range)
    );
assert(!converted.empty);
assert(converted.front == "value1value2");
@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Jan 28, 2013

Member

Neat, always had that in mind but never had the immediate need to implement it.

One question, though - it seems to me that the range interface would fit nicely into MongoCursor itself. The opApply does not really imply that iteration works only once and the input range interface would be clearer there, I think.

Member

s-ludwig commented Jan 28, 2013

Neat, always had that in mind but never had the immediate need to implement it.

One question, though - it seems to me that the range interface would fit nicely into MongoCursor itself. The opApply does not really imply that iteration works only once and the input range interface would be clearer there, I think.

@mihails-strasuns

This comment has been minimized.

Show comment
Hide comment
@mihails-strasuns

mihails-strasuns Jan 28, 2013

Contributor

It was my first naive approach but then second opApply overload functionality is lost : foreach(key, value; cursor).
I don't know how to do same via range interface.

Contributor

mihails-strasuns commented Jan 28, 2013

It was my first naive approach but then second opApply overload functionality is lost : foreach(key, value; cursor).
I don't know how to do same via range interface.

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Jan 28, 2013

Member

Can't we just keep the second opApply in addition to the range interface?

Member

s-ludwig commented Jan 28, 2013

Can't we just keep the second opApply in addition to the range interface?

@mihails-strasuns

This comment has been minimized.

Show comment
Hide comment
@mihails-strasuns

mihails-strasuns Jan 28, 2013

Contributor

Tried this too, got compilation error about not being able to select foreach signature. Was not in mood to get deep into specs so reverted to this solution. There is one advantage of keeping range separate though - usual phobos approach of separating containers from ranges. I.e. later we may change the implementation so that MongoCursor.range iteration won't consume items from inner struct and change it to ForwardRange.

Contributor

mihails-strasuns commented Jan 28, 2013

Tried this too, got compilation error about not being able to select foreach signature. Was not in mood to get deep into specs so reverted to this solution. There is one advantage of keeping range separate though - usual phobos approach of separating containers from ranges. I.e. later we may change the implementation so that MongoCursor.range iteration won't consume items from inner struct and change it to ForwardRange.

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Jan 28, 2013

Member

But since MongoCursor itself consumes items during iteration (which surely is a good default regarding efficiency), it's more a range/range separation instead of a container/range. If a container/multiple iteration is needed, it think cursor.array() is an obvious solution that should be good enough for most cases.

It's just that only coll.find().map!(...) is much nicer than to always have to insert another range() call... I could also take look and see how range+index opApply can be made to work together - that just shouldn't be the only reason to separate the functionality into two basically equivalent entities. The spec seems to be a bit short on this.. range iteration isn't even mentioned in the foreach section, AFAICS.

Member

s-ludwig commented Jan 28, 2013

But since MongoCursor itself consumes items during iteration (which surely is a good default regarding efficiency), it's more a range/range separation instead of a container/range. If a container/multiple iteration is needed, it think cursor.array() is an obvious solution that should be good enough for most cases.

It's just that only coll.find().map!(...) is much nicer than to always have to insert another range() call... I could also take look and see how range+index opApply can be made to work together - that just shouldn't be the only reason to separate the functionality into two basically equivalent entities. The spec seems to be a bit short on this.. range iteration isn't even mentioned in the foreach section, AFAICS.

@mihails-strasuns

This comment has been minimized.

Show comment
Hide comment
@mihails-strasuns

mihails-strasuns Jan 28, 2013

Contributor

@s-ludwig
It is, there is a separate section for range foreach in dlang.org documentation: http://dlang.org/statement.html#ForeachRangeStatement But I can't find one on foreach signature resolution order.

Anyway, I am perfectly fine with changing this back to MongoCursor as a range - just a bit busy to find out right now. Let me know if you will be first one to find out.

Contributor

mihails-strasuns commented Jan 28, 2013

@s-ludwig
It is, there is a separate section for range foreach in dlang.org documentation: http://dlang.org/statement.html#ForeachRangeStatement But I can't find one on foreach signature resolution order.

Anyway, I am perfectly fine with changing this back to MongoCursor as a range - just a bit busy to find out right now. Let me know if you will be first one to find out.

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Jan 28, 2013

Member

But that section is just about n .. m style "ranges" ;)

I'm also still busy with other things, but should have some time later today. I'll post as soon as I have something.

Member

s-ludwig commented Jan 28, 2013

But that section is just about n .. m style "ranges" ;)

I'm also still busy with other things, but should have some time later today. I'll post as soon as I have something.

@mihails-strasuns

This comment has been minimized.

Show comment
Hide comment
@mihails-strasuns

mihails-strasuns Jan 28, 2013

Contributor

Hah, that is what you get when start coding 12h a day :)
Never mind, it was just my memory glitch. Looking forward to any updates.

Contributor

mihails-strasuns commented Jan 28, 2013

Hah, that is what you get when start coding 12h a day :)
Never mind, it was just my memory glitch. Looking forward to any updates.

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Jan 29, 2013

Member

Okay... it seems that once there is any opApply, the range interface is not taken into account for iteration anymore. The solution seems as simple as adding the range interface to MongoCursor and leave both opApplys there.

Do you mind if I just commit my changes and close the pull? Seems to be the fastest way.

Member

s-ludwig commented Jan 29, 2013

Okay... it seems that once there is any opApply, the range interface is not taken into account for iteration anymore. The solution seems as simple as adding the range interface to MongoCursor and leave both opApplys there.

Do you mind if I just commit my changes and close the pull? Seems to be the fastest way.

@mihails-strasuns

This comment has been minimized.

Show comment
Hide comment
@mihails-strasuns

mihails-strasuns Jan 29, 2013

Contributor

Sure, please do. May be worth referencing commit hash in comment here for random wanderers.

Contributor

mihails-strasuns commented Jan 29, 2013

Sure, please do. May be worth referencing commit hash in comment here for random wanderers.

s-ludwig added a commit that referenced this pull request Jan 29, 2013

Added an input range interface to MongoCursor. This is a redo of pull #…
…172.

The range interface is added directly to MongoCursor in the version instead
of introducing an extra range type. Original pull by Михаил Страшун (Dicebot).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment