onyx icon indicating copy to clipboard operation
onyx copied to clipboard

onyx/group-by-path(s)

Open MichaelDrogalis opened this issue 9 years ago • 3 comments

Onyx can currently group by either a single top-level key in a segment, a sequence of top-level keys, or by a function. Respectively those values are:

  • :k
  • [:k1 :k2 :k3]
  • ::grouping-fn

Grouping by non-top level keys is possible by using a grouping function, but is more troublesome than it should be given it's commonality. Extending :onyx/group-by-key is impractical because it's not possible to different from a key path ([:k1 :k1* :k1**]) from a collection of top level keys ([:k1 :k2 :k3]).

The addition of a new options ,:onyx/group-by-path and :onyx/group-by-paths, disambiguates the problem above by explicitly differentiating between a path and a sequence of paths, given that it's impossible to tell whether [[:k1 :k1* :k1**] [:k2 :k2* :k2**]] is a path two levels deep with collection keys, or a collection of key paths, each 3 levels. deep.

:onyx/group-by-key, :onyx/group-by-fn, :onyx/group-by-path, and :onyx/group-by-paths should be mutually exclusive.

MichaelDrogalis avatar Dec 04 '16 00:12 MichaelDrogalis

I agree that the functionality is necessary, but ask that we make a breaking change and switch to a single group-by option, which specifies the type of group-by. Something like :onyx/group-by [:fn some-fn], onyx/group-by [:path [:k1 :k1* :k1**]], etc.

The current mutually exclusive options are annoying to manipulate and validate and will only get more annoying as we add options, and it is not only us that needs to validate whether group-by is in effect, as users often check the task map.

This approach should be a simple transformation, so I don't think it's an expensive breaking change to make. It could also leave us open to user implemented group-by implementations.

On 4 December 2016 at 08:07, Michael Drogalis [email protected] wrote:

Onyx can currently group by either a single top-level key in a segment, a sequence of top-level keys, or by a function. Respectively those values are:

  • :k
  • [:k1 :k2 :k3]
  • ::grouping-fn

Grouping by non-top level keys is possible by using a grouping function, but is more troublesome than it should be given it's commonality. Extending :onyx/group-by-key is impractical because it's not possible to different from a key path ([:k1 :k1* :k1**]) from a collection of top level keys ([:k1 :k2 :k3]).

The addition of a new options ,:onyx/group-by-path and :onyx/group-by-paths, disambiguates the problem above by explicitly differentiating between a path and a sequence of paths, given that it's impossible to tell whether [[:k1 :k1* :k1**] [:k2 :k2* :k2**]] is a path two levels deep with collection keys, or a collection of key paths, each 3 levels. deep.

:onyx/group-by-key, :onyx/group-by-fn, :onyx/group-by-path, and :onyx/group-by-paths should be mutually exclusive.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/onyx-platform/onyx/issues/694, or mute the thread https://github.com/notifications/unsubscribe-auth/AAPZHamlUXuCyVBtvqaTw263aMg2eJ9wks5rEgRAgaJpZM4LDdDy .

lbradstreet avatar Dec 04 '16 04:12 lbradstreet

It would also be useful if aggregation functions supported paths, e.g.

{:window/id          ::test
 :window/task        ::test
 :window/type        :global
 :window/aggregation [::average [:path :to :a :nested :value]]}

kamituel avatar May 16 '18 14:05 kamituel

That'd be nice as it'd improve re-usability of the aggregations. I'd support a PR written in a way that is performant.

lbradstreet avatar May 16 '18 17:05 lbradstreet