Skip to content

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.

Sign up for GitHub

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

Closed
imarios wants to merge 1 commit into from
Closed

Adding back lit for TypedAggregate (renamed to litAggr) #143

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion dataset/src/main/scala/frameless/functions/AggregateFunctions.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,15 @@ import org.apache.spark.sql.{functions => untyped}

trait AggregateFunctions {

private def typedColumnToAggregate[A: TypedEncoder, T](a: TypedColumn[T, A]): TypedAggregate[T, A] =
new TypedAggregate[T,A](a.expr)

/** Creates a [[frameless.TypedColumn]] of literal value. If A is to be encoded using an Injection make
* sure the injection instance is in scope.
*
* apache/spark
*/
Copy link
Contributor Author

@imarios imarios May 30, 2017 •
edited
Loading

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.

Copy link
Contributor

@OlivierBlanvillain OlivierBlanvillain May 30, 2017

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

Copy link
Contributor Author

@imarios imarios May 30, 2017

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

Copy link
Contributor Author

@imarios imarios May 30, 2017 •
edited
Loading

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]?

Copy link
Contributor Author

@imarios imarios May 30, 2017

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 elided

Copy link
Contributor

@OlivierBlanvillain OlivierBlanvillain May 30, 2017

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...

Copy link
Contributor Author

@imarios imarios May 30, 2017

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.

Copy link
Contributor Author

@imarios imarios May 30, 2017

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?

Copy link
Contributor

@kanterov kanterov Jun 7, 2017

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.

Copy link
Contributor Author

@imarios imarios Jun 15, 2017

Choose a reason for hiding this comment

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

@kanterov this is precluded in #153. Let's move the discussion there.

def lit[A: TypedEncoder, T](value: A): TypedColumn[T, A] = frameless.functions.lit(value)
def litAggr[A: TypedEncoder, T](value: A): TypedAggregate[T, A] = typedColumnToAggregate(lit(value))

/** Aggregate function: returns the number of items in a group.
*
Expand Down
1 change: 1 addition & 0 deletions dataset/src/test/scala/frameless/SchemaTests.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package frameless

import frameless.functions.lit
import frameless.functions.aggregate._
import org.scalacheck.Prop
import org.scalacheck.Prop._
Expand Down
11 changes: 11 additions & 0 deletions dataset/src/test/scala/frameless/functions/AggregateFunctionsTests.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,17 @@ class AggregateFunctionsTests extends TypedDatasetSuite {
check(forAll(prop[Double] _))
}

test("litAggr") {
def prop[A: TypedEncoder, B: TypedEncoder, C: TypedEncoder](xs: List[A], b: B, c: C): Prop = {
val dataset = TypedDataset.create(xs)
val (r1, rb, rc, rcount) = dataset.agg(litAggr(1), litAggr(b), litAggr(c), count()).collect().run().head
(rcount ?= xs.size.toLong) && (r1 ?= 1) && (rb ?= b) && (rc ?= c)
}

check(forAll(prop[Boolean, Int, String] _))
check(forAll(prop[Option[Boolean], Vector[Option[Vector[Char]]], Long] _))
}

test("count") {
def prop[A: TypedEncoder](xs: List[A]): Prop = {
val dataset = TypedDataset.create(xs)
Expand Down