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

Closes #1792 - GroupBy Symbol Table Entry #1805

Merged
merged 9 commits into from
Oct 13, 2022

Conversation

Ethan-DeBandi99
Copy link
Contributor

Closes #1792

  • Adds a Symbol Table Entry for GroupBy objects. These are only created currently in the case when keys is provided. If all elements are provided, the object remains client side. Issue GroupBy Creation from_parts #1803 addresses adding support creation from all components.
  • GroupBySymEntry inherits from GenSymEntry in preparation for removing CompositeSymEntry. CompositeSymEntry currently does not add any information that is not found in GenSymEntry and does not appear necessary.
  • Updates the client side constructor to send a server message to create the GroupBy object.
  • Adds createGroupBy message.
  • Currently, the unique_keys property is not stored on the server. Issue GroupBy.unique_keys Server Side Computation #1804 will address computing Unique Keys on the server.

This code utilizes components of Unique, but does not require the full workflow at this time.

@Ethan-DeBandi99
Copy link
Contributor Author

Converting to draft until I resolve the bug in gasnet.

Also, realized I forgot to merge in updates to master and will do that as well.

@Ethan-DeBandi99 Ethan-DeBandi99 marked this pull request as draft September 26, 2022 22:12
@Ethan-DeBandi99
Copy link
Contributor Author

Adding a note that #1807 is needed before merging this.

@Ethan-DeBandi99 Ethan-DeBandi99 marked this pull request as ready for review October 3, 2022 11:31
Copy link
Contributor

@joshmarshall1 joshmarshall1 left a comment

Choose a reason for hiding this comment

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

Only one thing really stood out to me, but not a blocker. The rest looked good.

arkouda/groupbyclass.py Outdated Show resolved Hide resolved
…will be updated in the future to remove the dtype storage requirement. Currently only configured to build the GroupBy object from keys. Providing all elements is still fully client side, but will be moved in the future.
Copy link
Member

@stress-tess stress-tess left a comment

Choose a reason for hiding this comment

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

Looks good! The only thing that might need changed is the getSizeEstimate

arkouda/groupbyclass.py Outdated Show resolved Hide resolved
arkouda/groupbyclass.py Show resolved Hide resolved
src/MultiTypeSymEntry.chpl Outdated Show resolved Hide resolved
@stress-tess stress-tess merged commit e4a0239 into Bears-R-Us:master Oct 13, 2022
@Ethan-DeBandi99 Ethan-DeBandi99 deleted the 1792_groupbysymentry branch October 14, 2022 14:19
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.

GroupBySymEntry Creation
3 participants