-
Notifications
You must be signed in to change notification settings - Fork 605
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(pyspark): run pre-execute hooks for to_delta
#8848
fix(pyspark): run pre-execute hooks for to_delta
#8848
Conversation
|
0960a68
to
5026500
Compare
ibis/backends/tests/test_export.py
Outdated
@@ -311,6 +313,7 @@ def test_table_to_csv(tmp_path, backend, awards_players): | |||
reason="cannot inline WriteOptions objects", | |||
raises=DuckDBParserException, | |||
) | |||
@pytest.mark.never(["pyspark"], reason="backend writes a dir", raises=IsADirectoryError) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this result in effectively no testing for PySpark to_csv
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No; I added equivalent tests in ibis/backends/pyspark/tests/test_export.py.
Apologies for the drive by comment (I'm on leave, so don't block merging this on me). IMO we do want to figure out a consistent story around the artifacts produced by |
@deepyaman Thanks again for opening this PR! We were able to write a work around in our use case so this isn't a rush for us although it would be nice so that we don't have to continue maintaining a work around... On a separate note, while implementing our work around I tried to use
I'm just sharing this in case it's an actual issue. I'm using pyspark 3.3.4 & ibis 8.0.0. Below is a reproducible example.
Error
|
@deepyaman Is this PR still viable? @chloeh13q Implemented the various |
I'll double-check tomorrow! |
@cpcloud @chloeh13q I think most things are covered, but the changes to |
@deepyaman Yeah, let's wrap up the |
5026500
to
635d98c
Compare
to_delta
Pared it down to just that change. |
@mark-druffel needs Spark-native export functionality. We had put off implementing the fast path because we were trying to figure out if we could unify output options, but we should probably add the fast path until we figure out a better alternative.