dfdx / Spark.jl

Julia binding for Apache Spark
Other
205 stars 39 forks source link

Some fixes towards a working version for Julia v1.0 #61

Closed simonschoelly closed 6 years ago

simonschoelly commented 6 years ago

This fixes a lot of stuff for Julia v1.0 but you should definitely have a look at my changes, to make sure that I did not introduce new errors. I divided the tests into testsets and there is a single one that still fails: the one for flat_map. If I understand the problem correctly, the server somehow enters back an empty collection (or that happens when parsing the output from the server locally). Then the local reduce function fails, when trying to reduce over an empty collection.

dfdx commented 6 years ago

Wow, that was a huge work, thanks! Most of the code looks fine. I'm a bit suspicious about new iterator protocol since I had particular issues with previous version in Julia/Java interaction part, but I need to dive into it to make sure. I think I'll have time for it by the end of this week.

simonschoelly commented 6 years ago

Yeah, I also find the new iterator protocol not very intuitive. I saw that there is actually a flatten iterator in Base.Iterators.flatten so maybe you could just use this one instead of having your own implementation.

aviks commented 6 years ago

Yes, just use Base.Iterators.flatten, and get rid of the custom FlatMapIterator. The latter was useful on 0.5, since the base versions did not work with Tasks and Channels on 0.5 (due to done not being idempotent). In Julia 0.6 and later, the Base versions should work well.

There may be other simplifications that we can do due to this -- I remember reduce being overly complicated for this same reason.

dfdx commented 6 years ago

Since GitHub doesn't allow pushing to someone else's pull request, I've created #62. All the tests pass locally on Julia 0.7, but Travis now seems to be lacking maven. I'm going to spend some more time on CI, but won't block this update if Spark.jl is confirmed to work on at least one more user's system.

Closing this issue to move discussion to #62. @simonschoelly Thanks a lot for your great work!