Load CDK: Expanded namespace coverage

[johnny-schmidt]

What

Adds tests for

• null namespaces
• special characters/operators in namespaces

@edgao the null namespaces break glue as well. Not sure if that's a possible scenario for you

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
airbyte-docs	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Feb 4, 2025 5:09pm

[edgao]

@frifriSF59 is adding glue support for null namespace in #52068, so this is actually great timing

changes make sense, I think we probably need more functionality in the test to make this usable for most of our destinations though

[edgao]

I think this needs to be some sort of setDefaultNamespace(config, randomizedNamespace + "_default_namespace")-ish thing

(s3 supports null namespaces out of the box, but db/dw destinations have a config option to set the default schema/database/whatever)

[johnny-schmidt]

To clarify, the default namespace is set by the destination when the provided namespace is null?

If so, it should be null here, but we'll need a DefaultNamespaceProvider to do the test data diffing?

Something like
dumpAndDiffRecords(..., streamWithNullNamespace.copy(descriptor = streamWithNullNamespace.descriptor.copy(namespace = defaultNamespaceProvider.get(randomizedNamespace)), ...)?

[edgao]

yep it's done by the destination connector. e.g. in destination-postgres, the stream namespace turns into a postgres schema. So there's a "default schema" config option, for when a stream has null namespace

Not sure I see the need for a DefaultNamespaceProvider though - i.e. couldn't we hardcode it in the test? (setDefaultNamespace maybe being an overridable function, or an injected object, or something)

val stream = DestinationStream(Descriptor(namespace = null, name = "test_stream"), ...)
runSync(
  config = setDefaultNamespace(config, randomizedNamespace + "_default_namespace"),
  stream = stream,
  ...
)
dumpAndDiff(
  stream.copy(
    descriptor = stream.descriptor.copy(namespace = randomizedNamespace + "_default_namespace")
  ), ...
)

(also setDefaultNamespace maybe can happen during the test class's init block, to avoid people forgetting to do it)

[edgao]

I think the test needs to not change the stream name? otherwise lgtm

(also had a question, looking for your thoughts on how to

[edgao]

why do we need to change the stream name? I thought the intent of this test was to test that the destination can handle streams with different namespace but the same name, i.e. they should all use test_stream as the name

[edgao]

for now - I'll probably do some refactoring in this region later

I.e. as part of https://github.com/airbytehq/airbyte-internal-issues/issues/11543, I can probably combine the config-modifier with this thing. And then we would have two impls provided by the CDK, rather than one per destination. Curious what you think of approximately this:

class NullNamespaceIsValid {
  fun setDefaultNs(config, randomNs) {}
  fun getDefaultNs(randomNs) = null
}

abstract class NullNamespaceIsInvalid {
  const val DEFAULT_NS_PREFIX = "_defaultns"
  // contract is to set the default namespace to randomNs + DEFAULT_NS_PREFIX
  abstract fun setDefaultNs(config, randomNs)
  fun getDefaultNs(randomNs) = randomNs + DEFAULT_NS_PREFIX
}

[johnny-schmidt]

Why two? What's gained by this versus having one method that's just saying "tell us what to do when the namespace is null?"

[edgao]

b/c the missing functionality is telling the connector what to do when the namespace is null. I.e. being able to modify the config.

and at that point, I think there's exactly two valid behaviors?

1. the s3 way (default NS is just null, and modifying the config is a noop)
2. the db/dw way (default NS is based on the randomized NS, and modifying the config involves doing some JSON munging)