🎉 MySQL destination: normalization #4163

tuliren · 2021-06-16T17:16:45Z

What

This PR resolves Implement normalization for the MySql destination. #3429.

How

Describe the solution

Todo

~~Append hash to table name~~
- I could not fix all the test failures after making this change on both Java and Python
- This change would affect existing normalizations and requires a migration
~~Investigate dependency specification~~
- Move dbt-mysql to Dockerfile does not solve the problem
  - pip install dbt-mysql there still resulted in missing dependency exception related to dbt.
- Cannot change dbt version to 0.19.1
  - dbt-mysql requires dbt 0.19.0
  - Forking our own dbt-mysql that depends on dbt 0.19.1 resulted in a charset issue and test failures.
    - This approach requires more investigation.
Add unit test for mysql name converter
Check in mysql dbt output
Update changed dbt output

Pre-merge Checklist

Expand the checklist which is relevant for this PR.

Connector checklist

tuliren · 2021-06-18T18:34:09Z

/test connector=bases/base-normalization

🕑 bases/base-normalization https://github.com/airbytehq/airbyte/actions/runs/950760572
❌ bases/base-normalization https://github.com/airbytehq/airbyte/actions/runs/950760572

tuliren · 2021-06-18T18:53:24Z

@ChristopheDuong, I am running into this error in the MySQL integration test:

java.sql.SQLSyntaxErrorException: Identifier name '_airbyte_raw_nested_stream_with_complex_columns_resulting_into_long_names' is too long

Do you know why the truncation is not working? It worked in the unit tests.

Here is the logs.

ChristopheDuong · 2021-06-21T13:40:48Z

Do you know why the truncation is not working? It worked in the unit tests.

It seems to be working fine, for example on table names created by normalization: nested_stream_with_co__lting_into_long_names_ab1

but it is not applied to names of raw destination table names that are given as input tables to normalization: _airbyte_raw_nested_stream_with_complex_columns_resulting_into_long_names (as listed in the sources.yml file)

Do you know how would destination-mysql choose to call the output raw table for such a stream?

ChristopheDuong · 2021-06-21T14:00:29Z

Do you know how would destination-mysql choose to call the output raw table for such a stream?

After downloading and looking at the output log from the destination-mysql, here is the answer:

