🐛 Source Postgres: allow handling of java sql date #9554

shrodingers · 2022-01-17T15:50:41Z

What

Had an issue on a Postgres CDC source, where I have some Postgres date columns, and on the initial load, date object types were Java.sql.Date, which weren't handled properly. This caused a warning to be prompted for every row, and the date format conversion to fall on the default string converter.

How

Added a handler for Java.sql.Date type that can be returned by Debeezium, instead of a repetitive warning causing unreadable logs.

Allow for a specific handler and not the default one.

🚨 User Impact 🚨

Should be none, since it just adds a handler instead of the default one for debeezium based sources

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
Credentials added to Github CI. 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
Credentials added to Github CI. Instructions.
/test connector=connectors/<name> command is passing.
New Connector version released on Dockerhub by running the /publish command described here
After the new connector version is published, connector version bumped in the seed directory 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

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.

CLAassistant · 2022-01-17T15:50:50Z

All committers have signed the CLA.

marcosmarxm · 2022-01-18T03:17:45Z

thanks @shrodingers! I requested to the team review the contribution during this week.

DoNotPanicUA · 2022-01-20T12:16:36Z

...ebezium/src/main/java/io/airbyte/integrations/debezium/internals/DebeziumConverterUtils.java

@@ -42,7 +43,10 @@ public static String convertDate(final Object input) {
    } else if (input instanceof Number) {
      return DataTypeUtils.toISO8601String(
          new Timestamp(((Number) input).longValue()).toLocalDateTime());
-    } else if (input instanceof String) {
+    } else if (input instanceof Date) {
+      return DataTypeUtils.toISO8601String(input)


The input has Object type and can't be put as input parameter for the DataTypeUtils.toISO8601String without cast.

DoNotPanicUA · 2022-01-20T12:21:26Z

...ebezium/src/main/java/io/airbyte/integrations/debezium/internals/DebeziumConverterUtils.java

@@ -42,7 +43,10 @@ public static String convertDate(final Object input) {
    } else if (input instanceof Number) {
      return DataTypeUtils.toISO8601String(
          new Timestamp(((Number) input).longValue()).toLocalDateTime());
-    } else if (input instanceof String) {
+    } else if (input instanceof Date) {


Please specify the case which you are trying to fix with steps to reproduce.
I've tested your change using CDC MySql Source and there is no any issues with Date type.
More over the class java.sql.Date is not used there.
All columns with Date type processed inside the Debezium as class java.time.LocalDate. Example from data type test:

I'm gonna update the PR description, im using CDC for PostgreSql for this case (thought it was the same case for all dbs thus my initial description).

I had lots of warning while syncing columns with postgres's type Date looking like :
Uncovered date class type java.sql.Date. Use default converter. Which led me to this part of the code (only happened on the initial migration though, not while consuming the slots).

I had a minimal setup to reproduce this :

A postgres table with a date column

replication and CDC enabled

connection that syncs the table in incremental mode

I log the class name inside the convertDate function, and without this update i have the following log :

Maybe it is a postgres only issue, or that i didn't understand how everything works around Debezium, but this is the reason i tried to submit this PR

DoNotPanicUA

Hi @shrodingers ,
You PR looks good for me, but there are few small things left to do.

As this change will effect only Postgres processing and I will only incr. that version, please rename the PR from Debeezium based sources to Source Postgres.
The formatting check is a part of our build. It means that all changes should be in line with it. So, in order not to brake the master build, please make you changes in line with the changes in my technical PR [#9554 PR] Postgres/Mssql Source: Increase version #9753. (Note that sequence of imports also counts)
All tests are fine, so after that we can merge your PR and I will bump the version.

…ngers/debeeziumDate

shrodingers · 2022-01-24T16:20:39Z

Hi @DoNotPanicUA, thanks for your patience in this review !

Just changed the PR title, as it indeed only affected Postgres sources
Merged your technical Pr inside mine in order to update it with airbyte's master and fix the formatting issues.
I'll remain available if there is anything else needed here

DoNotPanicUA

👍

* #9554 PR * format * incr version for Postgres * Update docs/integrations/sources/postgres.md Co-authored-by: Marcos Marx <marcosmarxm@users.noreply.github.com> * test upd * test upd * incr ver for mssql as effected source Co-authored-by: Marcos Marx <marcosmarxm@users.noreply.github.com>

DoNotPanicUA · 2022-01-25T20:41:09Z

Hi @shrodingers,
Please be informed that 0.4.3 Postgres version is published and now can be used.
Thank you for your input into our project ;)

fix: allow handling of java sql date

2067d99

octavia-squidington-iii added the community label Jan 17, 2022

shrodingers changed the title ~~fix: allow handling of java sql date~~ 🐛 Debeezium based sources: allow handling of java sql date Jan 17, 2022

marcosmarxm requested a review from DoNotPanicUA January 17, 2022 18:19

marcosmarxm added the gl label Jan 18, 2022

marcosmarxm assigned DoNotPanicUA and marcosmarxm Jan 18, 2022

marcosmarxm added the area/databases label Jan 18, 2022

DoNotPanicUA suggested changes Jan 20, 2022

View reviewed changes

shrodingers and others added 2 commits January 21, 2022 16:21

fix: compilation

31c0a91

airbytehq#9554 PR

d822ea8

DoNotPanicUA mentioned this pull request Jan 24, 2022

[#9554 PR] Postgres/Mssql Source: Increase version #9753

Merged

DoNotPanicUA self-requested a review January 24, 2022 14:36

format

e683402

DoNotPanicUA suggested changes Jan 24, 2022

View reviewed changes

shrodingers changed the title ~~🐛 Debeezium based sources: allow handling of java sql date~~ 🐛 Source Postgres: allow handling of java sql date Jan 24, 2022

Merge remote-tracking branch 'upstream/aleonets/check_pr' into shrodi…

cad59ee

…ngers/debeeziumDate

DoNotPanicUA approved these changes Jan 24, 2022

View reviewed changes

DoNotPanicUA merged commit c83d3aa into airbytehq:master Jan 24, 2022

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

Bump Airbyte version from 0.35.7-alpha to 0.35.8-alpha #9762

Merged

This was referenced Jan 25, 2022

Bump Airbyte version from 0.35.9-alpha to 0.35.10-alpha #9798

Closed

Bump Airbyte version from 0.35.9-alpha to 0.35.10-alpha #9800

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🐛 Source Postgres: allow handling of java sql date #9554

🐛 Source Postgres: allow handling of java sql date #9554

shrodingers commented Jan 17, 2022 •

edited

Loading

CLAassistant commented Jan 17, 2022 •

edited

Loading

marcosmarxm commented Jan 18, 2022

DoNotPanicUA Jan 20, 2022

DoNotPanicUA Jan 20, 2022

shrodingers Jan 21, 2022 •

edited

Loading

DoNotPanicUA left a comment

shrodingers commented Jan 24, 2022

DoNotPanicUA left a comment

DoNotPanicUA commented Jan 25, 2022

🐛 Source Postgres: allow handling of java sql date #9554

🐛 Source Postgres: allow handling of java sql date #9554

Conversation

shrodingers commented Jan 17, 2022 • edited Loading

What

How

🚨 User Impact 🚨

Community member or Airbyter

Airbyter

Community member or Airbyter

Airbyter

CLAassistant commented Jan 17, 2022 • edited Loading

marcosmarxm commented Jan 18, 2022

DoNotPanicUA Jan 20, 2022

Choose a reason for hiding this comment

DoNotPanicUA Jan 20, 2022

Choose a reason for hiding this comment

shrodingers Jan 21, 2022 • edited Loading

Choose a reason for hiding this comment

DoNotPanicUA left a comment

Choose a reason for hiding this comment

shrodingers commented Jan 24, 2022

DoNotPanicUA left a comment

Choose a reason for hiding this comment

DoNotPanicUA commented Jan 25, 2022

shrodingers commented Jan 17, 2022 •

edited

Loading

CLAassistant commented Jan 17, 2022 •

edited

Loading

shrodingers Jan 21, 2022 •

edited

Loading