🎉 Destination S3 Support writing timestamps #7732

VitaliiMaltsev · 2021-11-08T11:10:10Z

What

Support writing timestamps for S3 in Avro and Parquet formats

How

According avro documentation https://avro.apache.org/docs/current/spec.html#Date.
Date

A date logical type annotates an Avro int, where the int stores the number of days from the unix epoch, 1 January 1970 (ISO calendar).

Time (microsecond precision)

A time-micros logical type annotates an Avro long, where the long stores the number of microseconds after midnight, 00:00:00.000000.

Timestamp (microsecond precision)

A timestamp-micros logical type annotates an Avro long, where the long stores the number of microseconds from the unix epoch, 1 January 1970 00:00:00.000000 UTC.

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-08T11:10:26Z

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.}

# Conflicts: # airbyte-integrations/connectors/destination-s3/src/main/java/io/airbyte/integrations/destination/s3/avro/JsonToAvroSchemaConverter.java # airbyte-integrations/connectors/destination-s3/src/main/java/io/airbyte/integrations/destination/s3/parquet/S3ParquetWriter.java

tuliren

Sorry about the delay.

I spent quite some time on this, and the main concern I have for the current implementation is that we need to iterate through the Json object and modify it.

I think a better approach is to 1) keep the Json -> Avro schema converter change in this PR, 2) update the Json -> Avro object converter in https://github.com/airbytehq/json-avro-converter. Here is probably the entry point.

In this way, the Json object does not need to be mutated, and it is only iterated through once (in the Json -> Avro object converter). It also makes support more formats easier in the future, because we don't need to maintain the mutation methods in AvroRecordHelper.java. Instead, the object converter already has a pretty solid iteration framework that we can extend.

Let me know what you think.

VitaliiMaltsev · 2021-11-15T17:27:49Z

@VitaliiMaltsev, I have added you as a maintainer of that repo. By the way, when you create PR, please change the base of the PR to airbytehq/json-avro-converter (highlighted in blue):

Otherwise the PR will be created on the original repo.

created PR airbytehq/json-avro-converter#9. Please review

… conversion

tuliren

Nice.

Can you also update the doc here to include how the logical types you added here are processed?

tuliren · 2021-11-30T06:39:43Z

...-s3/src/main/java/io/airbyte/integrations/destination/s3/avro/JsonToAvroSchemaConverter.java

