gradoop icon indicating copy to clipboard operation
gradoop copied to clipboard

[#1544] Make temporal grouping result temporal

Open timo95 opened this issue 3 years ago • 6 comments

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.

timo95 avatar Jul 28 '21 14:07 timo95

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.

ChrizZz110 avatar Jul 29 '21 07:07 ChrizZz110

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.

timo95 avatar Jul 29 '21 07:07 timo95

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

ChrizZz110 avatar Aug 17 '21 13:08 ChrizZz110

  1. Keeping the constructor overload prettier (required parameters first, optional ones last)
  2. Following the api of the non-temporal aggregation functions

timo95 avatar Aug 17 '21 13:08 timo95

Convinced.

ChrizZz110 avatar Aug 17 '21 13:08 ChrizZz110

Done.

timo95 avatar Sep 06 '21 11:09 timo95