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

Support rollup and integer states state/timeline aggs #640

Merged
merged 8 commits into from
Dec 6, 2022

Conversation

syvb
Copy link
Member

@syvb syvb commented Nov 29, 2022

  • Adds rollup for state_agg and timeline_agg.
  • Supports integer states for state_agg and timeline_agg. An aggregate can have integer states or string states, but not both. For a few functions this would mean having two functions with the same argument types but differing return types, so in that case I made a separate function for integer-valued aggregates (e.g. state_int_timeline).

Ideally this should have been two separate PRs, but my changes to rollup and the rest of the state_agg code have become intertwined.

(this should be merged after #636)

@syvb syvb force-pushed the sv/state_agg-rollup branch from 9d0ed0f to 41ff3dd Compare November 29, 2022 14:25
@syvb syvb force-pushed the sv/state_agg-rollup branch 2 times, most recently from 8171f41 to 1e97905 Compare November 30, 2022 20:18
@syvb syvb changed the title Support rolling up state/timeline aggs Support rollup and integer states state/timeline aggs Dec 1, 2022
@syvb syvb force-pushed the sv/state_agg-rollup branch from 4832e02 to eda09f5 Compare December 1, 2022 20:11
@syvb syvb marked this pull request as ready for review December 1, 2022 20:27
@syvb syvb mentioned this pull request Dec 2, 2022
@syvb syvb force-pushed the sv/state_agg-rollup branch 2 times, most recently from d2e6721 to ad5fd1d Compare December 6, 2022 00:59
Copy link
Contributor

@WireBaron WireBaron left a comment

Choose a reason for hiding this comment

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

I believe the inputs to the trans function all below to the aggregate context so shouldn't need to be cloned.

@syvb syvb force-pushed the sv/state_agg-more branch from 4997c37 to 327e22e Compare December 6, 2022 14:53
@syvb syvb force-pushed the sv/state_agg-rollup branch 2 times, most recently from 1aa764b to 0e06731 Compare December 6, 2022 19:06
@syvb syvb force-pushed the sv/state_agg-more branch from 327e22e to 33dc255 Compare December 6, 2022 21:53
@syvb syvb force-pushed the sv/state_agg-rollup branch from 0e06731 to 9aaeb3c Compare December 6, 2022 21:58
@syvb syvb force-pushed the sv/state_agg-rollup branch from 9aaeb3c to 35b1fe3 Compare December 6, 2022 22:06
@bors bors bot deleted the branch main December 6, 2022 22:18
@bors bors bot closed this Dec 6, 2022
@syvb syvb reopened this Dec 6, 2022
@syvb syvb changed the base branch from sv/state_agg-more to main December 6, 2022 22:19
@syvb
Copy link
Member Author

syvb commented Dec 6, 2022

bors r+

@bors
Copy link
Contributor

bors bot commented Dec 6, 2022

Build succeeded:

@bors bors bot merged commit 6347638 into main Dec 6, 2022
@bors bors bot deleted the sv/state_agg-rollup branch December 6, 2022 22:30
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.

3 participants