Skip to content

Conversation

@marvinl803
Copy link
Contributor

@marvinl803 marvinl803 commented Mar 19, 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

This is a follow up to issue #1989 where it adds support to the Binary datatype. If there is any further changes needed, please let me know.

@dangotbanned dangotbanned changed the title Added Binary dtype support. feat: Binary dtype support Mar 19, 2025
@FBruzzesi FBruzzesi marked this pull request as ready for review March 19, 2025 09:43
@dangotbanned dangotbanned added the enhancement New feature or request label Mar 19, 2025
Copy link
Member

@FBruzzesi FBruzzesi left a comment

Choose a reason for hiding this comment

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

Thanks @marvinl803 πŸš€

I added a couple of commits to support spark-like as well and fix a typo in the docstrings.

I will wait for someone else to review this as I am a bit biased πŸ˜‡

In the meanwhile, I opened a PR in py-shiny to add binary support there as well

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.

thanks all!

@MarcoGorelli
Copy link
Member

@FBruzzesi feel free to ship!

@marvinl803
Copy link
Contributor Author

Thanks @marvinl803 πŸš€

I added a couple of commits to support spark-like as well and fix a typo in the docstrings.

I will wait for someone else to review this as I am a bit biased πŸ˜‡

In the meanwhile, I opened a PR in py-shiny to add binary support there as well

FBruzzesi

Thank you as well, @FBruzzesi I really appreciate it πŸ™Œ!

@FBruzzesi FBruzzesi merged commit 319c9d1 into narwhals-dev:main Mar 19, 2025
29 of 30 checks passed
Comment on lines +337 to +342
def test_cast_binary(request: pytest.FixtureRequest, constructor: Constructor) -> None:
if "pandas" in str(constructor) and PANDAS_VERSION < (2, 2):
request.applymarker(pytest.mark.xfail)

if any(backend in str(constructor) for backend in ("dask", "modin")):
request.applymarker(pytest.mark.xfail)
Copy link
Member

Choose a reason for hiding this comment

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

@marvinl803 could we also skip this test for cudf please?

Copy link
Member

Choose a reason for hiding this comment

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

nevermind, just did it - thanks again for your pr!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My bad, I just saw this. Absolutely no problem at all!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

dtypes enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Enh]: Add support for Binary & Time datatype

4 participants