AlignmentRecordRDD does not extend GenomicRDD per javac #1092

Closed
heuermh opened this Issue Jul 21, 2016 · 4 comments

Comments

Projects
2 participants
@heuermh
Member

heuermh commented Jul 21, 2016

The following Java source does not compile

  SparkContext sc = ...;
  JavaADAMContext javaAdamContext = new JavaADAMContext(sc);
  AlignmentRecordRDD alignments = javaAdamContext.loadAlignments(inputPath);
  JavaRDD<AlignmentRecord> rdd = alignments.jrdd();
$ mvn compile
...
[ERROR] .../JavaCountAlignments.java:87: error: cannot find symbol
[ERROR]         JavaRDD<AlignmentRecord> rdd = alignments.jrdd();
[ERROR]                                                  ^
[ERROR]   symbol:   method jrdd()
[ERROR]   location: variable alignments of type AlignmentRecordRDD

It appears javac does not consider `AlignmentRecordRDD` extending from `GenomicRDD`
$ javap -cp adam-core_2.10-0.19.1-SNAPSHOT.jar \
  org.bdgenomics.adam.rdd.read.AlignmentRecordRDD

Compiled from "AlignmentRecordRDD.scala"
public interface org.bdgenomics.adam.rdd.read.AlignmentRecordRDD {
  public abstract org.bdgenomics.adam.rdd.fragment.FragmentRDD toFragments();
  ...

$ javap -cp adam-core_2.10-0.19.1-SNAPSHOT.jar \
  org.bdgenomics.adam.rdd.GenomicRDD

Compiled from "GenomicRDD.scala"
public interface org.bdgenomics.adam.rdd.GenomicRDD<T, U extends org.bdgenomics.adam.rdd.GenomicRDD<T, U>> {
  public abstract org.apache.spark.rdd.RDD<T> rdd();
  ...

Here is the hierarchy in Scala source

sealed trait AlignmentRecordRDD
  extends AvroReadGroupGenomicRDD[AlignmentRecord, AlignmentRecordRDD]
{ ... }

abstract class AvroReadGroupGenomicRDD[T <% IndexedRecord: Manifest, U <: AvroReadGroupGenomicRDD[T, U]]
  extends AvroGenomicRDD[T, U]
{ ... }

abstract class AvroGenomicRDD[T <% IndexedRecord: Manifest, U <: AvroGenomicRDD[T, U]]
  extends ADAMRDDFunctions[T] with GenomicRDD[T, U]
{ ... }

Surprisingly, this compiles and works at runtime
  JavaADAMContext javaAdamContext = new JavaADAMContext(sc);
  AlignmentRecordRDD alignments = javaAdamContext.loadAlignments(inputPath);
  JavaRDD<AlignmentRecord> rdd = ((GenomicRDD) alignments).jrdd();
$ ADAM_MAIN=com.github.heuermh.adam.commands.JavaCountAlignments \
  adam-submit --jars adam-commands_2.10-0.19.1-SNAPSHOT.jar \
  -- \
  small.sam 

Using ADAM_MAIN=com.github.heuermh.adam.commands.JavaCountAlignments
Using SPARK_SUBMIT=/usr/local/bin/spark-submit
(1,20)
@fnothaft

This comment has been minimized.

Show comment
Hide comment
@fnothaft

fnothaft Jul 21, 2016

Member

I'm thinking that the problem may be that we're using traits and not abstract classes. I don't think most of these classes explicitly need multiple inheritance, so perhaps that's the cleanest route to a resolution?

Member

fnothaft commented Jul 21, 2016

I'm thinking that the problem may be that we're using traits and not abstract classes. I don't think most of these classes explicitly need multiple inheritance, so perhaps that's the cleanest route to a resolution?

@heuermh

This comment has been minimized.

Show comment
Hide comment
Member

heuermh commented Jul 21, 2016

Full source example heuermh/adam-commands@54ec130

@heuermh heuermh modified the milestone: 0.20.0 Sep 7, 2016

@heuermh heuermh referenced this issue Sep 7, 2016

Closed

Release ADAM version 0.20.0 #1048

47 of 61 tasks complete

@heuermh heuermh modified the milestones: 0.20.0, 0.22.0 Oct 13, 2016

@fnothaft

This comment has been minimized.

Show comment
Hide comment
@fnothaft

fnothaft Feb 6, 2017

Member

Traits hate him! A software engineer in Oakland is able to get AlignmentRecordRDD to extend GenomicRDD with this one weird trick!

diff --git a/adam-core/src/main/scala/org/bdgenomics/adam/rdd/read/AlignmentRecordRDD.scala b/adam-core/src/main/scala/org/bdgenomics/adam/rdd/read/AlignmentRecordRDD.scala
index 0a00175..d82791e 100644
--- a/adam-core/src/main/scala/org/bdgenomics/adam/rdd/read/AlignmentRecordRDD.scala
+++ b/adam-core/src/main/scala/org/bdgenomics/adam/rdd/read/AlignmentRecordRDD.scala
@@ -83,7 +83,7 @@ private[adam] class AlignmentRecordArraySerializer extends IntervalArraySerializ
   }
 }
 
