🎉 Extend logic for JDBC connectors to provide additional properties in JSON schema #7859

VitaliiMaltsev · 2021-11-11T10:18:45Z

What

Currently all jdbc connectors can provide only primitive json types
( STRING, NUMBER, OBJECT, ARRAY, BOOLEAN, NULL)

and json schema looks like

{ "type": "string" },
{ "type": "number" }

How

JsonSchemaPrimitive class was updated with jsonSchemaTypeMap

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
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 · 2021-11-11T10:18:52Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

vmaltsev seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

etsybaev

LGTM.
But please make sure you pass all PR's check-points (for example to match the PR's name convention) and it would be good to check some destinations to make sure nothing will be broken by such schema changing.

VitaliiMaltsev · 2021-11-11T10:40:02Z

LGTM. But please make sure you pass all PR's check-points (for example to match the PR's name convention) and it would be good to check some destinations to make sure nothing will be broken by such schema changing.

currently destinations can not handle json schemas type with keywords and just skip the "format". So everything should be as previously

tuliren

This is nice! Such a concise approach.

I have two questions / comments:

JsonSchemaPrimitive is widely used in so many connectors. It's surprising that this change does not affect any of them. Even though theoretically no destination will be affected, it's still safer to run integration tests on a few sources and destinations.
Have you thought about any alternative approach and compared them? E.g. replace JsonSchemaPrimitive with a class, which allows more flexible specifications. One concern about this approach is that to support more formats, there will be many more enums to add, and it does not handle things like pattern, max, etc. Using a class seems more flexible and extensible.

sherifnada · 2021-11-12T18:45:36Z

airbyte-protocol/models/src/main/java/io/airbyte/protocol/models/JsonSchemaPrimitive.java

+
+  STRING_DATE(ImmutableMap.of("type", "string", "format", "date")),
+  STRING_TIME(ImmutableMap.of("type", "string", "format", "time")),
+  STRING_TIMESTAMP(ImmutableMap.of("type", "string", "format", "date-time")),


i recommend changing this to STRING_DATETIME as it matches the json primitive it represents

i recommend changing this to STRING_DATETIME as it matches the json primitive it represents

done

sherifnada · 2021-11-12T18:45:49Z

airbyte-protocol/models/src/main/java/io/airbyte/protocol/models/JsonSchemaPrimitive.java

@@ -4,11 +4,28 @@

 package io.airbyte.protocol.models;

+import com.google.common.collect.ImmutableMap;
+
 public enum JsonSchemaPrimitive {


might make sense to call this JsonSchemaType as these no longer represent primitives

sherifnada · 2021-11-12T18:46:53Z

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

-      case DATE -> JsonSchemaPrimitive.STRING;
-      case TIME -> JsonSchemaPrimitive.STRING;
-      case TIMESTAMP -> JsonSchemaPrimitive.STRING;
+      case DATE -> JsonSchemaPrimitive.STRING_DATE;


I think we need to be super careful about how MySQL/Postgres/MSSQL/Oracle handle timezones. For example, from the MySQL docs:

MySQL converts TIMESTAMP values from the current time zone to UTC for storage, and back from UTC to the current time zone for retrieval. (This does not occur for other types such as DATETIME.) By default, the current time zone for each connection is the server's time. The time zone can be set on a per-connection basis. As long as the time zone setting remains constant, you get back the same value you store. If you store a TIMESTAMP value, and then change the time zone and retrieve the value, the retrieved value is different from the value you stored. This occurs because the same time zone was not used for conversion in both directions. The current time zone is available as the value of the time_zone system variable. For more information, see Section 5.1.15, “MySQL Server Time Zone Support”.

So for MySQL to correctly express timestamps we would need to set the connection timezone to UTC whenever we're reading the date types in UTC (we should also have tests for this)

wdyt?

good point. But currently jdbc destinations can not handle different date-time datatypes for databases and just write value as string

@VitaliiMaltsev

currently jdbc destinations can not handle different date-time datatypes for databases and just write value as string

I believe this is inaccurate - normalization writes appropriately typed date/time types depending on the value in the format field

tuliren · 2021-11-12T19:01:58Z

@sherifnada, what do you think about changing JsonSchemaPrimitive from an enum to a class? It looks like enum won't be able to support more formats. I think eventually we need to switch to a class.

sherifnada · 2021-11-12T19:33:23Z

@tuliren good idea, we can even have the current enum be a set of public static constants in that class, but still make it flexible enough to have any valid fields in an object's definition

tuliren · 2021-11-13T00:00:49Z

Given the potential complicity of changing JsonSchemaPrimitive to a class, I am fine with merging this PR as is, and do that in a separate PR.

But it is still important to run integration tests on a few sources and destinations to make sure nothing is broken.

VitaliiMaltsev · 2021-11-13T20:05:06Z

/test connector=connectors/source-oracle

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

VitaliiMaltsev · 2021-11-13T20:27:26Z

/test connector=connectors/source-cockroachdb

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

VitaliiMaltsev · 2021-11-13T20:46:32Z

Given the potential complicity of changing JsonSchemaPrimitive to a class, I am fine with merging this PR as is, and do that in a separate PR.

But it is still important to run integration tests on a few sources and destinations to make sure nothing is broken.

@tuliren i ran some tests for Oracle Source, CockroachDB Source, MySQL Source, Postgres Destination and MSSQL Destination. And also created a separate ticket for changing JsonSchemaPrimitive to a class #7944. Can i merge this PR ?

tuliren · 2021-11-13T20:52:27Z

@VitaliiMaltsev, yes. Thank you for running the tests, and creating the issue. Feel free to merge it.

sherifnada · 2021-11-15T01:42:01Z

...src/testFixtures/java/io/airbyte/integrations/source/jdbc/test/JdbcSourceAcceptanceTest.java

            .withSupportedSyncModes(Lists.newArrayList(SyncMode.FULL_REFRESH, SyncMode.INCREMENTAL))
            .withSourceDefinedPrimaryKey(
                List.of(List.of(COL_FIRST_NAME), List.of(COL_LAST_NAME)))));
  }

