Window dense rank #248
Window dense rank #248
Conversation
|
|
||
| //Need different name because companion class has `orderByMany` defined as well | ||
| //Need a class and not object in order to define what `T` is explicitly. Otherwise it's a mess | ||
| //This makes for some pretty horrid syntax though. |
There was a problem hiding this comment.
Very open to suggestions on how to make this syntax nicer. https://github.com/typelevel/frameless/pull/248/files#diff-c5eeb2825da1c4337deff8bccfefb194R93 is pretty gross in my view.
|
|
||
| } | ||
|
|
||
| //TODO: Move these to the other funcs? |
There was a problem hiding this comment.
should WindowFunction be its own object or should we lop them in with the other functions?
There was a problem hiding this comment.
How is it done in vanilla? I would follow that.
There was a problem hiding this comment.
it's in with the regular functions in functions.scala but that file is also 2500 lines long and seems a bit excessive.
We've also split out AggregateFunctions despite those also originally coming from functions.scala
|
|
||
| trait WindowFunctions { | ||
|
|
||
| //TODO: TypedAggregate version that can be used in `agg` |
There was a problem hiding this comment.
Punting to a later PR. I'm still unsure how to do this right.
Codecov Report
@@ Coverage Diff @@
## master #248 +/- ##
==========================================
- Coverage 96.99% 96.15% -0.84%
==========================================
Files 52 54 +2
Lines 866 885 +19
Branches 10 9 -1
==========================================
+ Hits 840 851 +11
- Misses 26 34 +8
Continue to review full report at Codecov.
|
|
|
||
| } | ||
|
|
||
| //TODO: Move these to the other funcs? |
There was a problem hiding this comment.
How is it done in vanilla? I would follow that.
| import shapeless.{ HList, ProductArgs } | ||
|
|
||
| trait OrderedWindow | ||
| trait PartitionedWindow |
There was a problem hiding this comment.
Have you considered using a HKT instead?
There was a problem hiding this comment.
I'm not sure what you mean by that here. I use these traits to ensure that a partition or an order has been assigned to a window as some window functions require one or both to be present (eg denseRank needs an ordering). An ordering might be on multiple disparate types and has no affect on the return type of the window. What would be gained by making OrderedWindow a HKT here or do you mean something else?
|
Hey @frosforever, anything else you want to add here? Should I go over it and merge if everything looks ok? |
|
Hey @imarios, sorry this has languished a bit. I've left some comments on things I'm a bit stuck on that aren't really blockers but I'm very open to suggestions. |
|
@frosforever Do you plan to work again on this PR? It's more than 6 months old, if not then we should probably close it... |
|
Hey @OlivierBlanvillain! Sadly this PR is stuck in a bit of an impasse and I have not had time to return to it. I'd love to get back to this at some point but have no idea when that might happen. |
Builds on top of #236, and replaces #225.
Adds Window function support with
denseRankas the first supported function.TODO in other later PRs: