🎉 Source MySQL: support all MySQL 8.0 types #7970

tuliren · 2021-11-15T09:30:25Z

What

Close Support all types in MySQL source #7726.
The same approach can be applied to other databases.
The version is bumped again in Bump the minor version for mysql source #8731 to 0.5.0.

How

The current AbstractJdbcSource is changed to a generic class.
A new JdbcCompatibleSourceOperations interface is added that extends from SourceOperations. It has more interface methods to be used in AbstractJdbcSource.
- All the type related methods are moved from the source class to this interface.
The current JdbcSourceOperations is broken into two classes:
- AbstractJdbcCompatibleSourceOperations has most of the original logics with generic types.
- JdbcSourceOperations extends from AbstractJdbcCompatibleSourceOperations with the JDBCTypes.
- MySqlSourceOperations extends from AbstractJdbcCompatibleSourceOperations with the MysqlType.

TODOs

Add more unit tests.
Handle complex MySQL types (e.g. Point).
Use MysqlType in all methods.

Pre-Merge Checklist - Updating 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

sherifnada

I like it! Great direction

.../src/test-integration/java/io/airbyte/integrations/source/mysql/MySqlSourceDatatypeTest.java

tuliren · 2021-12-02T00:45:54Z

@sherifnada, @etsybaev, @DoNotPanicUA, this PR is ready for review. Would you mind taking a look?

etsybaev

This looks quite impressive!

etsybaev · 2021-12-05T20:17:11Z

...rc/main/java/io/airbyte/integrations/standardtest/source/AbstractSourceDatabaseTypeTest.java

