Feat/untangling imports #161
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- change of name from `stat_counter.py` to `statcounter.py` as that's the name in pyspark. - Moved as much as I could into 'terminals'. Meaning modules which are _not_ depending on any others except for externals to pysparkling. There's still a lot to do here! - Moved stuff out from pysparkling into pysparkling.sql because the SQL code just does not belong in the root! - The most glaring example of this is the method `toDF` which is moved to sql.session.py and is being monkey-patched. Just like pyspark is doing it. (don't re-invent the wheel :)). - Moved stuff to private (to pysparkling) modules (modules starting with `_`) which are not defined in pyspark. This helped to reduce the complexity a lot.
- change of name from `stat_counter.py` to `statcounter.py` as that's the name in pyspark. - Moved as much as I could into 'terminals'. Meaning modules which are _not_ depending on any others except for externals to pysparkling. There's still a lot to do here! - Moved stuff out from pysparkling into pysparkling.sql because the SQL code just does not belong in the root! - The most glaring example of this is the method `toDF` which is moved to sql.session.py and is being monkey-patched. Just like pyspark is doing it. (don't re-invent the wheel :)). - Moved stuff to private (to pysparkling) modules (modules starting with `_`) which are not defined in pyspark. This helped to reduce the complexity a lot.
- Added a script to compare pyspark & pysparkling to see what still needs to be implemented.
Report:I will commit this in a future commit, but maybe we can discuss what the direction would be? For me I'd like to have the public interface of pysparkling the same as pyspark. And any internal details handled in "Problem" I see is when pyspark is changing, and if we would want to focus our efforts on maintaining an all-inclusive library. For me, I need compatibility with pyspark 2.3.2 at the very least. Files that should not be exposed:
REPORT: pyspark vs pysparkling
|
|
I'm also thinking: should the private interface be implemented or not? I would say no, because it is subject to change at any time. But there are some things that we do at our company that can only be done via the private interface... So that would be making my life more difficult there. I think I'll just concentrate on the public one for now. |
|
Because of the untangled imports I could now also add my long standing dream of auto-injection. A proof-of-concept has been added in the latest commit (29a7d99). The program itself does not change one bit, but the execution happens via pypark or via pysparkling: vs |
|
|
||
|
|
||
| class SparkContext: | ||
| """Accepts the same initialization parameters as pyspark, but redirects everything to Context.""" |
There was a problem hiding this comment.
Why not subclassing Context instead?
There was a problem hiding this comment.
Because I actually wanted to make SparkContext a singleton. And as far as I can see, Context is not a singleton.
PlusI went a far way out to try to copy the pyspark API into SparkContext, but finally ended up with the current implementation.
Long story short: inheriting & checking for single use in new should be better :-). I updated the code as such.
| @@ -19,6 +20,8 @@ | |||
|
|
|||
| log = logging.getLogger(__name__) | |||
|
|
|||
| __all__ = ['SparkContext'] | |||
There was a problem hiding this comment.
It is a rather big change in pysparkling API which we may want to discuss, it may not be aligned with the previous vision that were the API was on purpose different as pyspark (namely here because it is not a SparkContext that is created, it's a pysparkling context)
There was a problem hiding this comment.
Ok, needs to be discussed than. For me, I want it to be a drop-in replacement for pyspark (during tests). As such it would need to be 100% aligned with the pyspark API.
@svenkreiss , @tools4origins : what is your view on this?
I also refer to the auto-injector: https://github.com/svenkreiss/pysparkling/pull/161/files#diff-b6988b98630fdeab52d583b68acee7ba2d41304b72a201045c0c141c7fd502dc
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
@tools4origins : you're not going to like this one when merging :(. Sorry about that!
Cleanup of imports.
stat_counter.pytostatcounter.pyas that's the name in pyspark.toDFwhich is moved to sql.session.py and is being monkey-patched. Just like pyspark is doing it. (don't re-invent the wheel :))._) which are not defined in pyspark. This helped to reduce the complexity a lot.What I'm trying to achieve here is to reduce the dependency hell of the "SQL" module.
Not much code has been changed, some code copied from pyspark to make it easier, but basically moving methods around into different files.
I also started here to make a distinction between pysparkling internals and the pyspark interface. What I mean with that is that pyspark has a certain file structure, this I kept as rigorous as possible (it was already largely that way anyhow), But pysparkling specific methods I tried to move into "private" modules (_types, _casts, _expressions, ...).