test: fix legacy acceptance tests #69152

jpefaur · 2025-11-03T21:02:50Z

What

Legacy Acceptance Tests were not working, so this PR contains a few different changes to make these tests green. Changes are either:

Disable tests that don't make sense anymore
Change the cdk testing framework to adjust for new behavior
Add missing features
...etc

There's a more thorough description of each of the issues addressed in this PR in this doc

when doing toString we don't output seconds, which we want to include

github-actions · 2025-11-03T21:03:16Z

👋 Greetings, Airbyte Team Member!

Here are some helpful tips and reminders for your convenience.

Helpful Resources

Breaking Changes Guide - Breaking changes, migration guides, and upgrade deadlines
Developing Connectors Locally
Managing Connector Secrets
On-Demand Live Tests
On-Demand Regression Tests
#connector-ci-issues
#connector-publish-updates
#connector-build-statuses

PR Slash Commands

Airbyte Maintainers (that's you!) can execute the following slash commands on your PR:

/format-fix - Fixes most formatting issues.
/bump-version - Bumps connector versions.
- You can specify a custom changelog by passing changelog. Example: /bump-version changelog="My cool update"
- Leaving the changelog arg blank will auto-populate the changelog from the PR title.
/run-cat-tests - Runs legacy CAT tests (Connector Acceptance Tests)
/build-connector-images - Builds and publishes a pre-release docker image for the modified connector(s).
JVM connectors:
- /update-connector-cdk-version connector=<CONNECTOR_NAME> - Updates the specified connector to the latest CDK version.
  Example: /update-connector-cdk-version connector=destination-bigquery
- /bump-bulk-cdk-version type=patch changelog='foo' - Bump the Bulk CDK's version. type can be major/minor/patch.
Python connectors:
- /poe connector source-example lock - Run the Poe lock task on the source-example connector, committing the results back to the branch.
- /poe source example lock - Alias for /poe connector source-example lock.
- /poe source example use-cdk-branch my/branch - Pin the source-example CDK reference to the branch name specified.
- /poe source example use-cdk-latest - Update the source-example CDK dependency to the latest available version.

📝 Edit this welcome message.

github-actions · 2025-11-03T21:06:58Z

`destination-postgres` Connector Test Results

0 tests ±0 0 ✅ ±0 0s ⏱️ ±0s
0 suites ±0 0 💤 ±0
0 files ±0 0 ❌ ±0

Results for commit 3a89ba0. ± Comparison against base commit bb34e41.

♻️ This comment has been updated with latest results.

jpefaur · 2025-11-03T22:31:03Z

airbyte-cdk/bulk/core/load/src/main/kotlin/io/airbyte/cdk/load/data/AirbyteValue.kt

    constructor(timestamp: String) : this(OffsetDateTime.parse(timestamp))
    override fun compareTo(other: TimestampWithTimezoneValue): Int = value.compareTo(other.value)
-    @JsonValue fun toJson() = value.toString()
+    @JsonValue fun toJson() = value.format(DateTimeFormatter.ISO_OFFSET_DATE_TIME)


otherwise the string we output doesn't have seconds. Older versions of the connector do contain seconds.

jpefaur · 2025-11-03T22:31:37Z

...es/kotlin/io/airbyte/integrations/base/destination/typing_deduping/BaseTypingDedupingTest.kt

        return false
    }

