Remove several implicits for TraversableOnce to Fields

[johnynek]

addresses #1657

Note, .isDefined on fields is actually what you might expect:

scala> import com.twitter.scalding.Dsl._
import com.twitter.scalding.Dsl._

scala> List().isDefined
res0: Boolean = false

scala> List(1).isDefined
res1: Boolean = true

scala> List(1, 2).isDefined
res2: Boolean = true

where you get in trouble is when the container type cannot be converted to a field (which is common). Then you get a runtime error in that case. So, the risk is less than I thought for this case: either a "correct" result or a runtime exception.

Note, we can't remove the implicit for List (as we see above) because List extends Product. It is pretty crazy that Product has an implicit in scalding jobs... We could get rid of that probably without breaking too many people by adding 22 implicits converting each of the tuples to Fields without using Product. Maybe that is the way to go.

[piyushnarang]

+1, changes look good to me. We have ~20 or so failing targets due to these implicits (some fairly simple to fix, some will likely require us to create an internal trait / object that exposes these implicits to avoid calling fields / intFields a bunch of times in a given job). Would make our internal migration to 0.17.0 simpler if we pulled this in for the next release as there's a bunch of other things to be fixed / handled in it. So if you're not affected by it, would prefer merging this after we cut the rel.

[johnynek]

we can wait.

[johnynek]

So, @fwbrasil had the idea that we should port these implicits to use a macro so we could fail at compile time if we can't do the conversion, that will probably work for 99% of the Twitter fields API uses and allow us to remove all unsafe implitics.

[ianoc]

Should we re-visit this now since 0.17 is out?

[piyushnarang]

@ianoc yeah we released 0.17.x at Twitter a few months back. I'll let @fwbrasil / @pankajroark / @ttim chime in if they have any reservations about the fields api breakages.

[johnynek]

I’d love for you guys to chime in and see if you can do a CI build or develop to see if we have any source incompatibilities that look painful.

On Wed, Oct 4, 2017 at 09:30 Piyush Narang [email protected] wrote:

@ianoc https://github.com/ianoc yeah we released 0.17.x at Twitter a few months back. I'll let @fwbrasil https://github.com/fwbrasil / @pankajroark https://github.com/pankajroark / @ttim https://github.com/ttim chime in if they have any reservations about the fields api breakages.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/twitter/scalding/pull/1659#issuecomment-334264589, or mute the thread https://github.com/notifications/unsubscribe-auth/AAEJdraYeoua5UKpNC2_c6oeA6Oa6D1iks5so9zagaJpZM4MsXhD .

-- P. Oscar Boykin, Ph.D. | http://twitter.com/posco | http://pobox.com/~boykin

[pankajroark]

Let me start a sandbox for this. I'll let someone else in the team evaluate the results, perhaps @fwbrasil

[pankajroark]

Actually something seems broken in my set up, I'm unable to publish to internal maven. I'll let someone else in the team run the sandbox tomorrow.

[johnynek]

We are getting close to 0.18 in my opinion.

I’d like to remove as much old code as we can without breaking (more than a few) call sites at Twitter. I figure if we don’t break Twitter, almost everyone else will be okay.

On Wed, Oct 4, 2017 at 19:14 Pankaj Gupta [email protected] wrote:

Actually something seems broken in my set up, I'm unable to publish to internal maven. I'll let someone else in the team run the sandbox tomorrow.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/twitter/scalding/pull/1659#issuecomment-334361464, or mute the thread https://github.com/notifications/unsubscribe-auth/AAEJdiZnSEgdhkLoIC1VHid_BYB4vWtTks5spGWjgaJpZM4MsXhD .

-- P. Oscar Boykin, Ph.D. | http://twitter.com/posco | http://pobox.com/~boykin

[ianoc-stripe]

Is this still valid? if so if the cascading 3 big breaking change is coming might be nice to bundle this in if it doesn't break tw

[johnynek]

@fwbrasil what do you think? Could you see about fixing the 20 targets that are using this to import your own private implicit in those targets? I'd really love to remove having an implicit on every collection that has similar methods (isDefined) as scala has.

[fwbrasil]

@johnynek I think it's reasonable. Can we have a deprecated object in Scalding that has the implicits so we just need to import them?

[johnynek]

that means we have to publish for you to fix.

you could try now to see what the errors are, build with this branch, and then just fix it currently so merging won't cause you problems later. Of course, that won't help you spot future regressions, but I guess most new code is the typed API that does not use this.

[fwbrasil]

I think it's fine if we fix this while updating the scalding version

[johnynek]

@fwbrasil I added DeprecatedFieldConversions and I exercise it in the tests:

https://github.com/twitter/scalding/pull/1659/files#diff-2b5cd65002aa3a695acf20a452296452R22

What do you think can we merge this?

[CLAassistant]

All committers have signed the CLA.