+      if ((nonNullFieldTypes
+          .stream().anyMatch(schema -> schema.getLogicalType() != null)) &&
+          (!nonNullFieldTypes.contains(Schema.create(Schema.Type.STRING)))) {
+        nonNullFieldTypes.addLast(Schema.create(Schema.Type.STRING));


Changing the nonNullFieldTypes from the generic List to LinkedList does not seem necessary. The add method on the List interface is equivalent to addLast.

tuliren · 2021-11-30T06:41:46Z

...-s3/src/main/java/io/airbyte/integrations/destination/s3/avro/JsonToAvroSchemaConverter.java

@@ -240,6 +254,11 @@ Schema getNullableFieldTypes(final String fieldName, final JsonNode fieldDefinit
      if (!nonNullFieldTypes.contains(NULL_SCHEMA)) {
        nonNullFieldTypes.add(0, NULL_SCHEMA);
      }
+      if ((nonNullFieldTypes
+          .stream().anyMatch(schema -> schema.getLogicalType() != null)) &&
+          (!nonNullFieldTypes.contains(Schema.create(Schema.Type.STRING)))) {


It's probably better to save Schema.create(Schema.Type.STRING)) as a static private variable to avoid creating the string schema repeatedly. This if block does it twice: one in the check, and one to be appended to the field type list.

tuliren · 2021-11-30T06:46:21Z

...-s3/src/main/java/io/airbyte/integrations/destination/s3/avro/JsonToAvroSchemaConverter.java

@@ -240,6 +254,11 @@ Schema getNullableFieldTypes(final String fieldName, final JsonNode fieldDefinit
      if (!nonNullFieldTypes.contains(NULL_SCHEMA)) {
        nonNullFieldTypes.add(0, NULL_SCHEMA);
      }
+      if ((nonNullFieldTypes


Can you add a comments before this if block to explain why the string schema is added at the end?

Something like:

Logical types are converted to a union of logical type itself and string. The purpose is to default the logical type field to a string, if the value of the logical type field is invalid and cannot be properly processed.

VitaliiMaltsev · 2021-11-30T09:34:34Z

/publish connector=connectors/destination-s3

🕑 connectors/destination-s3 https://github.com/airbytehq/airbyte/actions/runs/1520165378
❌ connectors/destination-s3 https://github.com/airbytehq/airbyte/actions/runs/1520165378

# Conflicts: # airbyte-config/init/src/main/resources/config/STANDARD_DESTINATION_DEFINITION/4816b78f-1489-44c1-9060-4b19d5fa9362.json # airbyte-integrations/connectors/source-mongodb-strict-encrypt/src/test-integration/java/io/airbyte/integrations/source/mongodb/MongodbSourceStrictEncryptAcceptanceTest.java

VitaliiMaltsev · 2021-11-30T09:52:40Z

/publish connector=connectors/destination-s3

🕑 connectors/destination-s3 https://github.com/airbytehq/airbyte/actions/runs/1520234304
✅ connectors/destination-s3 https://github.com/airbytehq/airbyte/actions/runs/1520234304

tuliren · 2021-11-30T21:28:31Z

Follow ups:

Update the Avro schema converter doc about the new logical type support.
Bump and publish the GCS destination connector to pick up this change.

VitaliiMaltsev · 2021-12-01T07:55:04Z

Follow ups:

Update the Avro schema converter doc about the new logical type support.

Bump and publish the GCS destination connector to pick up this change.

@tuliren bumped GCS version and updated documentation #8360
Please review

* get date-time format form json schema * created universal date-time converter * implemented jsonnode transformation for avro and parquet * removed unneeded dependency from build.gradle * fix checkstyle * add DateTimeUtilsTest * add AvroRecordHelperTest * resolve merge conflicts | fix checkstyle * update LocalTime parsing * added String type to avro schema for Logical Types, removed date-time conversion * fix checkstyle * fix checkstyle * added static String schema, added comments * bump version Co-authored-by: vmaltsev <[email protected]>

vmaltsev added 3 commits November 2, 2021 18:17

get date-time format form json schema

e1b334d

created universal date-time converter

b779682

implemented jsonnode transformation for avro and parquet

af6949b

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

removed unneeded dependency from build.gradle

b6c1013

VitaliiMaltsev temporarily deployed to more-secrets November 8, 2021 11:15 Inactive

fix checkstyle

01110b0

VitaliiMaltsev temporarily deployed to more-secrets November 8, 2021 11:18 Inactive

add DateTimeUtilsTest

061b8c9

VitaliiMaltsev temporarily deployed to more-secrets November 8, 2021 14:20 Inactive

add AvroRecordHelperTest

ef275b3

VitaliiMaltsev temporarily deployed to more-secrets November 8, 2021 14:56 Inactive

vmaltsev added 2 commits November 8, 2021 16:58

resolve merge conflicts | fix checkstyle

98345ae

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

VitaliiMaltsev temporarily deployed to more-secrets November 8, 2021 15:10 Inactive

VitaliiMaltsev temporarily deployed to more-secrets November 8, 2021 16:05 Inactive

VitaliiMaltsev changed the title ~~Vmaltsev/4610 destination s3 write timestamps~~ Destination S3 Support writing timestamps Nov 9, 2021

VitaliiMaltsev changed the title ~~Destination S3 Support writing timestamps~~ 🎉 Destination S3 Support writing timestamps Nov 9, 2021

VitaliiMaltsev requested review from etsybaev and yurii-bidiuk November 9, 2021 10:08

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

Destination S3: Support writing timestamps #4610

Closed

yurii-bidiuk approved these changes Nov 9, 2021

View reviewed changes

etsybaev approved these changes Nov 9, 2021

View reviewed changes

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

update LocalTime parsing

a0ace25

VitaliiMaltsev temporarily deployed to more-secrets November 11, 2021 09:31 Inactive

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

tuliren suggested changes Nov 12, 2021

View reviewed changes

tuliren mentioned this pull request Nov 17, 2021

Support date/time logical types in avro airbytehq/json-avro-converter#9

Merged

added String type to avro schema for Logical Types, removed date-time…

50aba8e

… conversion

VitaliiMaltsev temporarily deployed to more-secrets November 18, 2021 10:39 Inactive

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

fix checkstyle

1a87b2f

VitaliiMaltsev temporarily deployed to more-secrets November 18, 2021 11:29 Inactive

vmaltsev added 2 commits November 18, 2021 14:45

fix checkstyle

c58e67f

Merge branch 'master' into vmaltsev/4610-destination-s3-write-timestamps

7b2dad0

VitaliiMaltsev temporarily deployed to more-secrets November 18, 2021 12:48 Inactive

tuliren approved these changes Nov 30, 2021

View reviewed changes

added static String schema, added comments

50a6d53

VitaliiMaltsev temporarily deployed to more-secrets November 30, 2021 09:14 Inactive

jrhizor temporarily deployed to more-secrets November 30, 2021 09:37 Inactive

bump version

e70efd9

VitaliiMaltsev temporarily deployed to more-secrets November 30, 2021 09:50 Inactive

VitaliiMaltsev temporarily deployed to more-secrets November 30, 2021 09:53 Inactive

jrhizor temporarily deployed to more-secrets November 30, 2021 09:54 Inactive

VitaliiMaltsev merged commit 065bbf6 into master Nov 30, 2021

VitaliiMaltsev deleted the vmaltsev/4610-destination-s3-write-timestamps branch November 30, 2021 10:08

jrhizor mentioned this pull request Dec 1, 2021

Bump Airbyte version from 0.32.8-alpha to 0.33.0-alpha #8356

Closed

jrhizor mentioned this pull request Dec 1, 2021

Bump Airbyte version from 0.32.8-alpha to 0.32.9-alpha #8391

Merged

tuliren mentioned this pull request Dec 3, 2021

Fix avro logical type conversion test #8479

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🎉 Destination S3 Support writing timestamps #7732

🎉 Destination S3 Support writing timestamps #7732

VitaliiMaltsev commented Nov 8, 2021 •

edited

Loading

CLAassistant commented Nov 8, 2021

tuliren left a comment

VitaliiMaltsev commented Nov 15, 2021

tuliren left a comment

tuliren Nov 30, 2021

VitaliiMaltsev Nov 30, 2021

tuliren Nov 30, 2021

VitaliiMaltsev Nov 30, 2021

tuliren Nov 30, 2021

VitaliiMaltsev Nov 30, 2021

VitaliiMaltsev commented Nov 30, 2021 •

edited by github-actions bot

Loading

VitaliiMaltsev commented Nov 30, 2021 •

edited by github-actions bot

Loading

tuliren commented Nov 30, 2021

VitaliiMaltsev commented Dec 1, 2021 •

edited

Loading

🎉 Destination S3 Support writing timestamps #7732

🎉 Destination S3 Support writing timestamps #7732

Conversation

VitaliiMaltsev commented Nov 8, 2021 • edited Loading

What

How

Recommended reading order

Pre-merge Checklist

Community member or Airbyter

Airbyter

Community member or Airbyter

Airbyter

CLAassistant commented Nov 8, 2021

tuliren left a comment

Choose a reason for hiding this comment

VitaliiMaltsev commented Nov 15, 2021

tuliren left a comment

Choose a reason for hiding this comment

tuliren Nov 30, 2021

Choose a reason for hiding this comment

VitaliiMaltsev Nov 30, 2021

Choose a reason for hiding this comment

tuliren Nov 30, 2021

Choose a reason for hiding this comment

VitaliiMaltsev Nov 30, 2021

Choose a reason for hiding this comment

tuliren Nov 30, 2021

Choose a reason for hiding this comment

VitaliiMaltsev Nov 30, 2021

Choose a reason for hiding this comment

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

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

tuliren commented Nov 30, 2021

VitaliiMaltsev commented Dec 1, 2021 • edited Loading

VitaliiMaltsev commented Nov 8, 2021 •

edited

Loading

VitaliiMaltsev commented Nov 30, 2021 •

edited by github-actions bot

Loading

VitaliiMaltsev commented Nov 30, 2021 •

edited by github-actions bot

Loading

VitaliiMaltsev commented Dec 1, 2021 •

edited

Loading