Destination Oracle: change table limit and format record data #5542

marcosmarxm · 2021-08-20T01:21:50Z

What

Today we support Oracle minimum version 11 source and destination.
Unfortunately Oracle 11 doesn't have the json_value function or support json objects. This makes normalization really hard to be implemented.

Suggestion: Removes support for Oracle 11g in destination. The EOL for Oracle 11G was dec-2020.
What this causes to our tests? The unique public container exists is Dockerhub is Oracle 11g (dont know how...). Oracle allows you download Oracle 18 or 19 version from their registry, which could be a way to run integration tests. The problem is this container took ~45 min to spin up which can be really complicated. The viable solution is have a cloud instance similar to Redshift to help us with integration tests.

This modification will support Oracle 12g+ (this is the first version supports table name with 128 chars and json objects). The suggestions to use Oracle 19 in tests is because this version is available in Oracle Registry/AWS RDS.

How

Oracle 19 json_value doesnt allow ' or " in keys so it we need to clean ~~some records~~ the key name using a formatted function. Example: column`_'with\"_quotes becomes column___with\__quotes.

Checklist

create AWS RDS Instance
add secrets
run tests
update docs

sherifnada · 2021-08-23T15:15:37Z

...nation-oracle/src/main/java/io/airbyte/integrations/destination/oracle/OracleOperations.java

