Skip to content

[yarpl] Reduce operator for Flowable - #411

Merged
lehecka merged 14 commits into
rsocket:masterfrom
phoad:yarpl_reduce
May 19, 2017
Merged

[yarpl] Reduce operator for Flowable#411
lehecka merged 14 commits into
rsocket:masterfrom
phoad:yarpl_reduce

Conversation

@phoad

Copy link
Copy Markdown
Member

Adding Reduce operator for Flowable

Flowables::range(0, 10)
->reduce([] (int64_t acc, int64_t x) { return acc + x; })
->subscribe(mySubscriber);

and have mySubscriber get sent a single result of 45 and then get completed.

@alexmalyshev

Copy link
Copy Markdown
Contributor

Like filter(), this version of flowable::ReduceOperator doesn't have request(n) semantics. A way to see this would be to do Flowables::justN<int64_t>({1, 2, 3})->reduce([] (int a, int b) { return a + b; }); If I subscribe to that flowable with a subscriber that does request(1), then my subscriber should get back onNext(6); onComplete();

Right now, your implementation will forward that request(1) to the justN(), which will send through 1. But you will never get the rest of the items. reduce() needs its upstream to finish sending through all of its items before it can send through its single item and then complete.

Question for @benjchristensen, would you expect Flowables::just<int64_t>(5)->reduce(someFn) to call someFn(0, 5)? I think single-item reduce should never call the accumulator function, but I'm not actually sure what the desired behavior here is... (short of renaming to foldLeft and adding an initial argument).

@alexmalyshevalexmalyshev left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Comments above

@phoad

Copy link
Copy Markdown
MemberAuthor

I believe even with one item, if we reduce we should call the reduce function. Think like reduce function is taking the negative of the value, namely subtracting it from the initial value. Then we will return wrong value if we do not call the function at all..

Flowables::just<int64_t>(5)->reduce([] (int a, int b) { return a - b; });

Result should be -5, not 5.

@phoad

Copy link
Copy Markdown
MemberAuthor

Furthermore, I believe we need a default initial value for the "a" parameter. It might be correct to use the default of the integral values (for +/- operators), but if we think about reducing with (*, /) operators we will always get 0.. We might also end up with a double/float/long number as the result of the reduce, but we can specify this by using these values as type of the parameter "a".

I wanted to ask before doing further..

@alexmalyshev

alexmalyshev commented May 12, 2017

Copy link
Copy Markdown
Contributor

Think like reduce function is taking the negative of the value, namely subtracting it from the initial value. Then we will return wrong value if we do not call the function at all..

Flowables::just<int64_t>(5)->reduce([] (int a, int b) { return a - b; });

Result should be -5, not 5.

I don't agree here. reduce() is about combining N items together, pairwise. If you only have one item, then there's nothing to combine it with (unless you have an initial value of course, but that sounds like a different foldLeft() operator to me). I think using integer subtraction is a bad example because it's not the right use case for reduce(), you wouldn't combine items by subtracting them. Operators that are more about "combining elements", are things like addition, multiplication, string concatenation. All things that work just fine if you don't run the argument function on single-item streams.

Furthermore, I believe we need a default initial value for the "a" parameter

It might just be the case that we want a foldLeft() operator that does have an initial value, separate from reduce().

@phoad

Copy link
Copy Markdown
MemberAuthor

So, you mean, the initial value better to be the first item in the inputs. Nice!
If we have only one value, it is the initial value, which is the result value, that we do not perform any operation on.
If we have multiple values and we apply multiplication, as the initial value for int is 0, as we use the first item in the inputs as the initial value, we will not end up with 0 at the end for the multiplication. Perfect! Huh.

@phoad

phoad commented May 12, 2017

Copy link
Copy Markdown
MemberAuthor

The definition of the reduce is a bit different: http://reactivex.io/documentation/operators/reduce.html

"The Reduce operator applies a function to the first item emitted by the source Observable"

I will update the code according to your explanations. Thanks Alex!
http://stackoverflow.com/questions/7764197/difference-between-foldleft-and-reduceleft-in-scala

…tion and value. Reduce operator's implementation now matches with the desired behavior. TODO - I will add unit tests for Reduce Observable.
typename U,
typename D,
typename F,
typename = typename std::enable_if<std::is_trivially_assignable<D, U>::value>,

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I might add a "#if version" check in here so it can build for older gcc versions. Is it an appreciated way in the team for such features that are only available in the new compiler versions.

subscriber->onNext(acc_);
callSuperOnComplete();
} else {
callSuperOnError(std::make_exception_ptr(std::runtime_error("Upstream has no value")));

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I thought again and perhaps we should not throw at all and just call complete without calling onNext.
I will do this update along with implementing Observable Reduce Operator unit tests.

@alexmalyshev

Copy link
Copy Markdown
Contributor

LGTM, but conflicts need fixing :(

@benjchristensen

Copy link
Copy Markdown
Contributor

Fixed the conflicts, but there is an asan failure that I don't quite understand yet. Doesn't seem related to this change, but need to understand the cause.

Comment threadtck-test/server.cpp
std::make_unique<FramedDuplexConnection>(
std::move(connection), inlineExecutor());
std::unique_ptr<RequestHandler> requestHandler =
std::make_unique<ServerRequestHandler>();

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Sorry, these updates should not go to the review. I am reverting these lines.

@phoad

Copy link
Copy Markdown
MemberAuthor

@alexmalyshev, @lehecka can you help in checking the update in?

"Only those with write access to this repository can merge pull requests."

@lehecka
lehecka merged commit e0867a9 into rsocket:masterMay 19, 2017
@phoad
phoad deleted the yarpl_reduce branch June 20, 2017 19:16
Sign up for freeto 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.

4 participants

@phoad@alexmalyshev@benjchristensen@lehecka