twitter / algebird

Abstract Algebra for Scala
https://twitter.github.io/algebird
Apache License 2.0
2.29k stars 345 forks source link

Change type for rollup #488

Open johnynek opened 9 years ago

johnynek commented 9 years ago

Right now, cubing and rollup have the same type of keys: tuples or Options.

The problem is, for rollup, most of the possible values are uninhabited. For instance None, Some(_) would never happen. A better type would be:

(A, B) => Option[(A, Option[B])]
(A, B, C) => Option[(A, Option[(B, Option[C])])]

now, occasionally we chose looser types because using them is significantly easier, for instance in outerJoin in scalding we make the value type be: (Option[V1], Option[V2]) but (None, None) is impossible (we'd be better with a type that EitherOrBoth or something that has three values).

If we are going to change this, we should do it before we publish a new version (even though it does not break binary compatibility because the types that change are generated by a macro).

Note, scalding's TypedText type could flatten this key to read and write it without a problem (unless the values are Strings, since Option[String] is ambiguous.

/cc @sid-kap @ianoc

avibryant commented 9 years ago

Probably unhelpful comment: those types remind me of shapeless' HList.

johnynek commented 9 years ago

Yes. It is like an HList that may have a length from 0 to n, and that is not known at compile time, but given a position in the list, if present, the type is known.

avibryant commented 9 years ago

I'm too lazy to think about this right this second, but it seems like there might be an infix type constructor notation that would make these nested options less awkward.

sid-kap commented 9 years ago

This makes sense. As a bonus, this would allow for dealing with case classes with more than 22 values. Also, it would mean that the user doesn't have to see long strings of None,None,None.

avibryant commented 9 years ago

Yeah, so if you do a right-associative infix type constructor it works:

type >>:[A,B] = (A,Option[B])
type StringIntDoubleRollup = Option[String >>: Int >>: Double]
val x: StringIntDoubleRollup = Some(("foo", Some((1, Some(1.0)))))
ianoc commented 9 years ago

Thing I'd wonder about this: is will all use cases just revert to converting to a tuple of options? Most use cases like writing out to thrift will be much more natural/easy with a struct of optional fields, we'd just end up immediately unpacking this alternate type right?

johnynek commented 9 years ago

That's a cool trick, @avibryant. One wonders if not having TupleN rather than a similar right associative function and unapplies would circumvent some of the Tuple22 pain:

val myTup: Int ->: String ->: Int = 1 ->: "yo" -> 2

val x ->: y ->: z = myTup // unapply here

Of course, you could do the nesting in the first position rather than the second and not need the right associativity (and ensuing : ugliness).