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

Convert pair returned by first/last aggregators to number (#5566) #5699

Closed
wants to merge 1 commit into from

Conversation

kskalski
Copy link

No description provided.

@jihoonson
Copy link
Contributor

jihoonson commented Apr 30, 2018

@kskalski, thank you for the contribution!

I believe this PR would be useful for some use cases, but existing implementation is also useful as well. So, I think it would be better to add an option to First/LastAggregatorFactory to get only time/value or both. The default should be both as it does.

@kskalski
Copy link
Author

Yes, I guess it would be a better fix to get the right value when processing is done. Actually First/LastAggregator already have methods for fetching just the value or timestamp, however I don't know the code-base enough to find out when/where they should be called.
I'm not sure where the values surfaced in the exception are actually produced, both *AggregatorFactory and Aggregator have methods, which return SerializablePair. My guess is that somewhere the code calls Aggregator::get instead of Aggregator::getDouble, which for First/Last Aggregator returns a pair. Unfortunately the exception I get is already past that phase, values are already produced and there is only symptomatic way to deal with it.

@jihoonson
Copy link
Contributor

@kskalski sorry for the delayed review. I'm closing and reopening this PR to trigger Travis build. The link is not available now.

@jihoonson jihoonson closed this Jul 27, 2018
@jihoonson jihoonson reopened this Jul 27, 2018
@jihoonson
Copy link
Contributor

Hi @kskalski, please check my comment here: #5566 (comment). I think this is not a bug, but we need a better doc for this.

I'm closing this PR shortly.

@jihoonson jihoonson closed this Jul 28, 2018
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.

None yet

2 participants