+    protected open fun disableRawTableComparison(): Boolean {


the default scenario is that there is a final table and there isn't a raw table.

jpefaur · 2025-11-03T22:31:54Z

...es/kotlin/io/airbyte/integrations/base/destination/typing_deduping/BaseTypingDedupingTest.kt

    @ValueSource(longs = [0L, 42L])
    @Throws(Exception::class)
-    fun incrementalDedup(inputGenerationId: Long) {
+    open fun incrementalDedup(inputGenerationId: Long) {


so that I can disable it

jpefaur · 2025-11-03T22:35:19Z

...te/integrations/destination/postgres/typing_deduping/AbstractPostgresTypingDedupingTest.java

    String finalTableName = getStreamNamespace() + "." + Names.toAlphanumericAndUnderscore(getStreamName());
-    getDatabase().execute("CREATE VIEW " + getStreamNamespace() + ".v1 AS SELECT * FROM " + rawTableName);
+    if (!disableRawTableComparison()) {
+      getDatabase().execute("CREATE VIEW " + getRawSchema() + ".v1 AS SELECT * FROM " + rawTableName);


note that I changed the schema. I think this is the right thing to do since before this was only working because the final table schema existed. Now we can't assume that since there might not exist a final table.

jpefaur · 2025-11-03T22:36:25Z

...rations/destination/postgres/typing_deduping/PostgresRawOverrideDisableTypingDedupingTest.kt

+    @Disabled @ParameterizedTest @ValueSource(longs = []) override fun testIncrementalSyncDropOneColumn(inputGenerationId: Long) {}
+
+    // this test assumes that the fully qualified raw table name is lowercased.
+    // This was only a restriction in older versions of the connector.


I don't really know why we had this restriction though

this makes me nervous - is this saying that where previously we did create table "airbyte_internal"."blah_raw__stream_the_table", we're now doing airbyte_internal."blah_raw__stream_The_Table"? (assuming the stream name is The_Table)

wouldn't this require users to rewrite their SQL queries to target the mixed-case table?

(these stats are about a year old, but it looks like around 1/3 of cloud destination-postgres users have T+D disabled today, so that's pretty significant impact)

yes, you read that right. This is in the current docs:

Airbyte Postgres destination will create raw tables and schemas using the Unquoted identifiers by replacing any special characters with an underscore. All final tables and their corresponding columns are created using Quoted identifiers preserving the case sensitivity. Special characters in final tables are replaced with underscores.

So in the current state of the new connector this would be changed to something like

For both final and raw tables, and their corresponding columns are created using Quoted identifiers preserving the case sensitivity.

So you are right, this will cause issues to existing queries. It sounds like we want to avoid this, I'll change it.

ok I might need some help here. I thought this commit was going to make it so that both raw tables name and namespace were always lowercased, but the change didn't really do anything. It seems that still the raw table name and namespaces are created using the PostgresFinalTableNameGenerator. Any thoughts on what I could be missing?

hm, I think this destination is doing the same thing as snowflake (where the "final table name generator" is also used for the raw tables) - you probably should just edit this thing

airbyte/airbyte-integrations/connectors/destination-postgres/src/main/kotlin/io/airbyte/integrations/destination/postgres/db/PostgresNameGenerators.kt

Lines 48 to 52 in 3a89ba0

TypingDedupingUtil.concatenateRawTableName(

streamDescriptor.namespace ?: config.schema,

streamDescriptor.name

)

.toPostgresCompatibleName()

(also - the condition in the name generator is wrong. Compare snowflake's implementation - it should be if (!config.legacyRawTablesOnly), not if (config.internalTableSchema.isNullOrBlank()))

jpefaur · 2025-11-03T22:37:17Z

...rations/destination/postgres/typing_deduping/PostgresRawOverrideDisableTypingDedupingTest.kt

+    // This was only a restriction in older versions of the connector.
+    @Disabled @Test override fun testMixedCasedSchema() {}
+
+    // disabling dedup tests since dedup not supported when setting `disable_type_dedupe` to true.


Note that even though running dedup when disable_type_dedupe is set to true doesn’t make a ton of sense, it is a config that wouldn’t error out in older versions of the connector, but this will error out on the new version of the connector.

jpefaur · 2025-11-03T22:37:41Z

...n/io/airbyte/integrations/destination/postgres/typing_deduping/PostgresTypingDedupingTest.kt

+
+    // older versions of the connector used to error out in this scenario. This is not
+    // true for newer versions of the connector
+    @Disabled @Test override fun interruptedTruncateWithPriorData() {}


truth be told, I don't 100% understand what this test is doing, I just know it used to fail and doesn't fail anymore. I'm assuming this is the behavior we want.

👍 this is covered by new CDK tests

(specifically: the old CDK threw an error if we reached the end of stdin without seeing a "stream status" message; the new CDK only throws an error if we get an explicit INCOMPLETE stream status. In practice, platform always sends an explicit complete/incomplete status, so there's no behavior change at runtime)

jpefaur · 2025-11-03T22:38:34Z

...s/src/integrationTestLegacy/resources/dat/sync1_cursorchange_expectedrecords_dedup_raw.jsonl

@@ -1,4 +1,4 @@
-{"_airbyte_extracted_at": "1970-01-01T00:00:01.000000Z", "_airbyte_data": {"id1": 1, "id2": 200, "old_cursor": 0, "_ab_cdc_deleted_at": null, "name" :"Alice", "address": {"city": "San Francisco", "state": "CA"}}, "_airbyte_meta": {"changes":[],"sync_id":42}, "_airbyte_generation_id": 43}


null values are not present on the raw table anymore. This is built into the bulk cdk

jpefaur · 2025-11-03T22:39:21Z

...in/kotlin/io/airbyte/integrations/destination/postgres/sql/PostgresDirectLoadSqlGenerator.kt

    private val postgresColumnUtils: PostgresColumnUtils,
    private val postgresConfiguration: PostgresConfiguration) {
+
+    private val dropTableSuffix: String = if (postgresConfiguration.dropCascade == true) "CASCADE" else ""


dropCascade is part of the connector config. I think we just forgot to add support for this.

nice catch! We should probably eventually port the relevant test out of the legacy test suite, but not a blocker for this PR

(having the option to drop cascade is something that users really want, so we definitely should preserve it)

jpefaur · 2025-11-03T22:39:39Z

...in/kotlin/io/airbyte/integrations/destination/postgres/write/load/PostgresRecordFormatter.kt

-            jsonObject.replace(key, value.toJson())
-        }
-        val jsonData = jsonObject.serializeToString()
+        val jsonData =  Jsons.writeValueAsString(filteredRecord)


same as https://github.com/airbytehq/airbyte/pull/69152/files#r2488047061

jdpgrailsdev

LGTM, but maybe @edgao should take a look as he has more context.

edgao

one question on the naming thing - sounds kind of similar to the snowflake uppercasing fire drill, but maybe I'm misunderstanding. Otherwise lgtm!

edgao · 2025-11-05T00:23:37Z

...rations/destination/postgres/typing_deduping/PostgresRawOverrideDisableTypingDedupingTest.kt

+    @Disabled @ParameterizedTest @ValueSource(longs = []) override fun testIncrementalSyncDropOneColumn(inputGenerationId: Long) {}
+
+    // this test assumes that the fully qualified raw table name is lowercased.
+    // This was only a restriction in older versions of the connector.


this makes me nervous - is this saying that where previously we did create table "airbyte_internal"."blah_raw__stream_the_table", we're now doing airbyte_internal."blah_raw__stream_The_Table"? (assuming the stream name is The_Table)

wouldn't this require users to rewrite their SQL queries to target the mixed-case table?

(these stats are about a year old, but it looks like around 1/3 of cloud destination-postgres users have T+D disabled today, so that's pretty significant impact)

edgao · 2025-11-05T00:25:13Z

...n/io/airbyte/integrations/destination/postgres/typing_deduping/PostgresTypingDedupingTest.kt

+
+    // older versions of the connector used to error out in this scenario. This is not
+    // true for newer versions of the connector
+    @Disabled @Test override fun interruptedTruncateWithPriorData() {}


👍 this is covered by new CDK tests

(specifically: the old CDK threw an error if we reached the end of stdin without seeing a "stream status" message; the new CDK only throws an error if we get an explicit INCOMPLETE stream status. In practice, platform always sends an explicit complete/incomplete status, so there's no behavior change at runtime)

edgao · 2025-11-05T00:27:00Z

...in/kotlin/io/airbyte/integrations/destination/postgres/sql/PostgresDirectLoadSqlGenerator.kt

    private val postgresColumnUtils: PostgresColumnUtils,
    private val postgresConfiguration: PostgresConfiguration) {
+
+    private val dropTableSuffix: String = if (postgresConfiguration.dropCascade == true) "CASCADE" else ""


nice catch! We should probably eventually port the relevant test out of the legacy test suite, but not a blocker for this PR

(having the option to drop cascade is something that users really want, so we definitely should preserve it)

jpefaur added 10 commits November 3, 2025 10:56

disable raw table comparison when there are no raw tables

e262372

disable migrations

586fa04

disabling dedup tests since dedup not supported when setting to true

607dcdd

add drop cascade support

b30f92f

disable interruptedTruncateWithPriorData test

30b08f2

enable raw table comparison when disable_type_dedupe=true

1efc105

disable testMixedCasedSchema when disable_type_dedupe=true

7fd9790

avoid doing toString on datetime

3a1b10d

when doing toString we don't output seconds, which we want to include

disable testIncrementalSyncDropOneColumn

adcd960

delete raw values from expected records

4130abe

octavia-squidington-iii added the connectors/destination/postgres label Nov 3, 2025

jpefaur commented Nov 3, 2025

View reviewed changes

delete unnecessary comment

7be35f5

jpefaur commented Nov 3, 2025

View reviewed changes

jpefaur changed the title ~~Jose/fix legacy acceptance tests v2~~ fix legacy acceptance tests Nov 3, 2025

jpefaur changed the title ~~fix legacy acceptance tests~~ test: fix legacy acceptance tests Nov 3, 2025

jpefaur marked this pull request as ready for review November 3, 2025 22:40

jpefaur requested a review from a team as a code owner November 3, 2025 22:40

jpefaur requested review from jdpgrailsdev and subodh1810 November 3, 2025 22:40

jdpgrailsdev approved these changes Nov 4, 2025

View reviewed changes

jpefaur requested a review from edgao November 4, 2025 18:07

jpefaur force-pushed the jose/fix-legacy-acceptance-tests-v2 branch from 06be7bc to 7be35f5 Compare November 4, 2025 21:21

edgao approved these changes Nov 5, 2025

View reviewed changes

use lowercase namespace and name on raw tables

3a89ba0

	TypingDedupingUtil.concatenateRawTableName(
	streamDescriptor.namespace ?: config.schema,
	streamDescriptor.name
	)
	.toPostgresCompatibleName()

		@@ -1,4 +1,4 @@
		{"_airbyte_extracted_at": "1970-01-01T00:00:01.000000Z", "_airbyte_data": {"id1": 1, "id2": 200, "old_cursor": 0, "_ab_cdc_deleted_at": null, "name" :"Alice", "address": {"city": "San Francisco", "state": "CA"}}, "_airbyte_meta": {"changes":[],"sync_id":42}, "_airbyte_generation_id": 43}

test: fix legacy acceptance tests #69152

Are you sure you want to change the base?

test: fix legacy acceptance tests #69152

Conversation

jpefaur commented Nov 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What

Uh oh!

github-actions bot commented Nov 3, 2025

👋 Greetings, Airbyte Team Member!

Helpful Resources

PR Slash Commands

Uh oh!

github-actions bot commented Nov 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

destination-postgres Connector Test Results

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jpefaur Nov 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jdpgrailsdev left a comment

Choose a reason for hiding this comment

Uh oh!

edgao left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jpefaur commented Nov 3, 2025 •

edited

Loading

github-actions bot commented Nov 3, 2025 •

edited

Loading

`destination-postgres` Connector Test Results

jpefaur Nov 3, 2025 •

edited

Loading