@@ -150,8 +152,9 @@ private static void insertRawRecordsInSingleQuery(String tableName,
        int i = 1;
        for (final AirbyteRecordMessage message : records) {
          // 1-indexed
+          final JsonNode formattedData = StandardNameTransformer.formatJsonPath(message.getData());


this is modifying the data itself. Is there any way around this?

agreed with sherif. the name transformer was intended to transform "names" (e.g. table names and column names). in general we want to avoid modifying the data if at all possible.

I'm doing the same as in BigQuery destination (this change the json path to remove any non-supported char in the stream name not change the original data):

airbyte/airbyte-integrations/connectors/destination-bigquery/src/main/java/io/airbyte/integrations/destination/bigquery/BigQueryRecordConsumer.java

Lines 124 to 129 in 1954284

final JsonNode formattedData = StandardNameTransformer.formatJsonPath(recordMessage.getData());

return Jsons.jsonNode(ImmutableMap.of(

JavaBaseConstants.COLUMN_NAME_AB_ID, UUID.randomUUID().toString(),

JavaBaseConstants.COLUMN_NAME_DATA, Jsons.serialize(formattedData),

JavaBaseConstants.COLUMN_NAME_EMITTED_AT, formattedEmittedAt));

}

I updated the comment in PR, sorry the confuction: Oracle 19 json_value doesnt allow ' or " in keys so it we need to clean ~~some records~~ the key name using a formatted function. Example: column`_'with\"_quotes becomes column___with\__quotes.

Example of AIRBYTE_DATA saved using this function. The original data is kept but to run normalization I need a "cleaned" field name.

'{"date":"2020-08-29T00:00:00Z", "partition":{"DATA":[{"currency":"EUR"}], "double_array_data":[[{"id":"EUR"}]], "column___with__quotes":[{"currency":"EUR"}], --formatted "column`_\'with\\"_quotes":[{"currency":"EUR"}]}, --original "id":4.2}'

fair enough. this feels like a weird sacrifice we are making for dbt.

This is something needed for the destination itself because the source is writing a json key not supported.

cgardens

Removes support for Oracle 11g in destination.

👍 Fine with me. Is there some way that we can check the version of a database at runtime? e.g. during check connection. If so, can we throw a useful error message if they are using a version of the db that is too old.

What this causes to our tests? The unique public container exists is Dockerhub is Oracle 11g (dont know how...). Oracle allows you download Oracle 18 or 19 version from their registry, which could be a way to run integration tests. The problem is this container took ~45 min to spin up which can be really complicated. The viable solution is have a cloud instance similar to Redshift to help us with integration tests.

I don't understand what you're saying here. Is is possible to user a container with newer versions of oracle that is faster to start up? or are you saying even with newer versions it is really slow? I am fine with use using newer versions of oracle db in the tests if it allows us to user a docker container that is faster to spin up.

cgardens · 2021-08-23T16:10:47Z

...nation-oracle/src/main/java/io/airbyte/integrations/destination/oracle/OracleOperations.java

@@ -150,8 +152,9 @@ private static void insertRawRecordsInSingleQuery(String tableName,
        int i = 1;
        for (final AirbyteRecordMessage message : records) {
          // 1-indexed
+          final JsonNode formattedData = StandardNameTransformer.formatJsonPath(message.getData());


agreed with sherif. the name transformer was intended to transform "names" (e.g. table names and column names). in general we want to avoid modifying the data if at all possible.

marcosmarxm · 2021-08-23T17:58:13Z

I don't understand what you're saying here. Is is possible to user a container with newer versions of oracle that is faster to start up? or are you saying even with newer versions it is really slow? I am fine with use using newer versions of oracle db in the tests if it allows us to user a docker container that is faster to spin up.

@cgardens sorry. The container we use today (Oracle 11g) is pretty fast to spin up. The problem is with Oracle 18/19 that takes 45 min to setup (the newest version...)

cgardens · 2021-08-23T18:29:26Z

The container we use today (Oracle 11g) is pretty fast to spin up. The problem is with Oracle 18/19 that takes 45 min to setup (the newest version...)

That is awful! 😵 Yeah. I guess if that's the case we need to just have an oracle db that we keep running in our integration test environment. @sherifnada is that the right way to do it or is there something better?

sherifnada · 2021-08-23T18:35:32Z

@cgardens yup we should keep one running encoded via terraform

marcosmarxm · 2021-08-23T19:27:59Z

/test connector=connectors/destination-oracle

🕑 connectors/destination-oracle https://github.com/airbytehq/airbyte/actions/runs/1159962596
✅ connectors/destination-oracle https://github.com/airbytehq/airbyte/actions/runs/1159962596

marcosmarxm · 2021-08-23T19:30:28Z

.../test-integration/java/io/airbyte/integrations/destination/oracle/OracleIntegrationTest.java

-    database.query(ctx -> {
-      ctx.execute("alter database default tablespace users");
-      return null;
-    });


I removed this because requires a very strong privilege to execute it. I'm running the AWS instance where I can change the permission of my admin user.

marcosmarxm · 2021-08-23T19:40:44Z

@cgardens yup we should keep one running encoded via terraform

I created #5581 to solve this, I already created an instance in AWS to run tests. Can we follow this PR using that instance?

cgardens · 2021-08-23T23:22:59Z

yeah. i think that's okay.

marcosmarxm · 2021-08-24T02:27:11Z

/publish connector=connectors/destination-oracle

🕑 connectors/destination-oracle https://github.com/airbytehq/airbyte/actions/runs/1160984054
✅ connectors/destination-oracle https://github.com/airbytehq/airbyte/actions/runs/1160984054

* change oracle dest connector * remove container to run tests add depency to secrets/config.json * correct functions in tests * gradlew format

change oracle dest connector

94efc9a

github-actions bot added the area/connectors Connector related issues label Aug 20, 2021

marcosmarxm mentioned this pull request Aug 22, 2021

🎉 Oracle normalization #5562

Merged

9 tasks

marcosmarxm requested review from cgardens and sherifnada August 22, 2021 22:13

sherifnada suggested changes Aug 23, 2021

View reviewed changes

cgardens requested changes Aug 23, 2021

View reviewed changes

remove container to run tests add depency to secrets/config.json

9c5e179

marcosmarxm added 2 commits August 23, 2021 15:50

correct functions in tests

ca7fd95

gradlew format

fe88f45

github-actions bot added area/documentation Improvements or additions to documentation CDK Connector Development Kit labels Aug 23, 2021

Merge branch 'master' into marcosmarxm/oracle-dest

62f70b8

github-actions bot removed the CDK Connector Development Kit label Aug 23, 2021

marcosmarxm commented Aug 23, 2021

View reviewed changes

jrhizor temporarily deployed to more-secrets August 23, 2021 19:30 Inactive

marcosmarxm requested review from sherifnada and cgardens August 23, 2021 20:48

cgardens approved these changes Aug 23, 2021

View reviewed changes

jrhizor temporarily deployed to more-secrets August 24, 2021 02:29 Inactive

marcosmarxm merged commit 29d65e9 into master Aug 24, 2021

marcosmarxm deleted the marcosmarxm/oracle-dest branch August 24, 2021 02:39

avida pushed a commit that referenced this pull request Aug 25, 2021

Destination Oracle: change table limit and format record data (#5542)

29c5c72

* change oracle dest connector * remove container to run tests add depency to secrets/config.json * correct functions in tests * gradlew format

marcosmarxm mentioned this pull request Aug 30, 2021

Destination Oracle: use airbyte default column name + correct ci tests #5746

Merged

36 tasks

marcosmarxm restored the marcosmarxm/oracle-dest branch August 31, 2021 00:00

swyxio deleted the marcosmarxm/oracle-dest branch October 12, 2022 18:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Destination Oracle: change table limit and format record data #5542

Destination Oracle: change table limit and format record data #5542

marcosmarxm commented Aug 20, 2021 •

edited

Loading

sherifnada Aug 23, 2021

cgardens Aug 23, 2021

marcosmarxm Aug 23, 2021

marcosmarxm Aug 23, 2021

marcosmarxm Aug 23, 2021 •

edited

Loading

cgardens Aug 23, 2021

marcosmarxm Aug 24, 2021

cgardens left a comment

cgardens Aug 23, 2021

marcosmarxm commented Aug 23, 2021

cgardens commented Aug 23, 2021

sherifnada commented Aug 23, 2021

marcosmarxm commented Aug 23, 2021 •

edited by github-actions bot

Loading

marcosmarxm Aug 23, 2021

marcosmarxm commented Aug 23, 2021

cgardens commented Aug 23, 2021

marcosmarxm commented Aug 24, 2021 •

edited by github-actions bot

Loading

	final JsonNode formattedData = StandardNameTransformer.formatJsonPath(recordMessage.getData());
	return Jsons.jsonNode(ImmutableMap.of(
	JavaBaseConstants.COLUMN_NAME_AB_ID, UUID.randomUUID().toString(),
	JavaBaseConstants.COLUMN_NAME_DATA, Jsons.serialize(formattedData),
	JavaBaseConstants.COLUMN_NAME_EMITTED_AT, formattedEmittedAt));
	}

Destination Oracle: change table limit and format record data #5542

Destination Oracle: change table limit and format record data #5542

Conversation

marcosmarxm commented Aug 20, 2021 • edited Loading

What

How

Recommended reading order

Checklist

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

marcosmarxm Aug 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cgardens left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcosmarxm commented Aug 23, 2021

cgardens commented Aug 23, 2021

sherifnada commented Aug 23, 2021

marcosmarxm commented Aug 23, 2021 • edited by github-actions bot Loading

Choose a reason for hiding this comment

marcosmarxm commented Aug 23, 2021

cgardens commented Aug 23, 2021

marcosmarxm commented Aug 24, 2021 • edited by github-actions bot Loading

marcosmarxm commented Aug 20, 2021 •

edited

Loading

marcosmarxm Aug 23, 2021 •

edited

Loading

marcosmarxm commented Aug 23, 2021 •

edited by github-actions bot

Loading

marcosmarxm commented Aug 24, 2021 •

edited by github-actions bot

Loading