fix temporal type default value bug postgres #15877

subodh1810 · 2022-08-23T09:20:25Z

Issue : #15840
The bug is happening for date, timestamp (without time zone) and time without timezone data types with default values. While building the internal schema of the columns, debezium is failing with the error

🚨 User Impact 🚨

Are there any breaking changes? What is the end result perceived by the user? If yes, please merge this PR with the 🚨🚨 emoji so changelog authors can further highlight this if needed.

Pre-merge Checklist

Expand the relevant checklist and delete the others.

New Connector

Community member or Airbyter

Airbyter

If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.

Create a non-forked branch based on this PR and test the below items on it
Build is successful
If new credentials are required for use in CI, add them to GSM. Instructions.
/test connector=connectors/<name> command is passing
New Connector version released on Dockerhub by running the /publish command described here
After the connector is published, connector added to connector index as described here
Seed specs have been re-generated by building the platform and committing the changes to the seed spec files, as described here

Updating a connector

Community member or Airbyter

Grant edit access to maintainers (instructions)
Secrets in the connector's spec are annotated with airbyte_secret
Unit & integration tests added and passing. Community members, please provide proof of success locally e.g: screenshot or copy-paste unit, integration, and acceptance test output. To run acceptance tests for a Python connector, follow instructions in the README. For java connectors run ./gradlew :airbyte-integrations:connectors:<name>:integrationTest.
Code reviews completed
Documentation updated
- Connector's README.md
- Connector's bootstrap.md. See description and examples
- Changelog updated in docs/integrations/<source or destination>/<name>.md including changelog. See changelog example
PR name follows PR naming conventions

Airbyter

If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.

Create a non-forked branch based on this PR and test the below items on it
Build is successful
If new credentials are required for use in CI, add them to GSM. Instructions.
/test connector=connectors/<name> command is passing
New Connector version released on Dockerhub and connector version bumped by running the /publish command described here

Connector Generator

Issue acceptance criteria met
PR name follows PR naming conventions
If adding a new generator, add it to the list of scaffold modules being tested
The generator test modules (all connectors with -scaffold in their name) have been updated with the latest scaffold by running ./gradlew :airbyte-integrations:connector-templates:generator:testScaffoldTemplates then checking in your changes
Documentation which references the generator is updated as needed

Tests

Unit

Put your unit tests output here.

Integration

Put your integration tests output here.

Acceptance

Put your acceptance tests output here.

subodh1810 · 2022-08-23T09:41:58Z

/test connector=connectors/source-postgres

🕑 connectors/source-postgres https://github.com/airbytehq/airbyte/actions/runs/2910500256
✅ connectors/source-postgres https://github.com/airbytehq/airbyte/actions/runs/2910500256
No Python unittests run

Build Passed

Test summary info:

All Passed

edgao

couple minor/clarifying comments, lgtm otherwise

edgao · 2022-08-23T14:47:02Z

airbyte-db/db-lib/src/main/java/io/airbyte/db/jdbc/DateTimeConverter.java


  public static String convertToTimeWithTimezone(final Object time) {
    if (time instanceof final java.time.OffsetTime timetz) {
      return timetz.format(TIMETZ_FORMATTER);
+    } else {
+      if (!loggedUnknownTimeWithTimeZoneClass) {


I like this! Maybe it's worth asking infra to set an alert if this shows up in cloud (or maybe push oss telemetry)

airbyte-db/db-lib/src/main/java/io/airbyte/db/jdbc/DateTimeConverter.java

...ezium-v1-9-2/src/main/java/io/airbyte/integrations/debezium/internals/PostgresConverter.java

...te/integrations/io/airbyte/integration_tests/sources/AbstractPostgresSourceDatatypeTest.java

edgao · 2022-08-23T14:59:11Z

.../integrations/io/airbyte/integration_tests/sources/CdcWalLogsPostgresSourceDatatypeTest.java

+                  "'infinity'",
+                  "'-infinity'")
+              .addExpectedValues(
+                  "+294247-01-10T04:00:25.200000",


thanks, debezium

what type is debezium giving to us in this case? I think it would be nice to have a comment explaining why this is different from the snapshot test

ryankfu · 2022-08-23T15:34:51Z

airbyte-db/db-lib/src/main/java/io/airbyte/db/jdbc/DateTimeConverter.java

    } else {
+      if (!loggedUnknownTimestampClass) {


Correct me if I'm wrong but this logic is supposed to say something like, if in the else case this is indicative of an unknownTimestampClass and there is a negation here to only log this unknown once, right?

If so, what's the reason for limiting multiple logging instances? This logic wasn't initially clear why the loggedUnknownTime... variables were set to false and then set to true but it seems that when an loggedUnknownTime... variable has been set to true that indicates an unknownTime... was seen, right?

The reason I only want to log it once is to avoid the scenario where we have a million of these logs! That could happen in case if a table has a million records and debezium starts sending a different class type for those records

subodh1810 · 2022-08-23T16:45:27Z

/test connector=connectors/source-postgres

🕑 connectors/source-postgres https://github.com/airbytehq/airbyte/actions/runs/2913266296
✅ connectors/source-postgres https://github.com/airbytehq/airbyte/actions/runs/2913266296
No Python unittests run

Build Passed

Test summary info:

All Passed

ryankfu · 2022-08-23T16:59:36Z

...te/integrations/io/airbyte/integration_tests/sources/AbstractPostgresSourceDatatypeTest.java

                  "13:00:00.000000")
              .build());
    }

+    // time without time zone


Minor nit: it seems this comment is the same as the test above and this tests for null

hah I just copied the above test and replaced the values with null. the idea is to test whether columns with time without time zone data type and null values sync properly or not

subodh1810 · 2022-08-23T17:38:10Z

/publish connector=connectors/source-postgres

🕑 Publishing the following connectors:
connectors/source-postgres
https://github.com/airbytehq/airbyte/actions/runs/2913582495

Connector	Did it publish?	Were definitions generated?
connectors/source-postgres	✅	✅

if you have connectors that successfully published but failed definition generation, follow step 4 here ▶️

subodh1810 · 2022-08-23T17:38:24Z

/publish connector=connectors/source-postgres-strict-encrypt

🕑 Publishing the following connectors:
connectors/source-postgres-strict-encrypt
https://github.com/airbytehq/airbyte/actions/runs/2913583322

Connector	Did it publish?	Were definitions generated?
connectors/source-postgres-strict-encrypt	✅	❌

if you have connectors that successfully published but failed definition generation, follow step 4 here ▶️

ryankfu

Looks good to me, overall like the ability to log these unknown types as well

As Ed has noted, it may be useful to have these log messages surfaces into something where it can be tracked for cloud users

* fix temporal datatype bug for columns with default in postgres cdc * fix test * add test for date and time as well * add more logs for unknown classes * review comments * bump version * auto-bump connector version [ci skip] Co-authored-by: Octavia Squidington III <octavia-squidington-iii@users.noreply.github.com>

subodh1810 added 3 commits August 23, 2022 14:12

fix temporal datatype bug for columns with default in postgres cdc

608c3b8

fix test

a6b8f5c

add test for date and time as well

c7fa37d

subodh1810 self-assigned this Aug 23, 2022

octavia-squidington-iii added the connectors/source/postgres label Aug 23, 2022

github-actions bot added the area/connectors Connector related issues label Aug 23, 2022

add more logs for unknown classes

c76eca3

subodh1810 marked this pull request as ready for review August 23, 2022 09:41

subodh1810 requested a review from a team as a code owner August 23, 2022 09:41

subodh1810 temporarily deployed to more-secrets August 23, 2022 09:43 Inactive

subodh1810 mentioned this pull request Aug 23, 2022

java.lang.RuntimeException: java.time.format.DateTimeParseException: Text '0' could not be parsed at index 0 #15840

Closed

edgao approved these changes Aug 23, 2022

View reviewed changes

ryankfu reviewed Aug 23, 2022

View reviewed changes

review comments

4290b39

subodh1810 temporarily deployed to more-secrets August 23, 2022 16:37 Inactive

ryankfu reviewed Aug 23, 2022

View reviewed changes

subodh1810 added 2 commits August 23, 2022 22:58

Merge branch 'master' into fix-temporal-type-default-value-bug-postgres-

50c8175

bump version

5814e78

github-actions bot added the area/documentation Improvements or additions to documentation label Aug 23, 2022

octavia-squidington-iii added the connectors/source/postgres-strict-encrypt label Aug 23, 2022

subodh1810 temporarily deployed to more-secrets August 23, 2022 17:39 Inactive

ryankfu approved these changes Aug 23, 2022

View reviewed changes

auto-bump connector version [ci skip]

c9b000c

subodh1810 merged commit 828285d into master Aug 23, 2022

subodh1810 deleted the fix-temporal-type-default-value-bug-postgres- branch August 23, 2022 18:30

octavia-squidington-iii mentioned this pull request Aug 23, 2022

Bump Airbyte version from 0.40.1 to 0.40.2 #15909

Closed

octavia-squidington-iii mentioned this pull request Aug 24, 2022

Bump Airbyte version from 0.40.1 to 0.40.2 #15931

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix temporal type default value bug postgres #15877

fix temporal type default value bug postgres #15877

subodh1810 commented Aug 23, 2022 •

edited

Loading

subodh1810 commented Aug 23, 2022 •

edited by github-actions bot

Loading

edgao left a comment

edgao Aug 23, 2022

edgao Aug 23, 2022

ryankfu Aug 23, 2022

subodh1810 Aug 23, 2022

subodh1810 commented Aug 23, 2022 •

edited by github-actions bot

Loading

ryankfu Aug 23, 2022

subodh1810 Aug 23, 2022

subodh1810 commented Aug 23, 2022 •

edited by github-actions bot

Loading

subodh1810 commented Aug 23, 2022 •

edited by github-actions bot

Loading

ryankfu left a comment •

edited

Loading

fix temporal type default value bug postgres #15877

fix temporal type default value bug postgres #15877

Conversation

subodh1810 commented Aug 23, 2022 • edited Loading

🚨 User Impact 🚨

Pre-merge Checklist

Community member or Airbyter

Airbyter

Community member or Airbyter

Airbyter

Tests

subodh1810 commented Aug 23, 2022 • edited by github-actions bot Loading

Build Passed

edgao left a comment

Choose a reason for hiding this comment

edgao Aug 23, 2022

Choose a reason for hiding this comment

edgao Aug 23, 2022

Choose a reason for hiding this comment

ryankfu Aug 23, 2022

Choose a reason for hiding this comment

subodh1810 Aug 23, 2022

Choose a reason for hiding this comment

subodh1810 commented Aug 23, 2022 • edited by github-actions bot Loading

Build Passed

ryankfu Aug 23, 2022

Choose a reason for hiding this comment

subodh1810 Aug 23, 2022

Choose a reason for hiding this comment

subodh1810 commented Aug 23, 2022 • edited by github-actions bot Loading

subodh1810 commented Aug 23, 2022 • edited by github-actions bot Loading

ryankfu left a comment • edited Loading

Choose a reason for hiding this comment

subodh1810 commented Aug 23, 2022 •

edited

Loading

subodh1810 commented Aug 23, 2022 •

edited by github-actions bot

Loading

subodh1810 commented Aug 23, 2022 •

edited by github-actions bot

Loading

subodh1810 commented Aug 23, 2022 •

edited by github-actions bot

Loading

subodh1810 commented Aug 23, 2022 •

edited by github-actions bot

Loading

ryankfu left a comment •

edited

Loading