[SERVER-51875] Move value type to basic_types.idl Created: 29/Oct/20  Updated: 12/Nov/20  Resolved: 12/Nov/20

Status: Closed
Project: Core Server
Component/s: IDL
Affects Version/s: None
Fix Version/s: None

Type: Task Priority: Major - P3
Reporter: Samyukta Lanka Assignee: George Wangensteen
Resolution: Won't Do Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Sprint: Repl 2020-11-16
Participants:

 Description   

Some of the commands that we'll convert to IDL have parameters that accept any BSON value; for example, createIndexes's "comment" parameter. The aggregation framework has an IDL type called Value that we could use. Move Value into basic_types.idl.



 Comments   
Comment by George Wangensteen [ 12/Nov/20 ]

Thanks everyone for taking the time to think/talk this through. Given the discussion above, we're no longer planning to use the Document/Value library for this project; given the "heavy"-ness of the library and it's future, we don't think we need to create another dependency on it for this work. Instead, we'll adapt another, more lightweight existing IDL type to fit our needs, or write a new one if necessary. I've filed SERVER-52827 to track that work, and am closing this ticket as "won't do."

Comment by A. Jesse Jiryu Davis [ 11/Nov/20 ]

Thanks David! Sounds worth considering. Do we have a solution in mind for the storage ownership issue Dan pointed out a couple comments ago?

Comment by David Storch [ 11/Nov/20 ]

I've discussed this a bit with ian.boros and george.wangensteen. I'm a bit wary of expanding the uses of the Document/Value library into the context of parsing/IDL. The Document/Value library was specifically designed as a runtime representation of the data during execution of aggregation operations. If we succeed in replacing DocumentSource execution with the slot-based execution engine, then we would probably try to also delete the Document/Value library. Deleting it will become much more difficult if it starts to be used in more contexts.

Note that Document/Value has some heavy-weight features that make sense in the context of agg execution but less sense elsewhere:

  • It has a reference counting scheme if the underlying value is something like a sub-document that is held at a distance. In the context of parsing, I assume that you just want unique ownership and the reference counting would be overkill.
  • Values that are documents can be represented as BSON, and when individual fields of the BSON are accessed they are brought into DocumentStorage cache. Alternatively, this so-called "backing BSON" might be absent, in which case the DocumentStorage is not a cache but rather contains the full internal representation of the document. None of this fanciness is required for the purposes of parsing in IDL.

A simpler approach of creating a wrapper around BSONElement or some new lightweight variant type seems more appropriate here, if it's feasible to do so. ian.boros also brought up a good point of whether it's acceptable for this BSONElement/variant to point back into the original BSON for wide types like strings, objects, arrays, etc.

Comment by Daniel Gottlieb (Inactive) [ 02/Nov/20 ]

Not to (intentionally) make things more complicated. max.hirschhorn noted that BSONElement doesn't have a concept of owning its own storage, so that would be a wrinkle to figure out.

Comment by A. Jesse Jiryu Davis [ 30/Oct/20 ]

Ooh, the dependency chain is a really good point. Heretofore you can use anything in basic_types.idl and just depend on the "idl_parser" library. Henceforth you'll also need "service_context". We'll keep this in mind and consider making a basic wrapper around BSONElement be the "X" type instead of sharing "Value". In fact I've found dependency cycles sufficiently frustrating that I might now believe that's a better idea.

Comment by Daniel Gottlieb (Inactive) [ 30/Oct/20 ]

Thanks for clarifying some of the motivation for the choice of Value. I actually like the Value class over BSONElement because:

I think my main concern (i.e: FUD) with Value is that it's part of a separate BSON-like library. Pulling it up into the basic types introduces a whole new set of dependencies. In many languages, adding dependencies (internal to the project source) isn't worth worrying about, but in C++ and with our cycle detection, can lead to programmer frustration.

So trying to de-fud this, looking at the dependency chain. The two that stick out to me are field_path and date_time_support. field_path is obviously trivial. date_time_support is interesting because it depends on service_context. service_context pulls in a bunch of stuff that is obviously not related to IDL.

That said, a scons library that pulls in an IDL still gets to choose the libraries it depends on (AFAIK). I imagine the cases we want to use Value today already depend on service_context so I wouldn't expect any troubles to arise. But I wonder if in the future we'd need something similar to Value that doesn't pull in a service context dependency (and maybe resorts to making BSONElement work).

I think my last point is FUD. So to whatever degree someone was expecting my blessing to use Value, they have it. I did feel it was important to at least explore and detail the risks of using Value, even if they are more theoretical and unlikely to materialize.

And please let me know if my logic regarding dependencies was incorrect at any step. I still feel like I'm guessing at how that stuff works most of the time.

Comment by A. Jesse Jiryu Davis [ 30/Oct/20 ]

We need some IDL type "X" such that any BSON value is acceptable, and this IDL compiles:

commands:
  createIndexes:
    fields:
      comment:
        type: X

X can't be literally BSONElement, it needs to be an IDL type that appears in a "types:" list. The "Value" type is one possible X:

types:
  Value:
    bson_serialization_type: any
    description: A value with any BSON type.
    cpp_type: Value
    serializer: Value::serializeForIDL
    deserializer: Value::deserializeForIDL

... we could also define another type that uses BSONElement for its cpp_type, I think. But Value looks good and it's already written for exactly this purpose. I propose we just share it outside the aggregation framework.

Comment by Daniel Gottlieb (Inactive) [ 30/Oct/20 ]

I could be misinterpreting the problem being solved, but is adding BSONElement also an option? Assuming it is, are there advantages to using Value over BSONElement?

Generated at Thu Feb 08 05:26:41 UTC 2024 using Jira 9.7.1#970001-sha1:2222b88b221c4928ef0de3161136cc90c8356a66.