Skip to content

move deunion decision into agg functions#6795

Merged
mccanne merged 3 commits intomainfrom
agg-deunion
Apr 6, 2026
Merged

move deunion decision into agg functions#6795
mccanne merged 3 commits intomainfrom
agg-deunion

Conversation

@mccanne
Copy link
Copy Markdown
Collaborator

@mccanne mccanne commented Apr 4, 2026

This commit changes the calling convention for agg functions so that
values are not deunioned though the behavior depends on the sam vs vam.
In sam, the deunion is pushed into the agg funcs.  In the vam, an agg
func will specify whether or not it wants ripped args, but for now,
we just have an expedient hack to check for the agg func name "fuse"
as this is the only such agg func presently.

@mccanne
Copy link
Copy Markdown
Collaborator Author

mccanne commented Apr 4, 2026

I also changed sam any() to ignore input nulls as is done by the vam any().

Comment thread runtime/vam/op/aggregate/scalar.go Outdated
}
}
vector.Apply(true, s.consume, vals...)
vector.Apply(false, s.consume, vals...)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The main call to apply for vectors is in runtime/vam/op/aggregate/aggregate.go line 84. This one is kind of tricky because you probably want to deunion the key values while keeping aggregate values unioned. We don't really have a way of doing this.

Comment thread runtime/vam/expr/agg/union.go Outdated
}

func (u *union) Consume(vec vector.Any) {
vector.Apply(true, u.consume, vec)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This is going to need to be done the agg functions as well:

  • avg
  • min/max (math reducer)
  • collect
  • collectmap
  • distinct
  • and / or

I'm going to guess that the fuse operator doesn't want this.

This commit changes the calling convention for agg functions so that
values are not deunioned though the behavior depends on the sam vs vam.
In sam, the deunion is pushed into the agg funcs.  In the vam, an agg
func will specify whether or not it wants ripped args, but for now,
we just have an expedient hack to check for the agg func name "fuse"
as this is the only such agg func presently.
@mccanne
Copy link
Copy Markdown
Collaborator Author

mccanne commented Apr 5, 2026

Ok, I've changed the approach and rather than have vam agg not do a rip, it instead wraps vectors for fuse that shouldn't be ripped in vector.NoRip{}. We can generalize this to a parameter on the agg func down the road but I want to get this fusion stuff working so let's hack this in for now.

Comment thread runtime/vam/expr/agg/count.go Outdated
Comment on lines +12 to +22
func (a *count) Consume(vec vector.Any) {
func (c *count) Consume(vec vector.Any) {
vector.Apply(true, c.consume, vec)
}

func (a *count) consume(vecs ...vector.Any) vector.Any {
vec := vecs[0]
if vec.Kind() == vector.KindNull {
return
return vec
}
a.count += int64(vec.Len())
return vec
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I don't think we need these changes now that we just do NoRip for fuse, no?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Meant to take that out!

Comment thread runtime/vam/expr/agg/any.go Outdated
Comment on lines +18 to +29
vector.Apply(true, a.consume, vec)
}

func (a *Any) consume(vecs ...vector.Any) vector.Any {
vec := vecs[0]
if !a.val.IsNull() || vec.Kind() == vector.KindNull {
return
return vec
}
var b scode.Builder
vec.Serialize(&b, 0)
a.val = super.NewValue(vec.Type(), b.Bytes().Body())
return vec
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Also don't think we need these changes now that we have NoRip for fuse only

@mccanne mccanne merged commit 8e75f06 into main Apr 6, 2026
2 checks passed
@mccanne mccanne deleted the agg-deunion branch April 6, 2026 00:49
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.

2 participants