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 drop_nulls, fill_null, and filter Spark Expressions #1802

Open
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

lucas-nelson-uiuc
Copy link
Contributor

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

Added support for the following methods - made sure to remove all pyspark constructor checks in the main test suite:

  • drop_nulls
  • fill_null
    • noticed the only strategies currently supported in nw.Expr.fill_null are "forward" and "backward" - are there plans to add more strategies (possibly in v2)?
  • filter (work in progress)
    • solution works fine in PySpark but doesn't carry over to narwhals (pyspark.sql.function.filter expects the predicate to return a Column; however, it raises an error since _from_call returns SparkLikeExpr).
    • lmk if I'm missing something basic

@MarcoGorelli
Copy link
Member

thanks for your pr, really appreciate it!

to be honest i'm not totally sold on having expressions which change length but don't aggregate be supported in the lazy api

we may be able to support them, but only if they're followed by an aggregations, so they're kinda like the equivalent of sql's

select sum(a) filter (where b > 1)
from rel

Will get back to you on this anyway, but for now we may need to punt on these methods, really sorry

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants