-
Notifications
You must be signed in to change notification settings - Fork 321
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
Table.group_by #3305
Table.group_by #3305
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me.
Some style comments and questions but given it's a prototype, most are just suggestions for the future.
The important thing is the Group_By_Key
- IMO it should not repeat the implementations of existing functions if it can delegate to them unless it is really absolutely necessary.
distribution/lib/Standard/Table/0.0.0-dev/src/Data/Aggregate_Column.enso
Outdated
Show resolved
Hide resolved
distribution/lib/Standard/Table/0.0.0-dev/src/Data/Aggregate_Column.enso
Outdated
Show resolved
Hide resolved
- population argument specifies if group is a sample or the population | ||
type Standard_Deviation (column:Column|Text|Integer) (name:Text|Nothing=Nothing) (population:Boolean=False) | ||
|
||
## Creates a new column with the values concatenated together. NULL values will become an empty string. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Btw. why is NULL collated with ""
here instead of being ignored like in other cases ?
distribution/lib/Standard/Table/0.0.0-dev/src/Data/Aggregate_Column.enso
Outdated
Show resolved
Hide resolved
distribution/lib/Standard/Table/0.0.0-dev/src/Data/Aggregate_Column.enso
Show resolved
Hide resolved
distribution/lib/Standard/Table/0.0.0-dev/src/Data/Group_By.enso
Outdated
Show resolved
Hide resolved
distribution/lib/Standard/Table/0.0.0-dev/src/Data/Group_By.enso
Outdated
Show resolved
Hide resolved
distribution/lib/Standard/Table/0.0.0-dev/src/Data/Group_By.enso
Outdated
Show resolved
Hide resolved
distribution/lib/Standard/Table/0.0.0-dev/src/Data/Group_By.enso
Outdated
Show resolved
Hide resolved
array = new_table.at (j + key_length) . at 1 . to_array | ||
current = array . at row_index | ||
new = aggregator current i | ||
array . set_at row_index new |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This may stop working when we fix the "to_array
is leaking mutability" issue.
As this prototype may live shorter than the above issue - probably ignore this comment.
But just for future - maybe we could just create an Expandable_Array
which wraps a regular, explicitly-mutable array but also has the append
operation and does the allocations that give us O(1) amortized complexity of append
. Because I don't like that we are abusing the Vector_Builder
a bit when we could just create a data structure meant for such use-cases. But since this is a prototype - just commenting a suggestion for future.
81746c2
to
2f74a42
Compare
Handle no key
Use first/second Expanding tests
Sum of all Nothing == Nothing StDev calculation fixes
Support for Booleans and Nothing in the keys
434a021
to
9f99739
Compare
distribution/lib/Standard/Table/0.0.0-dev/src/Data/Aggregate_Column.enso
Outdated
Show resolved
Hide resolved
distribution/lib/Standard/Table/0.0.0-dev/src/Data/Group_By.enso
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me
Pull Request Description
Functioning group_by based of Enso Map.
Important Notes
This is an initial version which will be used to establish the API.
The grouping map will need to be moved to Java code for performance.
Checklist
Please include the following checklist in your PR:
./run dist
and./run watch
.