Source support primary keys #2488

ChristopheDuong · 2021-03-16T16:47:08Z

What

Closes #2461

How

JDBC Abstract Sources are filling up the sourceDefinedPrimaryKey field

Pre-merge Checklist

Run integration tests
Publish Docker images

Recommended reading order

airbyte-integrations/connectors/source-jdbc/src/main/java/io/airbyte/integrations/source/jdbc/AbstractJdbcSource.java
the rest

sherifnada

can we add more test cases to cover this feature more thoroughly? Also one style comment

sherifnada · 2021-03-16T21:41:38Z

...ectors/source-jdbc/src/main/java/io/airbyte/integrations/source/jdbc/AbstractJdbcSource.java

@@ -354,6 +357,16 @@ private static void assertColumnsWithSameNameAreSame(String schemaName, String t
                  return new ColumnInfo(f.get(INTERNAL_COLUMN_NAME).asText(), jdbcType);
                })
                .collect(Collectors.toList())))
+        .peek(t -> {


I think this would be better done in a for loop after the sequence of stream operations for readability

...ource-postgres/src/test/java/io/airbyte/integrations/source/postgres/PostgresSourceTest.java

ChristopheDuong · 2021-03-17T11:54:06Z

/publish connector=connectors/source-postgres

🕑 connectors/source-postgres https://github.com/airbytehq/airbyte/actions/runs/660941824
✅ connectors/source-postgres https://github.com/airbytehq/airbyte/actions/runs/660941824

ChristopheDuong · 2021-03-17T11:54:10Z

/publish connector=connectors/source-mysql

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

ChristopheDuong · 2021-03-17T11:54:15Z

/publish connector=connectors/source-mssql

🕑 connectors/source-mssql https://github.com/airbytehq/airbyte/actions/runs/660942406
✅ connectors/source-mssql https://github.com/airbytehq/airbyte/actions/runs/660942406

ChristopheDuong · 2021-03-17T11:54:21Z

/publish connector=connectors/source-redshift

🕑 connectors/source-redshift https://github.com/airbytehq/airbyte/actions/runs/660942490
❌ connectors/source-redshift https://github.com/airbytehq/airbyte/actions/runs/660942490
🕑 connectors/source-redshift https://github.com/airbytehq/airbyte/actions/runs/660942490
✅ connectors/source-redshift https://github.com/airbytehq/airbyte/actions/runs/660942490

sherifnada

I am going to sleep so well tonight with all these amazing test cases!!! Looks great.

cgardens · 2021-03-17T18:01:46Z

...ectors/source-jdbc/src/main/java/io/airbyte/integrations/source/jdbc/AbstractJdbcSource.java

+                      .withSourceDefinedPrimaryKey(
+                          t.getPrimaryKeys().stream().filter(Objects::nonNull).map(Collections::singletonList).collect(Collectors.toList())))


styling nit.

Suggested change

.withSourceDefinedPrimaryKey(

t.getPrimaryKeys().stream().filter(Objects::nonNull).map(Collections::singletonList).collect(Collectors.toList())))

.withSourceDefinedPrimaryKey(t.getPrimaryKeys()

.stream()

.filter(Objects::nonNull)

.map(Collections::singletonList)

.collect(Collectors.toList())))

cgardens · 2021-03-17T18:18:19Z

...ectors/source-jdbc/src/main/java/io/airbyte/integrations/source/jdbc/AbstractJdbcSource.java

