[SERVER-31773] BSONObj comparison Created: 31/Oct/17  Updated: 03/Nov/17  Resolved: 03/Nov/17

Status: Closed
Project: Core Server
Component/s: Internal Code, Querying
Affects Version/s: None
Fix Version/s: None

Type: Improvement Priority: Minor - P4
Reporter: Simon Assignee: David Storch
Resolution: Won't Fix Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Sprint: Query 2017-11-13
Participants:

 Description   

The DeferredComparison for BSONObj uses BSONObj::woCompare
In turn, it compares BSONElements using explicitly BSONElement::woCompare

This misses the opportunity to use the DeferredComparison of BSONElement for overloaded comparison operators.

Although this is a strong constraint in the current implementation, this does not allow to specify a different BSONElement comparison implementaton using the DeferredComparison mechanism that could be used instead.

Potential usage: implicit type conversion



 Comments   
Comment by David Storch [ 03/Nov/17 ]

I see, thanks for the additional detail. Yes, we could change the various implementations of BSONObj::ComparatorInterface::compare() to use BSONElement::ComparatorInterface, though you're right that we would have to do something to avoid code duplication with BSONObj::woCompare(). This is not something we're planning to pursue in the near future, however.

Best,
Dave

Comment by Simon [ 03/Nov/17 ]

Hello David

The fact that woCompare() exists shows that there are several comparison modes possible between objects (whether BSONObj, BSONElement or else).

For a consistence point of view, in my opinion, it would make sense that

  • BSONObj::woCompare() make use of BSONElement::woCompare().
  • BSONObj operator< (operator overload) make use of BSONElement operator<
  • and that the BSONObj::ComparatorInterface make use of BSONElement::ComparatorInterface

The fact that the current implementation all use woCompare() under the hood should not imply that shortcuts are taken and that BSONObj operator< use directly BSONElement::woCompare() for instance (that is not currently the case, this is for exemple only).

Although this means probably duplicating some comparison code at this time (because they all currently use woCompare()), this would allow for a more modular and fine-tuned comparison logic.

If woCompare() is always the desired comparison mode, why not always call it explicitely ?

Meanwhile, I understand that this ticket is not about a bug and that it does not bring direct new functionalities considering the amount of rework it may imply and intensive regression tests so it is Ok with me as "Won't Fix" even though I believe it could bring a valuable improvement to the consistency and reliability of the internal code.

Comment by David Storch [ 03/Nov/17 ]

Hi Uyttendaele,

If I understand correctly, you are suggesting a pure refactor in which we change BSONObj::woCompare() to be implemented in terms of the BSONElement::ComparatorInterface. While this is certainly an interesting idea, the MongoDB query engineering team does not see the value in pursuing this refactor right now. It is certainly the case that we could implement many interesting BSONObj or BSONElement comparators, such as those that can compare with implicit type conversion. However, this work seems separate from the pure refactor you suggest.

I am closing this ticket as Won't Fix. Please let me know if we misunderstood something, or if you have a more compelling reason for why our engineering team should consider this work. And thanks for your interest in MongoDB---several of the SERVER tickets you filed recently contain insightful observations!

Best,
Dave

Comment by Mark Agarunov [ 31/Oct/17 ]

Hello Uyttendaele,

Thank you for the detailed description of this behavior. I've set the fixVersion to "Needs Triage" for this new feature to be scheduled against our currently planned work. Updates will be posted on this ticket as they are available.

Thanks,
Mark

Generated at Thu Feb 08 04:28:08 UTC 2024 using Jira 9.7.1#970001-sha1:2222b88b221c4928ef0de3161136cc90c8356a66.