feat: Support for streams with unknown schemas

[visch]

Right now in https://github.com/meltano/sdk/blob/main/singer_sdk/helpers/_typing.py#L198 we ignore any properties that do not exist and print a message. While this is nice in most cases there are cases where you would rather have the tap print the data to the Record even if there is no corresponding schema. Today this works with nested objects, and is a workaround.

A big scary config item like IGNORE_SCHEMA_THIS_IS_SCARY_COULD_BREAK_TARGETS would be fine as well, what do you all think?

[aaronsteers]

Related discussion:

• Support variant object types in tap SCHEMA #191

@visch - Do you primarily have this in mind for top-level properties or for sub-properties? If only for subproperties, I think it makes sense to have an 'any valid json object' data type that does not need to be recursively defined. We don't have support for this as of yet in the SDK but probably the Postgres tap has some prior art since json columns are supported.)

For top-level properties, it would be trickier to leave these undefined/unnamed since most targets will at least to know the top-level column list in the SCHEMA message ahead of receiving RECORD messages.

If the tap data is fully variant, with no prior contracts, presumably a tap developer could mitigate the above by sending a single data_record column which is a fully variant JSON "Object" type.

What do you think of this approach?

cc @edgarrmondragon

[edgarrmondragon]

If the tap data is fully variant, with no prior contracts, presumably a tap developer could mitigate the above by sending a single data_record column which is a fully variant JSON "Object" type.

@aaronsteers I like that, and that's probably the behavior I'd expect, for example, from a hypothetical tap-mongodb built on the SDK.

[visch]

If the tap data is fully variant, with no prior contracts, presumably a tap developer could mitigate the above by sending a single data_record column which is a fully variant JSON "Object" type.

Yeah that makes a lot of sense, I don't know why I didn't think of it!

Could add to the sample an example of a unknown schema. As it's straight forward something like

{ 
    "type": "object",
    "properties:": {
        "type":"object",
        "raw": {}
    }
}

Using post_process() or something. In our case it would be very useful to have a raw object for each stream (easily doable) as we're in a discovery phase at the moment and trying to figure out what's wrong with the docs. If raw existed life would be easier.

[z3z1ma]

from a hypothetical tap-mongodb built on the SDK.

The tap mongo is not hypothetical 🎉
https://github.com/z3z1ma/tap-mongodb

If the tap data is fully variant, with no prior contracts, presumably a tap developer could mitigate the above by sending a single data_record column which is a fully variant JSON "Object" type.

I don't do this though, I deferred it to the target (which I also built for BigQuery). Mongo db data not wrapped in dict could be sent to blob storage and be valid/more representative of its source structure without further processing so I didn't take the wrap approach in the tap. My initial impression is that it makes more sense in the target to handle extra props via a variant type approach more than it is in the tap to nest its variant data. I guess the latter might play better with some existing targets but we be interested to know exactly which. 🤔

[edgarrmondragon]

The tap mongo is not hypothetical 🎉 https://github.com/z3z1ma/tap-mongodb

@z3z1ma that is awesome! I see you're using additionalProperties which gets rid of any schema validation errors, and you're probably getting tons of warnings for properties not found in the schema 😅.

@aaronsteers I think additionalProperties could be used more generally here to decide if unknown properties should be dropped from the record. If we make it default to False we can keep currently functionality and allow developers to use additionalProperties in their stream schemas to toggle property pruning. Wdyt?

[visch]

The tap mongo is not hypothetical 🎉 https://github.com/z3z1ma/tap-mongodb

@z3z1ma that is awesome! I see you're using additionalProperties which gets rid of any schema validation errors, and you're probably getting tons of warnings for properties not found in the schema 😅.

@aaronsteers I think additionalProperties could be used more generally here to decide if unknown properties should be dropped from the record. If we make it default to False we can keep currently functionality and allow developers to use additionalProperties in their stream schemas to toggle property pruning. Wdyt?

Awesome tap @z3z1ma :D

I messed with additionalProperties a bit I don't think it's respected in the check at https://github.com/meltano/sdk/blob/main/singer_sdk/helpers/_typing.py#L198 unless I'm missing something. I thought I tried it too 🤷

[edgarrmondragon]

I messed with additionalProperties a bit I don't think it's respected in the check at https://github.com/meltano/sdk/blob/main/singer_sdk/helpers/_typing.py#L198 unless I'm missing something. I thought I tried it too 🤷

@visch yeah, I'm suggesting we use that JSON schema feature to give control over property pruning. But you're right, I don't see how record properties are not removed in @z3z1ma's tap 🤔

[alex-harness]

That a good point 😄
That isn't in sync with what I am using. 🤦

Here is how Ive been doing it in my setup

Alternatively it could be more efficient from a cpu cycle perspective to monkey patch conform record types to a noop.

[edgarrmondragon]

@z3z1ma that makes sense, so you're shimming the fields in the schema, just with an empty type object 👍

[aaronsteers]

@aaronsteers I think additionalProperties could be used more generally here to decide if unknown properties should be dropped from the record. If we make it default to False we can keep currently functionality and allow developers to use additionalProperties in their stream schemas to toggle property pruning. Wdyt?

Coming back to this after quite a while and after a reading and rereading of the additionalProperties spec...

1. The expected behavior of additionalProperties: false is to fail validation. https://json-schema.org/understanding-json-schema/reference/object.html#additional-properties. The proposals below somewhat hijack the default 'fail' validation logic and instead apply pruning, warning, or failure when preferable per use case. (Noting this as a deliberate choice, as I think this makes sense in context - but nevertheless want to call it out.)
2. A recent PR added additionl_properties as a type hint. Below focuses on the false implementation (explicitly disallowing), which is a slightly different use case than the typing/casting scenario. https://json-schema.org/understanding-json-schema/reference/object.html#additional-properties
3. I'd be okay adding an optional constructor argument allow_additional_properties: bool | None = None, as an option in the ObjectType and PropertiesList helper classes. If not provided, I think the respective defaults would be:
   1. True for config,
   2. False for stream schema nodes which declare other properties,
   3. True for schema nodes which are only declared as object and do not have any other properties declarations.
4. Presumably then, when walking the nodes, we'd prune, alarm on, or allow unexpected nodes with logic something like the following:
   • for config validation:
     • we throw an error if we're parsing config with extra properties (or subproperties) and additionalProperties=False.
     • otherwise, we parse the config without complaining.
   • for record validation:
     • if additionalProperties=False: we prune extra nodes automatically (perhaps suppressing warning messages we'd otherwise print today).
     • if additionalProperties=True or additionaProperties={type: ...}: we keep any extra property nodes.
     • if additionalProperties=null or additionalProperties is not declared: we do as we do today and prune the extra nodes, while printing a warning message.
5. All of the above logic is calculated according to the top-level schema node, but also is overriden by any more closely-adjacent declarations relative to child nodes.
   • E.g.: if a child schema redeclared additionalProperties, then the redeclaration overrides behavior that otherwise would be inferred by a higher-level parent.

@edgarrmondragon - What do you think of the above overall? Would you tweak or change anything in the above?