gradoop
gradoop copied to clipboard
[#1544] Make temporal grouping result temporal
Fixes #1544
Description
- Allow custom setting of aggregation results (instead of only property values)
- Add flag to set time in temporal aggregation functions
- Add
temporalGrouping
with default time aggregation (min/max valid times) <- @ChrizZz110 @galpha any opinions?
Related Issue
#1544
Motivation and Context
How Has This Been Tested?
Types of Changes
- [ ] Bug fix (non-breaking change which fixes an issue)
- [X] New feature (non-breaking change which adds functionality)
- [X] Breaking change (fix or feature that would cause existing functionality to change)
Checklist:
- [X] My code follows the code style of this project.
- [ ] I have updated the documentation accordingly (if necessary).
- [X] I have added tests to cover my changes.
- [X] All new and existing tests passed.
- [X] I ran a spell checker.
First of all - I'm happy that you took that issue, since I think this is really necessary. According to the realization I have to think about when I read your code ... First thin I noticed is the transaction time, which should not be changed. The transaction times of grouped elements should never changed. tx-from is set, when the super vertex/edge is created by gradoop. I know, this sounds unfamiliar, but this is the semantic of tx-times - they are immutable.
The transaction time isn't changed in temporalGrouping
. I only added the possibility to change it with aggregations. I thought it would be good to have, since you can already read and aggregate it. I can remove this if you want.
Any special reason for changing the api of the aggregation class constructors? All systems that use these aggregate classes have to change the signature then
- Keeping the constructor overload prettier (required parameters first, optional ones last)
- Following the api of the non-temporal aggregation functions
Convinced.
Done.