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(operator): add groupBy #166

Closed
wants to merge 1 commit into from
Closed

feat(operator): add groupBy #166

wants to merge 1 commit into from

Conversation

benlesh
Copy link
Member

@benlesh benlesh commented Aug 13, 2015

closes #165


_next(value: T) {
this.group.complete();
this.parent.groups[this.group.key] = null;
Copy link
Member

Choose a reason for hiding this comment

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

Nitpicking here, but this logic seems ripe to put into a function on the GroupBySubscriber, the way we've done it in merge. This would also make it easier to subclass.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds reasonable

@benlesh
Copy link
Member Author

benlesh commented Aug 14, 2015

There is a glaring bug in this one... if you select an object as a key. I'll need to rework this to add some sort of hash code or use an Array and lookup groups each time in the event that keys are objects.

@benlesh
Copy link
Member Author

benlesh commented Aug 14, 2015

Made the desired changed for @trxcllnt ...

I'm not going to change the "bug" I mentioned above though, keys should be returned as strings or they will coerce as strings because it's using a hashtable under the hood for group storage. I'm sticking with this method for performance reasons. Native Map implementations and Map shims are much, much slower on the look up. Given that performance is a primary goal, and that creating a composite key in the keySelector should be trivial, I see no reason to slow things down to accommodate using "anything" as a key.

@benlesh
Copy link
Member Author

benlesh commented Aug 15, 2015

Compromise

After speaking with both @trxcllnt and @mattpodwysocki on this... I've come up with what I think is a solid compromise...

  • the underlying groups collection on the GroupBySubscriber starts off as null
  • when the first key arrives, the type is examined (just once) to determine if it's a string or not.
    • if the key is a string: Use the FastMap, which is a Map-duck-quack-quack that simply wraps a Hash table.
    • if the key is not a string: Use native Map or a simple polyfill (copy pasta'ed from RxJS-of-yore).

My tests have determined that FastMap is twice as fast for getting and setting values than Map.

Caveats

  • If the first key is a string, but other keys are Objects, you're going to have a bad time.

Future

I think this approach will enable us to easily remove the "FastMap" whenever ES6 Map is better optimized by more platforms.

@benlesh
Copy link
Member Author

benlesh commented Aug 19, 2015

merged via rebase with 1e13aea

@benlesh benlesh closed this Aug 19, 2015
@benlesh benlesh deleted the groupBy branch April 27, 2016 17:19
@lock
Copy link

lock bot commented Jun 7, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Jun 7, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

groupBy / groupByUntil
2 participants