Histogram Aggregation Function #1406
Conversation
Mytherin
left a comment
There was a problem hiding this comment.
Thanks for the PR! Looks exquisite. Some minor comments:
| if (!state->hist) { | ||
| state->hist = new map<T, size_t>(); | ||
| } | ||
| T value = input.GetValue(i).GetValue<T>(); |
There was a problem hiding this comment.
Can we change this to use the underlying array directly (i.e. auto input_data = (T *) input_data.data;) instead of the much slower value interface?
There was a problem hiding this comment.
Sure, I guess I'll have to write a specialized method for strings then
|
Looks great, I have a suggestion: Instead of naming the map entries |
|
@hannesmuehleisen I don't have a strong opposition to that, the reason I went for k and v was to not make the answer super large would be As a comparison, Presto would output: |
|
@pdet i think self-describing names are better, since this only affects the ToString representation. |
|
Thanks! |
I'm using map, since I guess that's preferred over unordered_map?