+  private JsonSchemaPrimitive resolveJsonSchemaType() {


shouldn't the oracle test instead override this method, instead of hardcoding a connector-specific edge case in the abstract test class? also, the name of this method should be clearer as resolveJsonSchemaType does not reflect it is only concerned with date types

sherifnada · 2021-11-15T01:47:40Z

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

-      case DATE -> JsonSchemaPrimitive.STRING;
-      case TIME -> JsonSchemaPrimitive.STRING;
-      case TIMESTAMP -> JsonSchemaPrimitive.STRING;
+      case DATE -> JsonSchemaPrimitive.STRING_DATE;


@VitaliiMaltsev

currently jdbc destinations can not handle different date-time datatypes for databases and just write value as string

I believe this is inaccurate - normalization writes appropriately typed date/time types depending on the value in the format field

sherifnada

@VitaliiMaltsev there's a few points we need to make sure are taken into account here:

If I'm reading this correctly, the current tests don't cover all newly introduced types i.e: we must test that all 3 newly introduced types (date, time, datetime) are used appropriately. It's crucial that we maximize test coverage, otherwise customers will find issues before we do.
JDBC connectors were not published in this PR, is this intended? in general whenever we make a change to a connector, we should publish it asap to make sure release notes are accurate and issues are caught quickly
I don't think we resolved the discussion around timezone. It is critical that we ensure (via comprehensive testing) there are no issues here. If I'm understanding this correctly, the MySQL example I mentioned could result in inaccurate date information being persisted in the destination, because depending on the timezone used in the JDBC connection, we might be formatting the output value incorrectly, which when writing normalized tables could result in the date format being misinterpreted as UTC when it is not (which would change the actual value of the date object)

… properties in JSON schema (#7859)" This reverts commit 2fe927b.

VitaliiMaltsev · 2021-11-15T08:31:29Z

@VitaliiMaltsev there's a few points we need to make sure are taken into account here:

If I'm reading this correctly, the current tests don't cover all newly introduced types i.e: we must test that all 3 newly introduced types (date, time, datetime) are used appropriately. It's crucial that we maximize test coverage, otherwise customers will find issues before we do.

JDBC connectors were not published in this PR, is this intended? in general whenever we make a change to a connector, we should publish it asap to make sure release notes are accurate and issues are caught quickly

I don't think we resolved the discussion around timezone. It is critical that we ensure (via comprehensive testing) there are no issues here. If I'm understanding this correctly, the MySQL example I mentioned could result in inaccurate date information being persisted in the destination, because depending on the timezone used in the JDBC connection, we might be formatting the output value incorrectly, which when writing normalized tables could result in the date format being misinterpreted as UTC when it is not (which would change the actual value of the date object)

@sherifnada this PR has already been merged with the master branch. Is it necessary to revert the changes in this PR before these 3 points are resolved?

VitaliiMaltsev · 2021-11-15T08:36:43Z

@sherifnada please advise. I'm not sure - do we need to publish all jdbc connectors if changes were not in connectors code?
Changes in this PR for airbyte-protocol and airbyte-db modules

sherifnada · 2021-11-15T08:44:32Z

JdbcSourceOperations (and others in this class) are leveraged by all JDBC connectors which means changing any such common class should trigger a publication of all JDBC connectors

… properties in JSON schema (#7859)" (#7969) This reverts commit 2fe927b.

…n JSON schema (airbytehq#7859) * add date-time formats to json schema creation * add jsonSchemaMap to enum * fix tests and checkstyle * remove python changes from PR * remove star import * Rename String_timestamp to String_Datetime * Rename String_timestamp to String_Datetime * fix checkstyle * fix jdbc source tests Co-authored-by: vmaltsev <vitalii.maltsev@globallogic.com>

… properties in JSON schema (airbytehq#7859)" (airbytehq#7969) This reverts commit 2fe927b.

vmaltsev added 3 commits November 10, 2021 13:26

add date-time formats to json schema creation

708baeb

add jsonSchemaMap to enum

a4ff8c4

fix tests and checkstyle

d70b453

github-actions bot added area/connectors Connector related issues area/platform issues related to the platform area/protocol labels Nov 11, 2021

VitaliiMaltsev temporarily deployed to more-secrets November 11, 2021 10:19 Inactive

remove python changes from PR

f1677c8

VitaliiMaltsev temporarily deployed to more-secrets November 11, 2021 10:24 Inactive

remove star import

41fb09d

VitaliiMaltsev temporarily deployed to more-secrets November 11, 2021 10:26 Inactive

VitaliiMaltsev changed the title ~~Vmaltsev/7777 extend json schema~~ Extend logic for JDBC connectors to provide additional properties in JSON schema Nov 11, 2021

VitaliiMaltsev requested review from etsybaev and yurii-bidiuk November 11, 2021 10:30

etsybaev approved these changes Nov 11, 2021

View reviewed changes

yurii-bidiuk approved these changes Nov 11, 2021

View reviewed changes

VitaliiMaltsev marked this pull request as ready for review November 11, 2021 10:40

VitaliiMaltsev requested a review from tuliren November 11, 2021 10:40

VitaliiMaltsev changed the title ~~Extend logic for JDBC connectors to provide additional properties in JSON schema~~ 🎉 Extend logic for JDBC connectors to provide additional properties in JSON schema Nov 11, 2021

VitaliiMaltsev temporarily deployed to more-secrets November 11, 2021 10:47 Inactive

VitaliiMaltsev linked an issue Nov 11, 2021 that may be closed by this pull request

Extend SourceOperations logic for JDBC connectors to provide additional properties in JSON schema #7777

Closed

tuliren reviewed Nov 12, 2021

View reviewed changes

tuliren requested a review from sherifnada November 12, 2021 08:22

sherifnada reviewed Nov 12, 2021

View reviewed changes

jrhizor temporarily deployed to more-secrets November 13, 2021 12:46 Inactive

jrhizor temporarily deployed to more-secrets November 13, 2021 17:42 Inactive

fix jdbc source tests

bfb0d1d

VitaliiMaltsev temporarily deployed to more-secrets November 13, 2021 19:47 Inactive

jrhizor temporarily deployed to more-secrets November 13, 2021 20:07 Inactive

jrhizor temporarily deployed to more-secrets November 13, 2021 20:29 Inactive

VitaliiMaltsev mentioned this pull request Nov 13, 2021

[EPIC] Change JsonSchemaPrimitive to a class #7944

Closed

VitaliiMaltsev merged commit 2fe927b into master Nov 13, 2021

VitaliiMaltsev deleted the vmaltsev/7777-extend-json-schema branch November 13, 2021 20:56

sherifnada reviewed Nov 15, 2021

View reviewed changes

VitaliiMaltsev restored the vmaltsev/7777-extend-json-schema branch November 15, 2021 08:05

VitaliiMaltsev temporarily deployed to more-secrets November 15, 2021 08:07 Inactive

VitaliiMaltsev added a commit that referenced this pull request Nov 15, 2021

Revert ":tada: Extend logic for JDBC connectors to provide additional…

e597b13

… properties in JSON schema (#7859)" This reverts commit 2fe927b.

VitaliiMaltsev mentioned this pull request Nov 15, 2021

Revert ":tada: Extend logic for JDBC connectors to provide additional… #7969

Merged

40 tasks

VitaliiMaltsev added a commit that referenced this pull request Nov 15, 2021

Revert ":tada: Extend logic for JDBC connectors to provide additional…

63c6249

… properties in JSON schema (#7859)" (#7969) This reverts commit 2fe927b.

VitaliiMaltsev mentioned this pull request Nov 15, 2021

🎉 Extend logic for JDBC connectors to provide additional properties in JSON schema (reopen) #7971

Closed

40 tasks

This was referenced Nov 16, 2021

Bump Airbyte version from 0.32.0-alpha to 0.32.1-alpha #8023

Closed

Bump Airbyte version from 0.32.0-alpha to 0.32.1-alpha #8044

Merged

schlattk pushed a commit to schlattk/airbyte that referenced this pull request Jan 4, 2022

Revert ":tada: Extend logic for JDBC connectors to provide additional…

680d42e

… properties in JSON schema (airbytehq#7859)" (airbytehq#7969) This reverts commit 2fe927b.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🎉 Extend logic for JDBC connectors to provide additional properties in JSON schema #7859

🎉 Extend logic for JDBC connectors to provide additional properties in JSON schema #7859

VitaliiMaltsev commented Nov 11, 2021 •

edited

Loading

CLAassistant commented Nov 11, 2021

etsybaev left a comment

VitaliiMaltsev commented Nov 11, 2021

tuliren left a comment

sherifnada Nov 12, 2021

VitaliiMaltsev Nov 13, 2021

sherifnada Nov 12, 2021

sherifnada Nov 12, 2021

VitaliiMaltsev Nov 13, 2021

sherifnada Nov 15, 2021

tuliren commented Nov 12, 2021

sherifnada commented Nov 12, 2021

tuliren commented Nov 13, 2021

VitaliiMaltsev commented Nov 13, 2021 •

edited by github-actions bot

Loading

VitaliiMaltsev commented Nov 13, 2021 •

edited by github-actions bot

Loading

VitaliiMaltsev commented Nov 13, 2021

tuliren commented Nov 13, 2021

sherifnada Nov 15, 2021

sherifnada Nov 15, 2021

sherifnada left a comment

VitaliiMaltsev commented Nov 15, 2021 •

edited

Loading

VitaliiMaltsev commented Nov 15, 2021

sherifnada commented Nov 15, 2021

🎉 Extend logic for JDBC connectors to provide additional properties in JSON schema #7859

🎉 Extend logic for JDBC connectors to provide additional properties in JSON schema #7859

Conversation

VitaliiMaltsev commented Nov 11, 2021 • edited Loading

What

How

Recommended reading order

Pre-merge Checklist

Community member or Airbyter

Airbyter

Community member or Airbyter

Airbyter

CLAassistant commented Nov 11, 2021

etsybaev left a comment

Choose a reason for hiding this comment

VitaliiMaltsev commented Nov 11, 2021

tuliren left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tuliren commented Nov 12, 2021

sherifnada commented Nov 12, 2021

tuliren commented Nov 13, 2021

VitaliiMaltsev commented Nov 13, 2021 • edited by github-actions bot Loading

VitaliiMaltsev commented Nov 13, 2021 • edited by github-actions bot Loading

VitaliiMaltsev commented Nov 13, 2021

tuliren commented Nov 13, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sherifnada left a comment

Choose a reason for hiding this comment

VitaliiMaltsev commented Nov 15, 2021 • edited Loading

VitaliiMaltsev commented Nov 15, 2021

sherifnada commented Nov 15, 2021

VitaliiMaltsev commented Nov 11, 2021 •

edited

Loading

VitaliiMaltsev commented Nov 13, 2021 •

edited by github-actions bot

Loading

VitaliiMaltsev commented Nov 13, 2021 •

edited by github-actions bot

Loading

VitaliiMaltsev commented Nov 15, 2021 •

edited

Loading