+    result.forEach(t -> {
+      try {
+        final List<String> primaryKeys = database.bufferedResultSetQuery(
+            conn -> conn.getMetaData().getPrimaryKeys(databaseOptional.orElse(null), t.getSchemaName(), t.getName()),


do we need to do this query once per table? or if we leave table null, can we just run this query once for the whole database?

I've tried to run the function without table names.

It seems like for MySQL this is not allowed and is throwing an exception about not having a table name but it does work for some other Databases... I made a new PR for this

See also:

The method getTables takes a like-pattern for the tableNamePattern parameter, so "%" matches all table names.
The method getPrimaryKeys and getExportedKeys do not take a pattern, so you will need to loop over the result of
getTables and execute those methods for each row of the getTables result set.

https://stackoverflow.com/a/40841486

cgardens · 2021-03-17T18:23:27Z

...ectors/source-jdbc/src/main/java/io/airbyte/integrations/source/jdbc/AbstractJdbcSource.java

+        final List<String> primaryKeys = database.bufferedResultSetQuery(
+            conn -> conn.getMetaData().getPrimaryKeys(databaseOptional.orElse(null), t.getSchemaName(), t.getName()),
+            resultSet -> resultSet.getString(JDBC_COLUMN_COLUMN_NAME));
+        t.addPrimaryKeys(primaryKeys);


One convention that we try to follow in our java code is to keep objects immutable wherever possible. This pattern of setting fields after initialization should only be used if absolutely necessary (e.g. it represents different stages of the object's lifecycle). You'll notice that in almost all of our java classes all of the fields are final. Here it is not necessary to set the field late. The integrity of this object is compromised by because we happen to use two separate queries to construct it.

I would suggest the following you do both queries 1. get all columns. 2 get all primary keys. and then construct the table info one time (no need for addPrimaryKeys).

LMK if any of this is unclear or if the motivation doesn't make sense.

cgardens · 2021-03-17T18:34:19Z

...c/src/testFixtures/java/io/airbyte/integrations/source/jdbc/test/JdbcSourceStandardTest.java

      connection.createStatement().execute(
          String.format(
              "INSERT INTO %s(id, name, updated_at) VALUES (1,'picard', '2004-10-19'),  (2, 'crusher', '2005-10-19'), (3, 'vash', '2006-10-19');",
              getFullyQualifiedTableName(TABLE_NAME)));
+


good call adding these test cases here. glad that this could be added without having to rewrite all of the tests!

cgardens · 2021-03-17T18:36:02Z

...c/src/testFixtures/java/io/airbyte/integrations/source/jdbc/test/JdbcSourceStandardTest.java

@@ -87,6 +88,8 @@
  private static final Set<String> TEST_SCHEMAS = ImmutableSet.of(SCHEMA_NAME, SCHEMA_NAME2);

  private static final String TABLE_NAME = "id_and_name";
+  private static final String TABLE_NAME_WITHOUT_PK = "id_and_name_without_pk";
+  private static final String TABLE_NAME_FULL_NAMES = "full_names";


can we call this multi column pk or something? i had trouble parsing this name. wasn't until i read the create table query that i understood it.

Source support primary keys

78e36c9

auto-assign bot requested review from michel-tricot and sherifnada March 16, 2021 16:47

Bumpversion of jdbc source connectors

e8c51ac

sherifnada suggested changes Mar 16, 2021

View reviewed changes

ChristopheDuong added 3 commits March 17, 2021 12:38

Tweaks from code review

47fc7b2

Wording

7af041f

wording

df4bd99

ChristopheDuong requested a review from sherifnada March 17, 2021 13:57

sherifnada approved these changes Mar 17, 2021

View reviewed changes

ChristopheDuong merged commit 41e8b6a into master Mar 17, 2021

ChristopheDuong deleted the chris/source-primary-keys branch March 17, 2021 18:28

cgardens reviewed Mar 17, 2021

View reviewed changes

ChristopheDuong mentioned this pull request Mar 18, 2021

Try to make one query to DB to retrieve primary keys #2514

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Source support primary keys #2488

Source support primary keys #2488

ChristopheDuong commented Mar 16, 2021 •

edited

Loading

sherifnada left a comment

sherifnada Mar 16, 2021

ChristopheDuong commented Mar 17, 2021 •

edited by github-actions bot

Loading

ChristopheDuong commented Mar 17, 2021 •

edited by github-actions bot

Loading

ChristopheDuong commented Mar 17, 2021 •

edited by github-actions bot

Loading

ChristopheDuong commented Mar 17, 2021 •

edited by github-actions bot

Loading

sherifnada left a comment

cgardens Mar 17, 2021

cgardens Mar 17, 2021

ChristopheDuong Mar 18, 2021

cgardens Mar 17, 2021

cgardens Mar 17, 2021

cgardens Mar 17, 2021

		.withSourceDefinedPrimaryKey(
		t.getPrimaryKeys().stream().filter(Objects::nonNull).map(Collections::singletonList).collect(Collectors.toList())))

Source support primary keys #2488

Source support primary keys #2488

Conversation

ChristopheDuong commented Mar 16, 2021 • edited Loading

What

How

Pre-merge Checklist

Recommended reading order

sherifnada left a comment

Choose a reason for hiding this comment

sherifnada Mar 16, 2021

Choose a reason for hiding this comment

ChristopheDuong commented Mar 17, 2021 • edited by github-actions bot Loading

ChristopheDuong commented Mar 17, 2021 • edited by github-actions bot Loading

ChristopheDuong commented Mar 17, 2021 • edited by github-actions bot Loading

ChristopheDuong commented Mar 17, 2021 • edited by github-actions bot Loading

sherifnada left a comment

Choose a reason for hiding this comment

cgardens Mar 17, 2021

Choose a reason for hiding this comment

cgardens Mar 17, 2021

Choose a reason for hiding this comment

ChristopheDuong Mar 18, 2021

Choose a reason for hiding this comment

cgardens Mar 17, 2021

Choose a reason for hiding this comment

cgardens Mar 17, 2021

Choose a reason for hiding this comment

cgardens Mar 17, 2021

Choose a reason for hiding this comment

ChristopheDuong commented Mar 16, 2021 •

edited

Loading

ChristopheDuong commented Mar 17, 2021 •

edited by github-actions bot

Loading

ChristopheDuong commented Mar 17, 2021 •

edited by github-actions bot

Loading

ChristopheDuong commented Mar 17, 2021 •

edited by github-actions bot

Loading

ChristopheDuong commented Mar 17, 2021 •

edited by github-actions bot

Loading