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 q12, q13, q14, q16, q22 #910

Merged
merged 9 commits into from
Sep 7, 2024

Conversation

luke396
Copy link
Member

@luke396 luke396 commented Sep 5, 2024

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.

Not done for now, some fix needed.

@github-actions github-actions bot added the enhancement New feature or request label Sep 5, 2024
.join(supplier, left_on="ps_suppkey", right_on="s_suppkey", how="left")
.filter(nw.col("ps_suppkey_right").is_null())
.group_by("p_brand", "p_type", "p_size")
.agg(nw.col("ps_suppkey").unique().len().alias("supplier_cnt"))
Copy link
Member Author

Choose a reason for hiding this comment

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

unique().len() changed from n_unique()

Because AttributeError: 'SeriesGroupBy' object has no attribute 'n_unique'.

Copy link
Member

Choose a reason for hiding this comment

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

thanks @luke396 ! looks like a bug, will try to resolve beforehand #915

Copy link
Member

Choose a reason for hiding this comment

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

I made an attempt in #917, I don't have time to test against the queries right now though πŸ™ˆ

@luke396
Copy link
Member Author

luke396 commented Sep 6, 2024

All works well, except q16 and q22 raise UserWarning: Found complex group-by expression, which can't be expressed efficiently with the pandas API. If you can, please rewrite your query such that group-by aggregations are simple (e.g. mean, std, min, max, ...).

@luke396 luke396 marked this pull request as ready for review September 6, 2024 04:45
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 @luke396 !

tpch/queries/q22.py Outdated Show resolved Hide resolved
.join(supplier, left_on="ps_suppkey", right_on="s_suppkey", how="left")
.filter(nw.col("ps_suppkey_right").is_null())
.group_by("p_brand", "p_type", "p_size")
.agg(nw.col("ps_suppkey").unique().len().alias("supplier_cnt"))
Copy link
Member

Choose a reason for hiding this comment

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

thanks @luke396 ! looks like a bug, will try to resolve beforehand #915

tpch/queries/q16.py Outdated Show resolved Hide resolved
tpch/queries/q22.py Outdated Show resolved Hide resolved
@luke396
Copy link
Member Author

luke396 commented Sep 7, 2024

As observed in #918, we have remove pandas and polars[eager], I've done the same in this current pull request.

(Not sure if this is appreciated.)

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.

awesome, thanks so much @luke396 !

yeah I cut down a bit on what gets executed to save some time (as you can intuit, I'm a bit impatient with waiting for things to run πŸ˜„ )

@MarcoGorelli MarcoGorelli merged commit 62c8ada into narwhals-dev:main Sep 7, 2024
22 checks passed
@luke396 luke396 deleted the add-new-query branch September 7, 2024 11:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request full-test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants