⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 42.85714% with 4 lines in your changes missing coverage. Please review.

Project coverage is 90.35%. Comparing base (bc487ef) to head (fc8ebae).
Report is 7 commits behind head on master.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

@@            Coverage Diff             @@
##           master     #506      +/-   ##
==========================================
- Coverage   91.43%   90.35%   -1.09%     
==========================================
  Files          18       18              
  Lines         829      902      +73     
  Branches       52       96      +44     
==========================================
+ Hits          758      815      +57     
- Misses         71       87      +16     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@rjurney Hello!

At the moment PR provides a (mostly) working version of GrahFrames API for PySpark Connect. There are a lot of open questions from my side:

• I would like to change tests to work for both connect and classic but it can be tricky because current tests are very old; at the moment I just copied the whole tests suite and slightly modified it;
• I do not know what is the best way to integrate Connect API to the current PySpark API: at the moment I implemented everything as a separate GraphFrameConnect class that has (almost) the same API like the GraphFrame;

How to try it: run the script in dev folder. Connect to the Spark Connect Server with SparkSession.builder.remote("sc://localhost:15002").getOrCreate() and use graphframes.connect.graphframes_client.GraphFrameConnect.

@rjurney Hello!

At the moment PR provides a (mostly) working version of GrahFrames API for PySpark Connect. There are a lot of open questions from my side:

• I would like to change tests to work for both connect and classic but it can be tricky because current tests are very old; at the moment I just copied the whole tests suite and slightly modified it;
• I do not know what is the best way to integrate Connect API to the current PySpark API: at the moment I implemented everything as a separate GraphFrameConnect class that has (almost) the same API like the GraphFrame;

How to try it: run the script in dev folder. Connect to the Spark Connect Server with SparkSession.builder.remote("sc://localhost:15002").getOrCreate() and use graphframes.connect.graphframes_client.GraphFrameConnect.

My PR #473 converts the unit tests to pytest tests. That should help there. My PR is a monster, I will work on breaking it up into pieces tomorrow and that should give you the ability to pull the test-related code into your PR.

It would be nice if there wasn't a completely different API for Spark Connect. How different are the implementations? I'll take a look at the PR tomorrow and see.

This is very cool work, thanks for it!

It would be nice if there wasn't a completely different API for Spark Connect. How different are the implementations? I'll take a look at the PR tomorrow and see.

It is the same, just implemented as a separate class (python/graphframes/connect/graphframe_client.py)

Is it feasible to add connect support to GraphFrame? Especially if it could detect a Connect connection and 'just work' or take an argument?

Is it feasible to add connect support to GraphFrame? Especially if it could detect a Connect connection and 'just work' or take an argument?

Of course. That is the idea, but the question is how to do it? In Spark 3.x devs made a decision to make two versions of DataFrame (pyspark.sql.DataFrame and pyspark.sql.connect.dataframe.DataFrame). In the Spark 4.x it is changed and there are three versions at the moment:

• Classic DataFrame: https://github.com/apache/spark/blob/master/python/pyspark/sql/classic/dataframe.py
• Connect DataFrame: https://github.com/apache/spark/blob/master/python/pyspark/sql/connect/dataframe.py
• DataFrame that contains dispatch mechanism: https://github.com/apache/spark/blob/master/python/pyspark/sql/dataframe.py

I did not want to touch the existing code in this PR. I would like to finalize the implementation as a standalone GraphFrameConnect class that has exactly the same API as an existing GraphFrame. And we can deal with an integration in the next iteration.

What do you think about it?
@rjurney

To be honest, I like the idea of dispatch like in PySpark, but it will require big changes in the current GraphFrame, tests, build, etc. I think it would be better to do in GraphFrames 1.0 because I would like to slightly change the public API.

For example, due to Spark Connect limitations it maybe tricky to return both loss and DataFrame from svdPlusPlus. I would like to change the signature and do not return the loss by default. The same about PageRank: I would like to make computation of the edge weights optional with default to False.

Is it feasible to add connect support to GraphFrame? Especially if it could detect a Connect connection and 'just work' or take an argument?

Of course. That is the idea, but the question is how to do it? In Spark 3.x devs made a decision to make two versions of DataFrame (pyspark.sql.DataFrame and pyspark.sql.connect.dataframe.DataFrame). In the Spark 4.x it is changed and there are three versions at the moment:

• Classic DataFrame: https://github.com/apache/spark/blob/master/python/pyspark/sql/classic/dataframe.py
• Connect DataFrame: https://github.com/apache/spark/blob/master/python/pyspark/sql/connect/dataframe.py
• DataFrame that contains dispatch mechanism: https://github.com/apache/spark/blob/master/python/pyspark/sql/dataframe.py

I did not want to touch the existing code in this PR. I would like to finalize the implementation as a standalone GraphFrameConnect class that has exactly the same API as an existing GraphFrame. And we can deal with an integration in the next iteration.

What do you think about it? @rjurney

Ohhhh wow, okay. Now this all makes sense :) I would not have thought that. Everything makes sense now. I think you're doing what makes sense.

Questions / Topics

1. Should we store the generated python code in the repository or not? Apache Spark is storing python code but JVM code is generated during the build. At the moment, my PR follows the way of Apache Spark.
2. Should we have a separate GraphFramesConnect or should we use runtime dispatch instead? At the moment my implementation offers a separate class, but Apache Spark uses dispatch.
3. Should a GraphFrames Connect be a part of graphframes distribution or should we provide a separate JAR instead?
4. Should be a Python Connect client be a part of graphframes distribution, or should it be an extra (like pip install graphframes[connect])?

@SemyonSinchenko thanks for your work here, this is just awesome. I am going to give it a thorough review this weekend, sooner if I can. Will have questions borne of my lack of Scala knowledge as much as anything :)

@rjurney What is the plan for the review? Will you review this changes, or should I try to ask someone else to review this?

@SemyonSinchenko TODAY

@SemyonSinchenko can you describe any changes to the user experience of GraphFrames? Today I am using databricks-connect via VSCode for PySpark on Databricks and I can't use GraphFrame.pageRank because I am on Connect. Will it work with this PR? What about checkpointing code? That doesn't work either. Just wondering.

@SemyonSinchenko can you describe any changes to the user experience of GraphFrames? Today I am using databricks-connect via VSCode for PySpark on Databricks and I can't use GraphFrame.pageRank because I am on Connect. Will it work with this PR? What about checkpointing code? That doesn't work either. Just wondering.

@rjurney

For classic users nothing should changes, only some minor / questinable things. For example, I introduced the build of JAR to the process of building the graphframes PySpark. Maybe I should remove it. Should I?

For connect users it is different. They should add graphframes-connect JAR to the Spark Connect Server part and add a spark conf like spark.connect.extensions.relation.classes=org.apache.spark.sql.graphframes.GraphFramesConnect to configuration of their Spark Connect Server. That is how the connect plugin system is supposed to be used.

Answers:

1. Will it work on Databricks? Most probably not, but I cannot be sure for 100%. The story is that Databricks Spark is a fork of Apache Spark and it does not match 100% to the Apache Spark. As I can assume based on the delta-io code for PySpark Connect, guys from Databricks backported the signature of plugins from the Spark 4.0 to their branch for DBRs 14.x and 15.x. There is a strong reason for that, the same reason why it was changed from 3.x to 4.x. But my code is targeting Apache Spark 3.4.x and 3.5.x with a workaround, related to the shading rules, to avoid the problem with plugins. So, in my understanding, even with my plugin, GraphFrames won't work on DBR 14.x and DBR 15.x (Spark 3.5.x), but I'm not 100% sure. My question is, should we maintain the DBR compatibility? And if so, how to do it without an access to the source code of DBRs? At the moment there is even no documentation about how to add Spark Connect plugins to the Databricks clusters and you should provide the right configuration before the start of runtime (I'm assuming that the only way is by init scripts that have terrible documentation).
2. I workarounded the checkpoint problem by the usage of the spark.checkpoint.dir variable. You can see it, for example, in these lines. For classic the behavior is the same.

@rjurney JFYI. Because I know about the breaking change in connect plugins system from Apache Spark 3.5.x to 4.0.x (the change, that in my understanding is already in the DBR 14.x and 15.x), I separated all the plugins logic from the plugin itself: the plugin itself is less than five logical lines of code to simplify the migration (and DBR shim) as much as possible. In Apache Spark 4.0.x the signature is changed to Optional<LogicalPlan> transform(byte[] relation, SparkConnectPlanner planner); and my workaround with shading of GraphFrames proto message is not needed anymore.

@SemyonSinchenko so is it ready for review then?

@SemyonSinchenko so is it ready for review then?

@rjurney Yes, it is. I still need to resolve merge conflicts, but 99% of them are related to the tests, not to the logic. Also one additional method will be introduced to wrap also a recently added power iteration clustering.

@rjurney I resolved all the conflicts.

An important changes you should check:

• I added the GraphFrames JAR to the python package to avoid JVM-errors;
• I moved tests to python/tests to avoid storing them inside the package;
• I significantly simplify tests by removing all the artifacts of unittests;

At the moment only part of tests are turned on for Connect because other part of tests is based on the examples that are called through py4j.

Wow, awesome! I'll review it now.

@SemyonSinchenko regarding the JAR, I think it is right to include it and think we should add it to MANIFEST.in.