chore: move pyspark tests into main test suite #1761
Conversation
| expr = plx.all_horizontal( | ||
| *chain(predicates, (plx.col(name) == v for name, v in constraints.items())) | ||
| ) |
There was a problem hiding this comment.
Needed to implement Expr.__eq__ to get this to work. It overlaps with @EdAbati PR
| @@ -62,7 +62,7 @@ def test_scalar_reduction_with_columns( | |||
| expected: dict[str, list[Any]], | |||
| request: pytest.FixtureRequest, | |||
| ) -> None: | |||
| if "duckdb" in str(constructor): | |||
| if "duckdb" in str(constructor) or ("pyspark" in str(constructor)): | |||
There was a problem hiding this comment.
Only one of the 5 tests is passing (leading to [XPASS(strict)])
There was a problem hiding this comment.
awesome @FBruzzesi - not sure what's happening with tests in running in ci?
We are not installing pyspark at all, therefore but now |
|
ah i see - maybe |
tests/conftest.py
Outdated
| if constructor == "pyspark": | ||
| if sys.version_info < (3, 12): | ||
| constructors.append(pyspark_lazy_constructor()) | ||
| else: | ||
| continue |
There was a problem hiding this comment.
@MarcoGorelli maybe this is too much? 🙈
There was a problem hiding this comment.
with pyspark 4.0.0 this would go 🤞
| module="pyspark", | ||
| category=DeprecationWarning, | ||
| ) | ||
| pd_df = pd.DataFrame(obj).replace({float("nan"): None}).reset_index() |
There was a problem hiding this comment.
If the objects that come into these constructors are (always?) dictionaries I think we can skip the trip through pandas and construct from a built-in Python object that spark knows how to ingest directly (list of dictionaries). Could be overly cautions, but Spark may infer data types differently if it is handed a pandas DataFrame rather than lists of Python objects.
Since pyspark supports a list of records we could convert dict → list of dicts like so
if isinstance(obj, dict):
obj = [{k: v for k, v in zip(obj, row)} for row in zip(*obj.values())]Or could pass in the rows & schema separately
if isinstance(obj, dict):
df = ...createDataFrame([*zip(*obj.values())], schema=[*obj.keys()])There was a problem hiding this comment.
I remember having issues with some tests, where we may need to specify the schema with column type. (but I don't remember exactly what was the problem)
But if we can skip pandas here, it would be 👌👌👌
There was a problem hiding this comment.
I had the same thought when migrating the codebase, yet I can confirm the data type being an issue for a subset of the tests. I would say to keep it like this for now and eventually address it
| yield session | ||
| session.stop() | ||
|
|
||
| register(session.stop) |
There was a problem hiding this comment.
TIL atexit.register, nice!
tests/conftest.py
Outdated
| if constructor == "pyspark": | ||
| if sys.version_info < (3, 12): | ||
| constructors.append(pyspark_lazy_constructor()) | ||
| else: | ||
| continue |
There was a problem hiding this comment.
with pyspark 4.0.0 this would go 🤞
| 'ignore:.*The distutils package is deprecated and slated for removal in Python 3.12:DeprecationWarning:pyspark', | ||
| 'ignore:.*distutils Version classes are deprecated. Use packaging.version instead.*:DeprecationWarning:pyspark', |
There was a problem hiding this comment.
@MarcoGorelli I moved these back to pyproject.toml, yet targeting pyspark module. Would that work for you?
What type of PR is this? (check all applicable)
Related issues
Checklist
If you have comments or can explain your changes, please do so below