-sealed trait AlignmentRecordRDD extends AvroReadGroupGenomicRDD[AlignmentRecord, AlignmentRecordRDD] {
+abstract class AlignmentRecordRDD extends AvroReadGroupGenomicRDD[AlignmentRecord, AlignmentRecordRDD] {
 
   protected def buildTree(rdd: RDD[(ReferenceRegion, AlignmentRecord)])(
     implicit tTag: ClassTag[AlignmentRecord]): IntervalArray[ReferenceRegion, AlignmentRecord] = {

Anywho, before I open this PR, what are peoples thoughts about having AlignmentRecordRDD, AlignedReadRDD, and UnalignedReadRDD? I originally thought it would be useful to have a separate class for RDDs of unaligned reads, but over the last 6 months, I haven't actually found a case where it has been useful. I'm somewhat inclined to remove AlignedReadRDD and UnalignedReadRDD and make AlignmentRecordRDD concrete. Any objections?

Member

fnothaft commented Feb 6, 2017

Traits hate him! A software engineer in Oakland is able to get AlignmentRecordRDD to extend GenomicRDD with this one weird trick!

diff --git a/adam-core/src/main/scala/org/bdgenomics/adam/rdd/read/AlignmentRecordRDD.scala b/adam-core/src/main/scala/org/bdgenomics/adam/rdd/read/AlignmentRecordRDD.scala
index 0a00175..d82791e 100644
--- a/adam-core/src/main/scala/org/bdgenomics/adam/rdd/read/AlignmentRecordRDD.scala
+++ b/adam-core/src/main/scala/org/bdgenomics/adam/rdd/read/AlignmentRecordRDD.scala
@@ -83,7 +83,7 @@ private[adam] class AlignmentRecordArraySerializer extends IntervalArraySerializ
   }
 }
 
-sealed trait AlignmentRecordRDD extends AvroReadGroupGenomicRDD[AlignmentRecord, AlignmentRecordRDD] {
+abstract class AlignmentRecordRDD extends AvroReadGroupGenomicRDD[AlignmentRecord, AlignmentRecordRDD] {
 
   protected def buildTree(rdd: RDD[(ReferenceRegion, AlignmentRecord)])(
     implicit tTag: ClassTag[AlignmentRecord]): IntervalArray[ReferenceRegion, AlignmentRecord] = {

Anywho, before I open this PR, what are peoples thoughts about having AlignmentRecordRDD, AlignedReadRDD, and UnalignedReadRDD? I originally thought it would be useful to have a separate class for RDDs of unaligned reads, but over the last 6 months, I haven't actually found a case where it has been useful. I'm somewhat inclined to remove AlignedReadRDD and UnalignedReadRDD and make AlignmentRecordRDD concrete. Any objections?

@heuermh

This comment has been minimized.

Show comment
Hide comment
@heuermh

heuermh Feb 7, 2017

Member

I'm somewhat inclined to remove AlignedReadRDD and UnalignedReadRDD and make AlignmentRecordRDD concrete.

+1
That's how it is in the Oaktown

Member

heuermh commented Feb 7, 2017

I'm somewhat inclined to remove AlignedReadRDD and UnalignedReadRDD and make AlignmentRecordRDD concrete.

+1
That's how it is in the Oaktown

fnothaft added a commit to fnothaft/adam that referenced this issue Feb 8, 2017

[ADAM-1092] AlignmentRecordRDD should be an abstract class.
Resolves #1092. Prior to this, javac would not recognize the AlignmentRecordRDD
trait as extending GenomicRDD.

@heuermh heuermh closed this in #1386 Feb 8, 2017

heuermh added a commit that referenced this issue Feb 8, 2017

[ADAM-1092] AlignmentRecordRDD should be an abstract class.
Resolves #1092. Prior to this, javac would not recognize the AlignmentRecordRDD
trait as extending GenomicRDD.

@heuermh heuermh added this to Completed in Release 0.23.0 Mar 8, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment