Skip to content
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

feat: add missing dunder methods in SparkLikeExpr and SparkLikeNamespace.lit #1708

Merged
merged 6 commits into from
Jan 9, 2025

Conversation

EdAbati
Copy link
Collaborator

@EdAbati EdAbati commented Jan 2, 2025

What type of PR is this? (check all applicable)

  • πŸ’Ύ Refactor
  • ✨ Feature
  • πŸ› Bug Fix
  • πŸ”§ Optimization
  • πŸ“ Documentation
  • βœ… Test
  • 🐳 Other

Related issues

Checklist

  • Code follows style guide (ruff)
  • Tests added
  • Documented the changes

If you have comments or can explain your changes, please do so below

@@ -145,32 +145,136 @@ def func(df: SparkLikeLazyFrame) -> list[Column]:

def __add__(self, other: SparkLikeExpr) -> Self:
return self._from_call(
lambda _input, other: _input + other,
lambda _input, other: _input.__add__(other),
Copy link
Collaborator Author

@EdAbati EdAbati Jan 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed to use the Column dunder methods as we do in Dask

@EdAbati EdAbati marked this pull request as ready for review January 2, 2025 14:39
@MarcoGorelli
Copy link
Member

thanks @EdAbati ! we recently updated the returns_scalars in dask in https://github.com/narwhals-dev/narwhals/pull/1684/files, should we do the same here in pyspark?

@EdAbati
Copy link
Collaborator Author

EdAbati commented Jan 2, 2025

Oh interesting ! I will make a separate PR that we can merge before this one

@MarcoGorelli
Copy link
Member

we can also remove the __r*__ definitions now 😎

@EdAbati EdAbati changed the title feat: add dunder methods in SparkLikeExpr feat: add missing dunder methods in SparkLikeExpr and SparkLikeNamespace.lit Jan 8, 2025
Comment on lines +72 to +73
if dtype is not None:
msg = "todo"
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need narwhals_to_native_dtype for Spark.

Sitting on my laptop there is an oldish branch with cast that has narwhals_to_native_dtype.
Can this be a follow up?

@EdAbati
Copy link
Collaborator Author

EdAbati commented Jan 8, 2025

Hey @MarcoGorelli I had a look at adding binary_operation_returns_scalar in Spark too. It takes a bit more time than expected I may have to look into it this weekend or next week.

Would it be ok to have it as a follow up?

Copy link
Member

@MarcoGorelli MarcoGorelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yup, sure! thanks @EdAbati , merging then so we can unblock the other pr

@MarcoGorelli MarcoGorelli added the enhancement New feature or request label Jan 9, 2025
@MarcoGorelli MarcoGorelli merged commit 0f38521 into narwhals-dev:main Jan 9, 2025
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants