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.

Sign up for GitHub

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

Jump to bottom

[DOCS] Naming consistency of length functions #2942

Merged
merged 7 commits into from
Oct 5, 2024
Merged

[DOCS] Naming consistency of length functions #2942

merged 7 commits into from
Oct 5, 2024

Conversation

vicky1999
Copy link
Contributor

Solves #2769

  • Added length function to Expression.list
  • Added deprecation warning to Expression.list.lengths

@github-actions github-actions bot added documentation Improvements or additions to documentation enhancement New feature or request labels Sep 26, 2024
@vicky1999 vicky1999 changed the title [FEAT] Naming consistency of length functions [DOCS] Naming consistency of length functions Sep 26, 2024
@codspeed-hq CodSpeed HQ
Copy link

codspeed-hq bot commented Sep 26, 2024 •
edited
Loading

CodSpeed Performance Report

Merging #2942 will not alter performance

Comparing vicky1999:vicky1999/#2769-list-length (1664237) with main (53a84ea)

Summary

✅ 17 untouched benchmarks

@kevinzwang kevinzwang self-requested a review September 27, 2024 18:38
kevinzwang
kevinzwang reviewed Sep 30, 2024
View reviewed changes
Copy link
Member

@kevinzwang kevinzwang left a comment

Choose a reason for hiding this comment

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

Thanks for making these changes! In addition to my comments, could you also change all of our existing tests and docs that use Expression.list.lengths?

  • test/series/test_cast.py
  • test/table/list/test_list_count_lengths.py (perhaps rename to test_list_count_length.py)
  • tutorials/delta_lake/2-distributed-batch-inference.ipynb

daft/expressions/expressions.py Outdated Show resolved Hide resolved
docs/source/api_docs/expressions.rst Outdated Show resolved Hide resolved
@vicky1999
Copy link
Contributor Author

@kevinzwang Changes were made as per the comments. Let me know if there are any other changes needed.

@vicky1999
Copy link
Contributor Author

@kevinzwang
we also have lengths function in series. So, created the length function and made lengths deprecated.

@kevinzwang kevinzwang self-requested a review October 4, 2024 19:37
kevinzwang
kevinzwang approved these changes Oct 4, 2024
View reviewed changes
Copy link
Member

@kevinzwang kevinzwang left a comment

Choose a reason for hiding this comment

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

Looks great! Let me know once you've resolved the one comment I had and I can merge this.

daft/expressions/expressions.py Outdated
Comment on lines 2938 to 2942
"""Gets the length of each list

Returns:
Expression: a UInt64 expression which is the length of each list
"""
Copy link
Member

Choose a reason for hiding this comment

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

Could you replace this docstring with something along the lines of

"(DEPRECATED) Please use Expression.list.length instead"

@vicky1999
Copy link
Contributor Author

@kevinzwang Docstring of lengths function is updated as per your comments.

@kevinzwang
Copy link
Member

@vicky1999 enabling auto-merge. Thank you for this contribution!

@kevinzwang kevinzwang enabled auto-merge (squash) October 5, 2024 04:25
@kevinzwang kevinzwang merged commit edeee9e into Eventual-Inc:main Oct 5, 2024
36 checks passed
sagiahrac pushed a commit to sagiahrac/Daft that referenced this pull request Oct 7, 2024
@vicky1999 @sagiahrac
[DOCS] Naming consistency of length functions (Eventual-Inc#2942)
7467ba3 
Solves Eventual-Inc#2769 

- Added `length` function to Expression.list
- Added deprecation warning to `Expression.list.lengths`
@colin-ho colin-ho mentioned this pull request Oct 7, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kevinzwang kevinzwang kevinzwang approved these changes

Assignees
No one assigned
Labels
documentation Improvements or additions to documentation enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@vicky1999 @kevinzwang