Skip to content
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

Make it easier to get around Tuple22 restrictions with Shapeless/HLists #1236

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from

Conversation

matt-martin
Copy link
Contributor

This is just a very basic attempt to integrate Shapeless (specifically HLists) into Scalding. There's certainly more that could be done, but I think this pull request represents a necessary first step. I would love some feedback, since I'm basically brand new to Shapeless and did this partly as an exercise to try to learn more about it. If this looks good/useful, then I suppose the next step would be thinking about how to use extensible records like Miles did in the example he already put together.

) ++ (
if(isScala210x(scalaVersion.value)) {
Seq(
"com.chuusai" % "shapeless_2.10.4" % shapelessVersion
Copy link
Collaborator

Choose a reason for hiding this comment

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

is this actually compatible with 2.10.3 -> 2.10.5?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My understanding (based on the README) is that shapeless 2.1.0 is only compatible with 2.11.x and 2.10.4, but I might be mistaken. I think the relevant bits are as follows:

Builds are available for Scala 2.11.x and for Scala 2.10.4
...
Note that for Scala 2.10.4 you must provide an explicit Scala version suffix to your shapeless dependency, and it is recommanded to add the macro paradise plugin to your build, for some macros provided by shapeless to work smoothly

For shapeless 2.0.0, they have the following comments:

Builds are available for Scala 2.11.x and for Scala 2.10.4.
...
For Scala 2.10.x you must specify a Scala version of at least 2.10.2, and add either cross CrossVersion.full or provide an explicit Scala version suffix to your shapeless dependency
...
Note that Scala 2.10.x releases are compatible with each other starting from 2.10.2, so a mismatch in minor versions above would be fine.

Copy link
Contributor

Choose a reason for hiding this comment

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

Shapeless 2.1.0 has full cross-versions for 2.10 starting at 2.10.4 (e.g for 2.10.5), so it'd be better to use this:

"com.chuusai" %% "shapeless" % shapelessVersion cross CrossVersion.full

So that we get the 2.10.5 version when Scalding updates to 2.10.5.

(I don't remember whether you need %% for full cross-versioning—I think either will work, and I find it a little clearer with %%.)

@matt-martin
Copy link
Contributor Author

See also: #746

new CTuple(list:_*)
}

override def arity: Int = ti()
Copy link
Collaborator

Choose a reason for hiding this comment

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

should be a val, to be sure we only call once.

@johnynek
Copy link
Collaborator

This is great.

There are two thoughts I have:

  1. I think we should add a scalding-shapeless subproject since dependencies are a real concern of people. Then you could import com.twitter.scalding.shapeless_support._ or something to get the needed implicits and types you have here.

  2. We should show an example with a TypedPipe and do something non-trivial with it.

All said, I am starting to think we don't need this as much as we used to now that we have macros to do this (and they should have no performance issues). You can make a case class for your record and get a TypeDescriptor that has all the stuff you need to flatten into cascading.

implicit def hListConverter[H, T <: HList, N <: Nat]
(implicit
g: TupleGetter[H],
len: Length.Aux[H :: T, N],
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm still getting a sense of the design here, but any reason you don't constrain the FromTupleEntry instances so that the I (which could be a type member) matches the length of the HList? That should allow you to omit the N type parameter here altogether.

@travisbrown
Copy link
Contributor

See my branch here for a quick demonstration of an alternative approach.

The key idea is that you don't need to track the length of the HList separately at the type level, so there are no Nat type parameters anywhere. You also don't need to check the size of the TupleEntry at each element, and my implementation avoids this by using a helper method that assumes the length has already been checked.

More controversially, you don't necessarily even need a separate FromTupleEntry type, since you can get rid of the Nat type parameter, and my FromTupleEntry extends TupleConverter directly. This means that instead of carrying around a Nat parameter and ToInt-ing it as needed, you can just use the arity that you're already tracking for TupleConverter.

All of the new tests pass unchanged.

@travisbrown
Copy link
Contributor

I should add that this would be a pretty fantastic start even if you weren't brand new to Shapeless, @matt-martin.

@matt-martin
Copy link
Contributor Author

Thanks all for the comments. Only skimmed them briefly today, but will take a closer look at them tomorrow

@milessabin
Copy link

I'm very happy to support things like this ... ping me if there's anything I can do to help.

@travisbrown
Copy link
Contributor

One other thing: it might be worth trying to do this via ProductTypeClass. I haven't tried it, and getting it to play nicely with everything else might be tricky, but if it worked it could give you converters for case classes for free.

@ianoc
Copy link
Collaborator

ianoc commented Mar 31, 2015

One thing on a code perspective, given shapelesses tighter requirements around scala versioning and adding another dep I think it would make sense to move this into a scalding-shapeless module. It'll make it easier to accept/merge.

@milessabin
Copy link

FWIW, from shapeless-2.2.0 I'm going to drop the specific sub-version requirements for Scala 2.10.x.

@CLAassistant
Copy link

CLAassistant commented Jul 18, 2019

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants