Use optional third argument as edge attribute.

[npanj]

This is probably easiest way to include edge attribute. Unless you guys have been thinking about some other ways? I also wanted to add one more argument for default edge attribute value..probably I will do that in another patch.

[AmplabJenkins]

Can one of the admins verify this patch?

[rxin]

This is definitely a good change.

[rxin]

Jenkins, test this please.

[rxin]

Upon further look, it might be better if we allow users to define the way to parse the 3rd argument to allow arbitrary data types. What do you think?

[AmplabJenkins]

Merged build triggered.

[AmplabJenkins]

Merged build started.

[AmplabJenkins]

Merged build finished.

[AmplabJenkins]

Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/15258/

[npanj]

Allowing arbitrary data types sounds like a good idea. I actually tried to do something like this:

---

def edgeListFile[[@specialized(Long, Int, Double) ED: ClassTag]](sc: SparkContext,
path: String,
canonicalOrientation: Boolean = false,
minEdgePartitions: Int = 1)
: Graph[Int, ED] =

and

if (lineArray.length >= 3) lineArray(2).asInstanceOf[ED]
else 1.asInstanceOf[ED]

---

But I am running into this error:

---

java.lang.ClassCastException: java.lang.String cannot be cast to java.lang.Integer
at scala.runtime.BoxesRunTime.unboxToInt(BoxesRunTime.java:106)

---

I guess I will have to handle primitive types separately?
(I am quite new to Scala) Are there any known patterns to handle casting? If you can point me to a code snippet within spark codebase(or else where ) that will be great.

Also do you guys follow any scala code style guide? ; so that I can follow that for future patches

[ankurdave]

Unlike Python (but like Java), Scala doesn't use asInstanceOf for arbitrary type conversions. In this case, it won't work to do "123".asInstanceOf[Int], because Scala doesn't know how to do the conversion. There are two ways to resolve this:

1. Have edgeListFile take a function that does the conversion:

   def edgeListFile[ED: ClassTag](..., parser: String => ED) = {
     ... parser(lineArray(2)) ...
   }
   // Can be called like this:
   GraphLoader.edgeListFile[Int](..., _.toInt)
   GraphLoader.edgeListFile[String](..., identity)

   This is simple, but it doesn't facilitate reuse of parsing functions. You always have to specify how to parse an Int, even though it's usually the same everywhere.

2. Define a library of standard parsers and use Scala implicits to select the right one automatically if it exists:

   // `Parseable[T]` is a type class [1].  If it is defined for Foo (meaning
   // there is an `implicit val` of type Parseable[Foo] in scope), it means you
   // can parse any String into a Foo.
   trait Parseable[T] { def parse(s: String): T }
   
   // Here are some parsers. The user can define their own by creating
   // additional implicit vals.
   implicit val IntParser = new Parseable[Int] { def parse(s: String) = s.toInt }
   implicit val StringParser = new Parseable[String] { def parse(s: String) = s }
   
   // Instead of an explicit function parameter, we use a context bound, which
   // lets us access the parser for ED using `implicitly[Parseable[ED]]`.
   def edgeListFile[ED: ClassTag : Parseable](...) = {
     ... implicitly[Parseable[ED]].parse(lineArray(2)) ...
   }
   // Can be called without passing anything extra:
   GraphLoader.edgeListFile[Int](...)
   GraphLoader.edgeListFile[String](...)

[1] http://docs.scala-lang.org/tutorials/FAQ/finding-implicits.html#context_bounds

[ankurdave]

ok to test

[AmplabJenkins]

Merged build triggered.

[AmplabJenkins]

Merged build started.

[AmplabJenkins]

Merged build finished.

[AmplabJenkins]

Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/15320/

[AmplabJenkins]

Merged build triggered.

[AmplabJenkins]

Merged build started.

[AmplabJenkins]

Merged build finished.

[AmplabJenkins]

Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/15329/

[AmplabJenkins]

Merged build triggered.

[AmplabJenkins]

Merged build started.

[AmplabJenkins]

Merged build finished.

[AmplabJenkins]

Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/15337/

[AmplabJenkins]

Merged build triggered.

[AmplabJenkins]

Merged build started.

[AmplabJenkins]

Merged build finished.

[AmplabJenkins]

Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/15340/

[npanj]

@ankurdave

• thanks for explanation about parsable and use of implicit.(I will try to implement it)
• about storage level of the vertex attributes, yes I was trying to see if it can be changed to handle memory pressure; but that didn't work. From your patch it seems that it is more complicated than what I thought initially. Any reason why this patch was not submitted? Also I believe some parts of this patch will also be applicable to SPARK-1991?
  (actually, I didn't mean to consider serialization changes for this pull request... but I guess everything from forked branch is pulled as pull request?; I haven't submitted patched on github before, so I was not familiar about it...)

[pwendell]

@ankurdave @rxin could you guys come to a decision on this one way or the other? Also @npanj mind adding [GRAPHX] to the title here? Right now this is getting sorted with the Spark core patches, which might be why people haven't seen it.

[ankurdave]

@pwendell @npanj @rxin This isn't mergeable at the moment because it doesn't parse the edge attributes correctly. Since it's difficult to do this parsing in general, there are two choices for now:

1. Modify the PR to accept only Int edge attributes (i.e., remove ED). This would help users who just want Int edge attributes, but it introduces a new special-case public API that we will need to support in the future.
2. Close the PR for now, and for users who want edge attributes, encourage them to implement their own custom version of GraphLoader.

I'd suggest closing this for now.

[SparkQA]

Can one of the admins verify this patch?

[ankurdave]

@npanj Mind closing this?

[npanj]

Sorry was out of loop for a while... closing it