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

feat(storage/reads): add window agg result set #19725

Merged
merged 1 commit into from
Oct 15, 2020

Conversation

fchikwekwe
Copy link
Contributor

@fchikwekwe fchikwekwe commented Oct 12, 2020

Add aggregate_resultset to OSS for mean aggregate pushdowns.

@fchikwekwe fchikwekwe force-pushed the feat/window-agg-res-set branch 3 times, most recently from 860da58 to f7e1bda Compare October 13, 2020 20:10
@fchikwekwe fchikwekwe marked this pull request as ready for review October 13, 2020 20:12
@fchikwekwe fchikwekwe requested review from a team and jpacik and removed request for a team October 13, 2020 20:44
Copy link
Contributor

@jpacik jpacik left a comment

Choose a reason for hiding this comment

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

I'm assuming this is just a straight copy?

@fchikwekwe
Copy link
Contributor Author

@jlapacik no, it wasn’t. It’s mostly copied but I had to make some adjustments to make this work in OSS. I can outline them for your tomorrow if needed.

@jpacik
Copy link
Contributor

jpacik commented Oct 15, 2020

That would be helpful thank you.

request := datatypes.ReadWindowAggregateRequest{
Aggregate: []*datatypes.Aggregate{
{
Type: datatypes.AggregateTypeMean,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since we're porting the mean agg first, these tests use mean.

5000000000000000,
5097600000000001,
},
Values: []int64{100, 55, 256, 83, 99, 124, 1979, 4, 67, 49929, 51000},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There wasn't an equal number of values and timestamps, so I adjusted that here.

Comment on lines +203 to +204
floatArrayCursor := cursor.(cursors.FloatArrayCursor)
floatArray := floatArrayCursor.Next()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since this is using mean, floatArrayCursors are needed instead of integer.

Comment on lines +162 to +164
var itr cursors.CursorIterator
var cur cursors.Cursor
itr, c.itrs = c.itrs[0], c.itrs[1:]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

OSS uses cursor iterator lists. In order to follow that convention here, I have to access the list to get the cursorIterator value.

@@ -244,15 +317,15 @@ func (c {{$type}}) Next() {{$arrayType}} {

{{end}}

type integer{{.Name}}CountArrayCursor struct {
type {{.Name}}CountArrayCursor struct {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was actually a change to make this match the original code a little more. the interger before these types is redundant.

@fchikwekwe
Copy link
Contributor Author

@jlapacik i think those are most of the relevant changes. Thanks

@fchikwekwe fchikwekwe merged commit 447ae59 into master Oct 15, 2020
@fchikwekwe fchikwekwe deleted the feat/window-agg-res-set branch October 15, 2020 14:53
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