Support cumulative aggregations #5209
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
lutter
force-pushed
the
lutter/agg-minmax
branch
from
a2a0243
to
ccb992e
Compare
lutter
force-pushed
the
lutter/agg-lifetime
branch
from
cde7c82
to
396e4d6
Compare
incrypto32
approved these changes
Feb 21, 2024
Comment on lines
169
to
259
| Ok(IdType::Bytes) => "max(id::text)::bytea", | ||
| Ok(IdType::String) | Ok(IdType::Int8) => "max(id)", |
There was a problem hiding this comment.
Isn't Int8 the only type one allowed for the ID field in a time series?
There was a problem hiding this comment.
Yes, I think that's a leftover from before I made Int8 mandatory as the id
| Sum => write!(w, "sum(\"{}\")", self.src_column.unwrap().name)?, | ||
| Max => write!(w, "max(\"{}\")", self.src_column.unwrap().name)?, | ||
| Min => write!(w, "min(\"{}\")", self.src_column.unwrap().name)?, | ||
| Sum => write!(w, "sum(\"{}\")", src.unwrap())?, |
There was a problem hiding this comment.
would it be cleaner to take a &SqlName instead of Option and call unwrap from the caller?
There was a problem hiding this comment.
Ignore this was resolved in a later commit than when reviewed
lutter
force-pushed
the
lutter/agg-lifetime
branch
from
396e4d6
to
fc97ac2
Compare
lutter
force-pushed
the
lutter/agg-lifetime
branch
from
fc97ac2
to
190171f
Compare
No actual change in functionality
Instead of having an Option for the argument to accomodate `count` which has no argument, use `id` as a dummy argument for that case.
For immutable entities, creating a BRIN index on (block$, vid) is not very helpful. A normal BTree on just block$ works much better.
lutter
force-pushed
the
lutter/agg-lifetime
branch
from
190171f
to
01be1d1
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR allows aggregates that are cumulative from the beginning of time rather than just within a time interval. That is indicated by declaring the aggregate as
@aggregate(fn: "sum", arg:"amount", cumulative: true)Check the docs in ./docs/aggregation.md for more details.
Note that this PR is on top of PR #5208