+   * fail it, so it is turned off by default. It should be enabled for all databases eventually.
+   */
+  protected boolean testCatalog() {
+    return false;


Is that expected to be committed or just accidentally forgotten?

same comment

This is expected. See the comment for the rationale. We need to check each database because we can turn it on by default.

sherifnada · 2021-12-08T20:43:20Z

...rc/main/java/io/airbyte/integrations/standardtest/source/AbstractSourceDatabaseTypeTest.java

+   * fail it, so it is turned off by default. It should be enabled for all databases eventually.
+   */
+  protected boolean testCatalog() {
+    return false;


same comment

sherifnada · 2021-12-08T20:43:57Z

...-cockroachdb/src/main/java/io/airbyte/integrations/source/cockroachdb/CockroachDbSource.java

@@ -97,9 +97,4 @@ public static void main(final String[] args) throws Exception {
    LOGGER.info("completed source: {}", CockroachDbSource.class);
  }

-  @Override
-  protected JdbcSourceOperations getSourceOperations() {


how come this was removed? is the class still useful?

This method is unnecessary. The sourceOperations is an instance variable, and it is assigned in the constructor. The AbstractJdbcSource can just reference that variable.

sherifnada · 2021-12-08T20:47:05Z

...s/source-mysql/src/main/java/io/airbyte/integrations/source/mysql/MySqlSourceOperations.java

+      case TIME -> putTime(json, columnName, resultSet, colIndex);
+      // The returned year value can either be a java.sql.Short (when yearIsDateType=false)
+      // or a java.sql.Date with the date set to January 1st, at midnight (when yearIsDateType=true).
+      // Currently, JsonSchemaPrimitive does not support integer, but only supports number.


seems like fixing up jsonSchemaPrimitive is high up on the list of things to fix also?

Added an issue: #8722

sherifnada · 2021-12-08T20:47:44Z

...s/source-mysql/src/main/java/io/airbyte/integrations/source/mysql/MySqlSourceOperations.java

+   * does not contain information about its charset, which is needed to determine whether the column
+   * is a string or binary. We don't distinguish between string vs binary in
+   * {@link JsonSchemaPrimitive} for now. So it is fine. However, in the future, if we want to
+   * separate these two types, we need to update the column metadata query for MySQL. See


should we have an issue for this?

Added an issue: #8723

Actually JDBC does return BINARY for CHAR columns with binary character set. Same for VARCHAR. So this is a false alarm.

sherifnada · 2021-12-08T20:48:13Z

.../src/test-integration/java/io/airbyte/integrations/source/mysql/MySqlSourceDatatypeTest.java

-            .fullSourceDataType("varchar(256) character set cp1251")
-            .addInsertValues("null", "'тест'")
-            .addExpectedValues(null, "тест")
+            // MySQL converts values in the ranges '0' - '69' to YEAR value in the range 2000 - 2069


what on earth...

tuliren · 2021-12-12T04:04:35Z

/test connector=connectors/source-mysql

🕑 connectors/source-mysql https://github.com/airbytehq/airbyte/actions/runs/1568650475
❌ connectors/source-mysql https://github.com/airbytehq/airbyte/actions/runs/1568650475
🐛 https://gradle.com/s/bcgx7vv2yn5n2

tuliren · 2021-12-12T04:36:10Z

/test connector=connectors/source-mysql

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

tuliren · 2021-12-12T05:01:20Z

/publish connector=connectors/source-mysql

🕑 connectors/source-mysql https://github.com/airbytehq/airbyte/actions/runs/1568730855
✅ connectors/source-mysql https://github.com/airbytehq/airbyte/actions/runs/1568730855

* Add jdbc compatible layer * Support routine mysql types * Format code * Fix build * Refactor abstract jdbc source and operation classes * Update mysql source operations * Test discover command for mysql * Remove abstract jdbc compatible source layer * Format code * Update template * Fix more types * Bump version * Log original field type * Update comments * Bump version in seed

github-actions bot added the area/connectors Connector related issues label Nov 15, 2021

tuliren requested review from DoNotPanicUA, sherifnada and etsybaev November 15, 2021 09:31

tuliren temporarily deployed to more-secrets November 15, 2021 09:34 Inactive

sherifnada reviewed Nov 15, 2021

View reviewed changes

.../src/test-integration/java/io/airbyte/integrations/source/mysql/MySqlSourceDatatypeTest.java Outdated Show resolved Hide resolved

tuliren added 8 commits November 18, 2021 01:40

Add jdbc compatible layer

9c13dfd

Support routine mysql types

17e3aa5

Format code

8b5fc8f

Fix build

0dd71c7

Refactor abstract jdbc source and operation classes

6a37360

Update mysql source operations

28dfc29

Test discover command for mysql

4725189

Merge branch 'master' into liren/support-all-mysql-types

3f675b2

tuliren force-pushed the liren/support-all-mysql-types branch from cf5e44f to 3f675b2 Compare November 29, 2021 10:52

tuliren temporarily deployed to more-secrets November 29, 2021 10:54 Inactive

tuliren temporarily deployed to more-secrets November 29, 2021 11:10 Inactive

Remove abstract jdbc compatible source layer

55a5499

tuliren force-pushed the liren/support-all-mysql-types branch from 809be96 to 55a5499 Compare November 29, 2021 11:12

tuliren temporarily deployed to more-secrets November 29, 2021 11:14 Inactive

Format code

f0f8d91

tuliren temporarily deployed to more-secrets November 29, 2021 11:21 Inactive

tuliren temporarily deployed to more-secrets November 29, 2021 11:23 Inactive

tuliren temporarily deployed to more-secrets November 29, 2021 11:43 Inactive

tuliren requested a review from sherifnada November 30, 2021 22:53

tuliren marked this pull request as ready for review November 30, 2021 22:53

Update template

dbed663

tuliren temporarily deployed to more-secrets December 2, 2021 00:47 Inactive

etsybaev approved these changes Dec 5, 2021

View reviewed changes

DoNotPanicUA approved these changes Dec 8, 2021

View reviewed changes

sherifnada approved these changes Dec 8, 2021

View reviewed changes

Merge branch 'master' into liren/support-all-mysql-types

59fb44a

tuliren temporarily deployed to more-secrets December 9, 2021 21:22 Inactive

tuliren temporarily deployed to more-secrets December 9, 2021 22:08 Inactive

tuliren temporarily deployed to more-secrets December 11, 2021 21:21 Inactive

tuliren mentioned this pull request Dec 11, 2021

Re-evaluate MySQL year type #8722

Closed

tuliren added 4 commits December 11, 2021 17:42

Fix more types

00bfea5

Bump version

72f7748

Log original field type

cda4c2b

Update comments

042cf35

github-actions bot added the area/documentation Improvements or additions to documentation label Dec 12, 2021

tuliren temporarily deployed to more-secrets December 12, 2021 04:06 Inactive

jrhizor temporarily deployed to more-secrets December 12, 2021 04:38 Inactive

jrhizor temporarily deployed to more-secrets December 12, 2021 05:03 Inactive

Bump version in seed

8c49d4d

tuliren temporarily deployed to more-secrets December 12, 2021 05:35 Inactive

tuliren merged commit 6843bc1 into master Dec 12, 2021

tuliren deleted the liren/support-all-mysql-types branch December 12, 2021 05:49

jrhizor mentioned this pull request Dec 12, 2021

Bump Airbyte version from 0.33.10-alpha to 0.33.11-alpha #8728

Merged

tuliren mentioned this pull request Dec 12, 2021

Bump the minor version for mysql source #8731

Merged

etsybaev mentioned this pull request Dec 16, 2021

Support all types in SQL Server source #7728

Closed

rodireich mentioned this pull request Apr 7, 2023

Ensure all source-mysql datatypes are represented correctly in airbyte datatypes #24470

Closed

rodireich assigned rodireich and unassigned rodireich Apr 10, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🎉 Source MySQL: support all MySQL 8.0 types #7970

🎉 Source MySQL: support all MySQL 8.0 types #7970

tuliren commented Nov 15, 2021 •

edited

Loading

sherifnada left a comment

tuliren commented Dec 2, 2021

etsybaev left a comment

etsybaev Dec 5, 2021

sherifnada Dec 8, 2021

tuliren Dec 9, 2021

sherifnada Dec 8, 2021

sherifnada Dec 8, 2021

tuliren Dec 11, 2021

sherifnada Dec 8, 2021

tuliren Dec 11, 2021

sherifnada Dec 8, 2021

tuliren Dec 11, 2021

tuliren Dec 12, 2021

sherifnada Dec 8, 2021

tuliren commented Dec 12, 2021 •

edited by github-actions bot

Loading

tuliren commented Dec 12, 2021 •

edited by github-actions bot

Loading

tuliren commented Dec 12, 2021 •

edited by github-actions bot

Loading

🎉 Source MySQL: support all MySQL 8.0 types #7970

🎉 Source MySQL: support all MySQL 8.0 types #7970

Conversation

tuliren commented Nov 15, 2021 • edited Loading

What

How

TODOs

Recommended reading order

Pre-Merge Checklist - Updating Connector

Community member or Airbyter

Airbyter

sherifnada left a comment

Choose a reason for hiding this comment

tuliren commented Dec 2, 2021

etsybaev 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

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 Dec 12, 2021 • edited by github-actions bot Loading

tuliren commented Dec 12, 2021 • edited by github-actions bot Loading

tuliren commented Dec 12, 2021 • edited by github-actions bot Loading

tuliren commented Nov 15, 2021 •

edited

Loading

tuliren commented Dec 12, 2021 •

edited by github-actions bot

Loading

tuliren commented Dec 12, 2021 •

edited by github-actions bot

Loading

tuliren commented Dec 12, 2021 •

edited by github-actions bot

Loading