Schema inference: Shape::widen()

[jshearer]

Implement Shape::widen which enables widening a Shape to fit a provided AsNode. This sets the groundwork for permanently keeping track of a running inferred schema in the combiner.

Of note, the schemas "inferred" by widen() are maximally strict:

• By default, newly inferred objects have have additionalProperties: false.
• Object fields initially have required: true until we encounter a document missing that field, at which point it will be downgraded to required: false. Note: This isn't required AFAICT. Should we really do this?

The next piece of work is implementing the stubbed-out enforce_field_count_limits. With that, we should have everything we need to implement the running inferred schema and emit it to the ops logs. Also of note is that the reduce: flow-inferred-schema-merge reduction annotation implementation can and should also use enforce_field_count_limits, as it's possible for itra-transaction documents to not exceed the limits while inter-transaction documents do, and we care about limiting both of those cases.

---

This change is 

[psFried]

What's the plan for determining whether a shape was actually modified by calls to widen? Are we keeping two copies and doing an equality comparison? It'd be nice if widen could return a bool indicating whether or not it was actually mutated by the operation, though I'm also recognizing that that approach might add to an already considerable level of complexity in that function.

We need this in order to determine whether or not to actually emit the new schema after draining the combiner. Totally fine to implement it in a subsequent PR, IMO, as long as we have a plan for it.

[jshearer]

What's the plan for determining whether a shape was actually modified by calls to widen?

I'm inclined to keep it simple for the moment and give each transaction a copy of the running shape, which we can then compare after the transaction commits. Yes it's less efficient than keeping track of whether the shape was modified or not, but since it only happens once every transaction vs once for every document, I don't think it'd be that painful performance-wise.

[jgraettinger]

More feedback for you

[jgraettinger]

LGTM % comments below

We can land this, but the next big question is, what does this do to performance? This is adding more overhead in an area that's already critical path, so we'll have to accept some regression, but do need to quantify "how much?".

There's a combiner benchmark that models a parameterized mix of real-world documents and can be further extended to drive widen and quantify some of this impact.

[jgraettinger]

nit: this comment could be a bit crisper. For example:

Two possible cases:
* This schema has individual `properties` as well as `additionalProperties: false`,
   meaning no other properties are possible. We recursively widen each of the input
  `fields` into their respective `properties`, adding new ones as required.
* This schema has `additionalProperties` _other_ than `false`.
   In this case, we apply each of `fields` to widen matched `properties`,
   and otherwise widen `additionalProperties` where not matched.

Right, and this is the subtle power of good comments. Writing this out made me realize that our behavior is incorrect in the general case, where we're widening an ObjShape that may have started as an actual schema.

To be fully correct, we would need to look for matched properties and even patternProperties to widen before we resort to widening additionalProperties.

In your shoes I would correct the implementation now -- because I'll otherwise forget about it -- but I'm okay punting given the time pressure for this feature. If we do punt, please add a good comment here and issue on this defect.

[jshearer]

Fixed and tested that:

• We widen explicitly named properties even if additionalProperties is non-false
• We widen matching patternProperties before widening additionalProperties

[jgraettinger]

nit: this comment / NOTE doesn't make sense here anymore.

[jshearer]

I actually decided to move the docstring from ObjShape::widen here instead since this is a public function so it'll be more visible and explains what's going on better.