2021-06-18 18:49:56 �[1;31mERROR�[m i.a.i.d.b.BufferedStreamConsumer(close):203 - {} - on close failed.
java.sql.SQLSyntaxErrorException: Identifier name '_airbyte_raw_nested_stream_with_complex_columns_resulting_into_long_names' is too long
	at com.mysql.cj.jdbc.exceptions.SQLError.createSQLException(SQLError.java:120) ~[mysql-connector-java-8.0.22.jar:8.0.22]
	at com.mysql.cj.jdbc.exceptions.SQLError.createSQLException(SQLError.java:97) ~[mysql-connector-java-8.0.22.jar:8.0.22]
	at com.mysql.cj.jdbc.exceptions.SQLExceptionsMapping.translateException(SQLExceptionsMapping.java:122) ~[mysql-connector-java-8.0.22.jar:8.0.22]
	at com.mysql.cj.jdbc.StatementImpl.executeInternal(StatementImpl.java:764) ~[mysql-connector-java-8.0.22.jar:8.0.22]
	at com.mysql.cj.jdbc.StatementImpl.execute(StatementImpl.java:648) ~[mysql-connector-java-8.0.22.jar:8.0.22]
	at org.apache.commons.dbcp2.DelegatingStatement.execute(DelegatingStatement.java:194) ~[commons-dbcp2-2.7.0.jar:2.7.0]
	at org.apache.commons.dbcp2.DelegatingStatement.execute(DelegatingStatement.java:194) ~[commons-dbcp2-2.7.0.jar:2.7.0]
	at io.airbyte.db.jdbc.JdbcDatabase.lambda$execute$0(JdbcDatabase.java:50) ~[io.airbyte-airbyte-db-0.26.0-alpha.jar:?]
	at io.airbyte.db.jdbc.DefaultJdbcDatabase.execute(DefaultJdbcDatabase.java:61) ~[io.airbyte-airbyte-db-0.26.0-alpha.jar:?]
	at io.airbyte.db.jdbc.JdbcDatabase.execute(JdbcDatabase.java:50) ~[io.airbyte-airbyte-db-0.26.0-alpha.jar:?]
	at io.airbyte.integrations.destination.jdbc.DefaultSqlOperations.createTableIfNotExists(DefaultSqlOperations.java:65) ~[io.airbyte.airbyte-integrations.connectors-destination-jdbc-0.26.0-alpha.jar:?]
	at io.airbyte.integrations.destination.jdbc.JdbcBufferedConsumerFactory.lambda$onCloseFunction$3(JdbcBufferedConsumerFactory.java:190) ~[io.airbyte.airbyte-integrations.connectors-destination-jdbc-0.26.0-alpha.jar:?]
	at io.airbyte.integrations.destination.buffered_stream_consumer.OnCloseFunction.accept(OnCloseFunction.java:29) ~[io.airbyte.airbyte-integrations.bases-base-java-0.26.0-alpha.jar:?]
	at io.airbyte.integrations.destination.buffered_stream_consumer.BufferedStreamConsumer.close(BufferedStreamConsumer.java:195) [io.airbyte.airbyte-integrations.bases-base-java-0.26.0-alpha.jar:?]
	at io.airbyte.integrations.base.FailureTrackingAirbyteMessageConsumer.close(FailureTrackingAirbyteMessageConsumer.java:82) [io.airbyte.airbyte-integrations.bases-base-java-0.26.0-alpha.jar:?]
	at io.airbyte.integrations.base.IntegrationRunner.consumeWriteStream(IntegrationRunner.java:138) [io.airbyte.airbyte-integrations.bases-base-java-0.26.0-alpha.jar:?]
	at io.airbyte.integrations.base.IntegrationRunner.run(IntegrationRunner.java:113) [io.airbyte.airbyte-integrations.bases-base-java-0.26.0-alpha.jar:?]
	at io.airbyte.integrations.destination.mysql.MySQLDestination.main(MySQLDestination.java:111) [io.airbyte.airbyte-integrations.connectors-destination-mysql-0.26.0-alpha.jar:?]

So I would say that you need to make sure the destination can implement some logic to fit long stream names successfully and reproduce the same logic in normalization when guessing the input table name... (you are experiencing here one of the pain points I was writing about in my refactoring of Configured Catalog document)

airbyte-integrations/bases/base-normalization/unit_tests/test_table_name_registry.py

...zation/integration_tests/resources/test_primary_key_streams/dbt_schema_tests/schema_test.yml

tuliren · 2021-06-23T19:03:43Z

/test connector=bases/base-normalization

🕑 bases/base-normalization https://github.com/airbytehq/airbyte/actions/runs/965479637
❌ bases/base-normalization https://github.com/airbytehq/airbyte/actions/runs/965479637

tuliren · 2021-07-01T06:02:15Z

@ChristopheDuong, this PR is ready for review.

However, I was not able to do the following with this PR:

Remove dbt-mysql and dbt dependencies from setup
- I tried to move dbt-mysql to Dockerfile. But installing dbt-mysql using pip does not solve the problem. There were still missing dependency exceptions related to dbt.
- I also could not change dbt version to 0.19.1.
  - This is because dbt-mysql requires dbt 0.19.0.
  - I tried to fork our own dbt-mysql that depends on dbt 0.19.1. But that ended up with a charset issue, which failed the tests. I think this approach should eventually work. But it requires more investigation. The root cause is probably due to some changes from dbt 0.19.0 to 0.19.1.
Append a hash at the end of long table names
- After making this change in both the Python and Java, I could not fix all the failed integration tests.
- This change would affect existing normalizations and requires a migration, which seems beyong the scope of this PR.

Can we merge this PR as is for now? I can create issues to track the remaining problems.

I plan to check in the new mysql test output after the code review is done, because there are lots of files and can make it harder to review the code. But I push them if you prefer to review those files as well.

ChristopheDuong · 2021-07-01T10:20:09Z

/test connector=bases/base-normalization

🕑 bases/base-normalization https://github.com/airbytehq/airbyte/actions/runs/989677289
❌ bases/base-normalization https://github.com/airbytehq/airbyte/actions/runs/989677289

ChristopheDuong · 2021-07-01T12:16:09Z

It seems @marcosmarxm added an extra test case with the dedup fix with CDC...

It is so weird, MySQL runs this query fine:

but fails with this because the column _ab_cdc_deleted_at contains NULL values.

(note that cast(null as float) is perfectly fine, and if you ran the select queries without create table then it's good too)

...ons/bases/base-normalization/dbt-project-template/macros/cross_db_utils/type_conversions.sql

ChristopheDuong · 2021-07-01T12:41:36Z

airbyte-integrations/bases/base-normalization/integration_tests/test_ephemeral.py

+        # In MySQL, the max number of columns is limited by row size (8KB),
+        # not by absolute column count. It is way fewer than 1500.
+        return


Not to be solved in this PR but we should probably handle the validity of rows (in terms of size) in the destination-mysql and log some warning/errors or reject such records?

About 250 columns, seems to be really low, especially for sources like hubspot/salesforce and they could easily run into exceptions there?

The max number of columns is related to the size of each column. In the test, we are creating string columns, which corresponds to char columns in mysql and each column is relatively large. That's why as few as 250 columns can fail the test. In reality, not every column will be of type char, and the max is 4096 in mysql.

tuliren · 2021-07-01T22:10:27Z

The root cause of the casting failure is that when using json_extract, the extracted json null is different from a native mysql null. The correct approach is to use json_value for extracting scaler json values, which will return native mysql types.

tuliren · 2021-07-01T22:21:12Z

/test connector=bases/base-normalization

🕑 bases/base-normalization https://github.com/airbytehq/airbyte/actions/runs/991703530
✅ bases/base-normalization https://github.com/airbytehq/airbyte/actions/runs/991703530

tuliren · 2021-07-01T23:20:25Z

/test connector=bases/base-normalization

🕑 bases/base-normalization https://github.com/airbytehq/airbyte/actions/runs/991823249
✅ bases/base-normalization https://github.com/airbytehq/airbyte/actions/runs/991823249

ChristopheDuong · 2021-07-02T08:22:02Z

good!

don't forget to bump and publish MySQL destination and the normalization image then

tuliren · 2021-07-04T03:07:56Z

/publish connector=bases/base-normalization

🕑 bases/base-normalization https://github.com/airbytehq/airbyte/actions/runs/997568949
✅ bases/base-normalization https://github.com/airbytehq/airbyte/actions/runs/997568949

github-actions bot added area/worker Related to worker normalization labels Jun 16, 2021

tuliren added 8 commits June 17, 2021 12:07

Add mysql dbt package

55752a9

Add mysql normalization support in java

049cfa2

Add mysql normalization support in python

9165e80

Fix unit tests

94c10be

Update readme

173ad67

Setup mysql container in integration test

3d9cbf7

Add macros

9e8e939

Depend on dbt-mysql from git repo

47945bf

tuliren force-pushed the liren/mysql-destination-normalization branch from 934481e to 47945bf Compare June 18, 2021 08:52

tuliren added 3 commits June 18, 2021 01:54

Remove mysql limitation test

7f04076

Test normalization

b9e3066

Revert protocol format change

f13a95b

Fix mysel json macros

dee3479

tuliren requested review from ChristopheDuong and marcosmarxm June 21, 2021 02:12

ChristopheDuong reviewed Jun 22, 2021

View reviewed changes

airbyte-integrations/bases/base-normalization/unit_tests/test_table_name_registry.py Outdated Show resolved Hide resolved

tuliren added 3 commits June 22, 2021 12:56

Fix two more macros

abe8eb7

Fix table name length

a6acc91

Fix array macro

bbc3905

github-actions bot added the area/connectors Connector related issues label Jun 23, 2021

tuliren added 2 commits June 23, 2021 11:56

Fix equality test macro

51fcb2a

Update replace-identifiers

5cbe550

tuliren commented Jun 23, 2021

View reviewed changes

...zation/integration_tests/resources/test_primary_key_streams/dbt_schema_tests/schema_test.yml Show resolved Hide resolved

tuliren added 2 commits June 30, 2021 23:04

Merge branch 'master' into liren/mysql-destination-normalization

ebf5cc4

Check in unit test for mysql name transformer

f94b1c8

ChristopheDuong reviewed Jul 1, 2021

View reviewed changes

...ons/bases/base-normalization/dbt-project-template/macros/cross_db_utils/type_conversions.sql Outdated Show resolved Hide resolved

ChristopheDuong reviewed Jul 1, 2021

View reviewed changes

Fix type conversion

9310950

tuliren and others added 2 commits July 1, 2021 15:11

Use json_value to extract scalar json fields

1c68996

Move dbt-mysql to Dockerfile (#4459)

4e2a0dd

tuliren added 5 commits July 1, 2021 15:27

Format code

5c49cc3

Check in mysql dbt output

46cab59

Remove unnecessary quote

aefc479

Update mysql equality test to match 0.19.0

84e6e1c

Check in schema_test update

0202369

ChristopheDuong approved these changes Jul 2, 2021

View reviewed changes

tuliren added 3 commits July 3, 2021 20:05

Update readme

967e1d8

Merge branch 'master' into liren/mysql-destination-normalization

73a1d25

Bump base normalization version

f972e4b

Update document

289ba24

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

tuliren merged commit 2caf390 into master Jul 4, 2021

tuliren deleted the liren/mysql-destination-normalization branch July 4, 2021 03:31

tuliren mentioned this pull request Jul 4, 2021

Destination MySQL: bump version #4531

Merged

ChristopheDuong mentioned this pull request Jul 9, 2021

Turn on MYSQL normalization flag. #4651

Merged

26 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🎉 MySQL destination: normalization #4163

🎉 MySQL destination: normalization #4163

tuliren commented Jun 16, 2021 •

edited by ChristopheDuong

Loading

tuliren commented Jun 18, 2021 •

edited by github-actions bot

Loading

tuliren commented Jun 18, 2021

ChristopheDuong commented Jun 21, 2021 •

edited

Loading

ChristopheDuong commented Jun 21, 2021 •

edited

Loading

tuliren commented Jun 23, 2021 •

edited by github-actions bot

Loading

tuliren commented Jul 1, 2021

ChristopheDuong commented Jul 1, 2021 •

edited by github-actions bot

Loading

ChristopheDuong commented Jul 1, 2021 •

edited

Loading

ChristopheDuong Jul 1, 2021

tuliren Jul 1, 2021

tuliren commented Jul 1, 2021

tuliren commented Jul 1, 2021 •

edited by github-actions bot

Loading

tuliren commented Jul 1, 2021 •

edited by github-actions bot

Loading

ChristopheDuong commented Jul 2, 2021

tuliren commented Jul 4, 2021 •

edited by github-actions bot

Loading

🎉 MySQL destination: normalization #4163

🎉 MySQL destination: normalization #4163

Conversation

tuliren commented Jun 16, 2021 • edited by ChristopheDuong Loading

What

How

Todo

Recommended reading order

Pre-merge Checklist

tuliren commented Jun 18, 2021 • edited by github-actions bot Loading

tuliren commented Jun 18, 2021

ChristopheDuong commented Jun 21, 2021 • edited Loading

ChristopheDuong commented Jun 21, 2021 • edited Loading

tuliren commented Jun 23, 2021 • edited by github-actions bot Loading

tuliren commented Jul 1, 2021

ChristopheDuong commented Jul 1, 2021 • edited by github-actions bot Loading

ChristopheDuong commented Jul 1, 2021 • edited Loading

ChristopheDuong Jul 1, 2021

Choose a reason for hiding this comment

tuliren Jul 1, 2021

Choose a reason for hiding this comment

tuliren commented Jul 1, 2021

tuliren commented Jul 1, 2021 • edited by github-actions bot Loading

tuliren commented Jul 1, 2021 • edited by github-actions bot Loading

ChristopheDuong commented Jul 2, 2021

tuliren commented Jul 4, 2021 • edited by github-actions bot Loading

tuliren commented Jun 16, 2021 •

edited by ChristopheDuong

Loading

tuliren commented Jun 18, 2021 •

edited by github-actions bot

Loading

ChristopheDuong commented Jun 21, 2021 •

edited

Loading

ChristopheDuong commented Jun 21, 2021 •

edited

Loading

tuliren commented Jun 23, 2021 •

edited by github-actions bot

Loading

ChristopheDuong commented Jul 1, 2021 •

edited by github-actions bot

Loading

ChristopheDuong commented Jul 1, 2021 •

edited

Loading

tuliren commented Jul 1, 2021 •

edited by github-actions bot

Loading

tuliren commented Jul 1, 2021 •

edited by github-actions bot

Loading

tuliren commented Jul 4, 2021 •

edited by github-actions bot

Loading