-
-
Notifications
You must be signed in to change notification settings - Fork 138
Adding back lit for TypedAggregate (renamed to litAggr) #143
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
Conversation
| * sure the injection instance is in scope. | ||
| * | ||
| * apache/spark | ||
| */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As you see, lit() is returning TypedColumn instead of TypedAggregate even though it's defined inside AggregateFunctions (so you would expect to operate during aggregation). Changing to lit() there was an ambiguity with lit() defined inside functions. So if both were in scope it was confusing the compiler. That's why I rename to litAggr.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder returning a TypedColumn[T, A] with TypedAggregate[T, A] would work
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, let me try this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@OlivierBlanvillain You suggest we change lit to: def lit[A: TypedEncoder, T](value: A): TypedAggregate[T,A] with TypedColumn[T, A]?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doing that gives me:
java.lang.ClassCastException: frameless.TypedColumn cannot be cast to frameless.TypedAggregate
at frameless.functions.package$.lit(package.scala:14)
... 42 elidedThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's the right hand side of your lit def?
A => B and A => C
Yeah that won't work. There is a hack which is to define the second one as def (a: A)(implicit d: DummyImplicit): C, but I guess this wouldn't apply here...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, that's the rhs of lit(). I am open to making this more elegant ... I did try to have the same name and make them both work, but I wasn't able to get there. That's when I resorted to changing the name and calling it litAggr.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know @kanterov worked on the TypedAggretate migration. Gleb, any suggestion here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm... one of the ways to fix would be to revert and get back to a hierarchy with TypedAggregateAndColumn.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Codecov Report
@@ Coverage Diff @@
## master #143 +/- ##
==========================================
- Coverage 93.02% 92.86% -0.16%
==========================================
Files 28 28
Lines 602 603 +1
Branches 12 11 -1
==========================================
Hits 560 560
- Misses 42 43 +1
Continue to review full report at Codecov.
|
9b2cfe3 to
6f6b838
Compare
|
Hi @kanterov, any input on this one? |
|
@OlivierBlanvillain @kanterov anything else we need to address here? |
|
Let's be patient and wait for @kanterov feedback on this one 😄 |
|
Sorry, just noticed this, I don't know, need to consider :) |
|
covered by #153. closing. |
After some gitter discussion I realized that we missed lit() for aggr.
Here is what you can do now with litAggr. Before, lit() would not work during aggregations.