[SPARK-27680][CORE][SQL][GRAPHX] Remove usage of Traversable #24584
Conversation
| } | ||
|
|
||
| /** Set multiple parameters together */ | ||
| def setAll(settings: Iterable[(String, String)]): SparkConf = { |
There was a problem hiding this comment.
Any caller is almost certainly passing an object that is Iterable as well as Traversable, but this maintains binary compatibility for now.
| case e: ExprValue => transform(e) | ||
| case Some(value) => Some(doTransform(value)) | ||
| case seq: Traversable[_] => seq.map(doTransform) | ||
| case seq: Iterable[_] => seq.map(doTransform) |
There was a problem hiding this comment.
This is the only part of the change that is of concern. It's internal, but this now only matches Iterable and not Traversable, which are not the same in Scala 2.12. However, AFAICT, no standard class we'd expect here doesn't extend Iterable. See https://docs.scala-lang.org/overviews/collections/overview.html ; Seq, Set and Map are also Iterable already. This shouldn't change behavior, but tests will help prove that.
|
The internal change is easy to understand. |
|
Test build #105327 has finished for PR 24584 at commit
|
|
BTW we have a bigger related issue: |
| /** | ||
| * Set multiple parameters together | ||
| */ | ||
| @deprecated("Use setAll(Iterable) instead", "3.0.0") |
There was a problem hiding this comment.
Hm, is it even worth keeping and deprecating this and removing it later? It'll be source compatible already and binary compatibility isn't guaranteed; one would have to go out of one's way to call the deprecated method at all, with a cast or with some odd custom collection
There was a problem hiding this comment.
One reason I put this forth is that the TraversableOnce problem is harder; its replacement IterableOnce doesn't exist in Scala 2.12. The same overload strategy doesn't work out quite right for the places it's used (generic types and function signatures make the overload ambiguous). I am considering proposing just changing the type there for 3.0.
There was a problem hiding this comment.
I can see setAll() is being used though. but maybe it won't work even after a rebuild (bin compat)?
There was a problem hiding this comment.
Calls to setAll will now bind to the more specific Iterable arg version in all cases in Spark, and I'm not aware of any Scala classes that aren't Iterable to begin with. One consequence I suppose is that passing an Iterator will use the deprecated method. But it'll still be there. I don't anticipate we'll remove it until we want to support Scala 2.13 and that won't be in 3.0. For now, no method is removed so should be no problem.
|
Merged to master |
6 participants
What changes were proposed in this pull request?
This removes usage of
Traversable, which is removed in Scala 2.13. This is mostly an internal change, except for the change in theSparkConf.setAllmethod. See additional comments below.How was this patch tested?
Existing tests.