Bmoric/add namespace to protocol

[benmoriceau]

What

update the protocol in order to add a namespace in association with the stream name.

There was a field that was missing in the change that happened to the state message in the protocol. This was about the need of a namespace field to make the stream name schema specific. This is related to this discussion

[lmossman]

Currently only used by JDBC destinations to determine what schema to write to.

I'm not sure if we want to put this in the protocol, as more connectors could start using this in the future.

Airbyte streams from the same sources should have the same namespace.

Is this always true? I'm not sure if it exists in prod, but I believe we have tests where a single source has multiple namespaces

[pedroslopez]

Is this always true? I'm not sure if it exists in prod, but I believe we have tests where a single source has multiple namespaces

Not necessarily - we have a few tickets planned to allow streams in the CDK to define namespaces #12492. This would be used for things like bases in airtable (#12491).

[jdpgrailsdev]

Perhaps this could be re-worded to address the original intent and @lmossman's suggestion:

Examples include JDBC destinations, which use this field to determine the destination schema...

[benmoriceau]

I feel like that the change is in progress and the use of it might change as well. I will just keep it at optional source defined namespace.

[pedroslopez]

Note: looks like format needs to be run to fix the connectors-base build

[evantahler]

Assuming the format & comment is fixed up, this looks good to me!

There may be a spot to talk about this on AirbyteStateMessage, but it's not required.

A question: When looking at the storage designed for per-stream state messages (doc), why was the choice to store both the stream_name and namespace rather than a "combined" name like name (string) = "${namespace}:#{stream_name}"? I'm a fan for clear column storage, but it may be possible to change the /meaning/ of AirbyteStreamState.name to accomplish the goal without updating the protocol - if that were a goal.

[lmossman]

why was the choice to store both the stream_name and namespace rather than a "combined" name like name (string) = "${namespace}:#{stream_name}"?

This approach feels more prone to error to me, as it requires everything implementing the protocol to know about this special format, and to only use it in the case where there is a namespace. I think having the namespace be an explicit optional field is a clearer interface

[lmossman]

We should probably add the following to the AirbyteStreamState object:

required:
  - name
  - state

to make it clear that those two properties are always required, and namespace is optional

[benmoriceau]

Done

[benmoriceau]

This approach feels more prone to error to me, as it requires everything implementing the protocol to know about this special format, and to only use it in the case where there is a namespace. I think having the namespace be an explicit optional field is a clearer interface

yes it is more error prone. This was also an organization that got use in other part of the protocol (AirbyteCatalog). We don't want any discrepancy here

[cgardens]

why was the choice to store both the stream_name and namespace rather than a "combined" name like name (string) = "${namespace}:#{stream_name}"?

to piggyback on what lake and benoit said. because we allow a stream name to include any utf8 characters there is no separator we can use without getting into character escaping hell. separate fields avoids that, but it is admittedly more verbose.

[cgardens]

thanks!

[cgardens]

@benmoriceau is there a reason we are doing name and namespace as top-level fields as opposed to a stream key object? Michel had mentioned it and I think it's a solid idea. Curious how we picked the current format.