malloy icon indicating copy to clipboard operation
malloy copied to clipboard

https://github.com/malloydata/malloy/pull/1325

Open mtoy-googly-moogly opened this issue 1 year ago • 4 comments

Cleaned up and normalized aggregate expressions which hadn't been touched in a while ... this includes some things which will require docs changes ...

source is now a keyword , this is a breaking change.

Will warn when m4warnings are enabled ...

  • COUNT(DISTINCT expression) is gone, will warn and suggestcount(expression)
  • COUNT(*) is gone, will warn and suggest count()
  • SUM/AVG(path) will warn and suggest path.SUM/AVG()
  • SUM/AVG(expr) will warn and suggest source.SUM/AVG()

Here's the full matrix of support ...

Function f() f(expr) source.f() source.f(expr) path.f() path.f(expr)
min/max err ok err ok ok err
sum/avg err warn err ok ok err
count ok ok ok ok ok err

mtoy-googly-moogly avatar Aug 21 '23 19:08 mtoy-googly-moogly

In your matrix, can we clarify path with source_path and field_path? That is, I'm not sure with the current matrix how some_join.avg(some_field) behaves in comparison tosome_join.some_field.avg(). Also, it seems like the grammar doesn't have support for source.stddev(foo), which is a "custom" aggregate function. Should we have that? Maybe that can be a separate PR.

Function f() f(expr) source.f() source.f(expr) srcpath.f() srcpath.f(expr) fldpath.f()
min/max err ok err ok ? ? ?
sum/avg err warn err ok ? ? ?
count ok ok ok ok ? ? ?
stddev ? ? ? ? ? ? ?

christopherswenson avatar Aug 22 '23 14:08 christopherswenson

@christopherswenson i don't understand what a "source path" is as opposed to a "field path"

Yes, this totally ignores custom aggregate functions. We can delay fixing this until we have a solution for all things. This was a quick-ish attempt to address something @lloydtabb said was M4 blocking. We can for sure discuss if it makes sense to do this in two parts or wait for it all at once.

mtoy-googly-moogly avatar Aug 31 '23 16:08 mtoy-googly-moogly

@christopherswenson is doing some new work based on this, closing this PR as his work starts from this branch

mtoy-googly-moogly avatar Sep 05 '23 17:09 mtoy-googly-moogly

re-opening just for the checklist

mtoy-googly-moogly avatar Sep 06 '23 16:09 mtoy-googly-moogly