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

Add parquet-scrooge sources #1064

Merged
merged 5 commits into from
Sep 25, 2014
Merged

Add parquet-scrooge sources #1064

merged 5 commits into from
Sep 25, 2014

Conversation

isnotinvain
Copy link
Contributor

This adds parquet-scrooge sources, and does some cleanup to prevent duplication between the various parquet sources.

@johnynek
Copy link
Collaborator

We are planning to drop 2.9 support after 0.12. Why do the extra work?

On Monday, September 22, 2014, Alex Levenson notifications@github.com
wrote:

This adds parquet-scrooge sources, and does some cleanup to prevent
duplication between the various parquet sources.

This can't be merged until parquet publishes a scala 2.9 artifact for

parquet-scrooge.

You can merge this Pull Request by running

git pull https://github.com/twitter/scalding alexlevenson/parquet-scrooge

Or view, comment on, or merge it at:

#1064
Commit Summary

  • Add parquet-scrooge sources

File Changes

Patch Links:


Reply to this email directly or view it on GitHub
#1064.

Oscar Boykin :: @posco :: http://twitter.com/posco

@isnotinvain
Copy link
Contributor Author

@johnynek well, at that point, we'll need a 2.11 artifact. So same problem whether it's 2.9/2.10 or 2.10/2.11 -- parquet publishes 2.10 only currently.

Using a %% in:
"com.twitter" %% "parquet-scrooge" % "1.6.0rc2"
Is not going to work with anything other than 2.10 currently.

@isnotinvain isnotinvain changed the title [DO NOT MERGE] Add parquet-scrooge sources Add parquet-scrooge sources Sep 24, 2014
@isnotinvain
Copy link
Contributor Author

Refactored to just support 2.10

johnynek added a commit that referenced this pull request Sep 25, 2014
@johnynek johnynek merged commit 9bb612f into develop Sep 25, 2014
@johnynek johnynek deleted the alexlevenson/parquet-scrooge branch September 25, 2014 00:31
@@ -12,6 +12,12 @@ import com.typesafe.sbt.SbtScalariform._
import scala.collection.JavaConverters._

object ScaldingBuild extends Build {

def scalaBinaryVersion(scalaVersion: String) = scalaVersion match {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why delta from the other projects handling of this? I'm a big fan of being consistent.

https://github.com/twitter/bijection/blob/develop/project/Build.scala#L13

Copy link
Collaborator

Choose a reason for hiding this comment

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

I missed that. This is a non-exhaustive match that will fail when we try to use 2.11. I guess we'll see it then and have to fix it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah the one we looked at in chill was not exhaustive either:
https://github.com/twitter/chill/blob/0.4.0/project/Build.scala#L151
(though it is now, we were looking at an older revision).

I just assumed it was not exhaustive on purpose (throw an exception for unrecognized scala version)
The reason I changed this to return the version instead of true / false was I figured it'd be more helpful for modules to have the version to make decisions on. Though I guess if we plan to always be in 2 versions supported at a time world, then mapping to true/false is effectively the same.

I can send an PR to make this match the other projects though.

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.

None yet

3 participants