nalply / rbts

Typescript red-black tree
ISC License
12 stars 3 forks source link

need to add equality comparator #9

Open noname0310 opened 2 years ago

noname0310 commented 2 years ago

202 line's key !== node.key is acts like a reference comparison for an Object. so, It is impossible to process a case where an object of the nature of a record is used as a key.

It would be better to add comparator like equalOp?: (a: K, b: K) => boolean in Tree<K, V> constructor parameter.

/** @internal */_findNode(
  key: K,
  node: Node<K, V> = this._root,
): Node<K, V> {
  while (node.ok && key !== node.key)
//                  ^^^^^^^^^^^^^^^^ --- here is problem
    node = this._less(key, node) ? node._left : node._right
  return node
}
nalply commented 2 years ago

Right. It's true that !== is not exactly the perfect thing to do because there's lessOp, however lessOp had a performance price. I am not sure whether eqOp would be a better solution, I am sorry. I suggest you fork and try out yourself.

Thanks for the report!

On Thu, Jan 13, 2022 at 4:53 PM noname @.***> wrote:

202 line's key !== node.key is acts like a reference comparison for an Object. so, It is impossible to process a case where an object of the nature of a record is used as a key.

It would be better to add comparator like equalOp?: (a: K, b: K) => boolean in Tree<K, V> constructor parameter.

/* @internal /_findNode( key: K, node: Node<K, V> = this._root,): Node<K, V> { while (node.ok && key !== node.key)// ^^^^^^^^^^^ --- here is problem node = this._less(key, node) ? node._left : node._right return node}

— Reply to this email directly, view it on GitHub https://github.com/nalply/rbts/issues/9, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABKCAWBVBDEJWS7YAVXNMTUV3YPXANCNFSM5L4HGVTQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you are subscribed to this thread.Message ID: @.***>

noname0310 commented 2 years ago

Is there no change in this repository regarding this topic? If so, it would be better to write down an additional explanation of this problem in the documentation.

nalply commented 2 years ago

Please try to run a test case with at least a million entries where the equalOp is used heavily (by calling get(), set() and delete() many times. Compare the equalOp with the !== case and see whether there's a big performance degradation. I would like to see the results of both runs.

When you do that it's easier for me to see whether equalOp causes performance problems.

Have a look at Readme.md and click on the profiler output to see what I am writing about.