Changes to RDD utility files for new variant schema #114
Conversation
…t, ADAMVariantContextRDDFunctions, FieldEnumerationSuite
| @@ -151,15 +145,14 @@ class AdamRecordRDDFunctions(rdd: RDD[ADAMRecord]) extends Serializable with Log | |||
| * @param r Read to map. | |||
| * @return List containing one or two mapping key/value pairs. | |||
| */ | |||
| def mapToBucket (r: ADAMRecord): List[(ReferencePosition, ADAMRecord)] = { | |||
| def mapToBucket (r: ADAMRecord): List[(Long, ADAMRecord)] = { | |||
There was a problem hiding this comment.
Are these changes intended? It looks like they revert to older code?
There was a problem hiding this comment.
Agreed; was that intentional? If not, this is the other cause of the build failure.
|
One or more automated tests failed |
| @@ -31,7 +31,7 @@ class ParquetFileTraversable[T <: IndexedRecord](sc: SparkContext, file: Path) e | |||
| } | |||
| val status = fs.getFileStatus(file) | |||
| var paths = List[Path]() | |||
| if (status.isDir) { | |||
| if (status.isDirectory) { | |||
There was a problem hiding this comment.
Just as a heads up, this is not reverse compatible with Hadoop 1.
There was a problem hiding this comment.
This should probably be discussed in some detail.
At the least, this change should also add some text into the POM saying that only 2.2 is supported (or equivalent).
For us here, this won't affect us. However, I feel like we could get by with waiting until 0.7.0 for this change, or possibly even a 0.6.2 release.
There was a problem hiding this comment.
It won't affect us here at Sinai, either. Do we have anyone using Hadoop 1.x?
There was a problem hiding this comment.
I'd vote for removing the comment about pre-2.2 from the POM instead.
There was a problem hiding this comment.
We've been running Hadoop 1.0.4. I'll see if we can move to Hadoop 2 for all our work.
There was a problem hiding this comment.
Nevermind, we're fine with Hadoop 2. Let's just make the move then.
There was a problem hiding this comment.
Wait. Why would be break Hadoop 1 compatibility if we can easily avoid it?
Can we just us "status.isDir" here and then open a discussion with the broader group on the mailing list? If everyone is ok with dropping Hadoop 1.x support, that's fine. We shouldn't decide that here.
|
Looks good to me @nealsid ! |
|
Do you want this to merge in now, or want to wait for more reviews? |
|
Wait a few more hours please... On Tue, Feb 18, 2014 at 5:09 PM, Frank Austin Nothaft
|
| @@ -0,0 +1,56 @@ | |||
| /* | |||
| * Copyright (c) 2013. Mount Sinai School of Medicine | |||
There was a problem hiding this comment.
This should probably be updated to 2014
|
One or more automated tests failed |
|
@nealsid: Is this ready to be merged, or did you want to address your comment first? |
|
One or more automated tests failed |
|
It's ready to merge, sorry, I should have clarified that. |
Changes to RDD utility files for new variant schema
|
Thanks @nealsid! Merged. |
After this I will work on cleaning the build.