Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

🎉 Destination S3 & GCS: support additional properties #7288

Merged
merged 19 commits into from
Nov 3, 2021
Merged

🎉 Destination S3 & GCS: support additional properties #7288

merged 19 commits into from
Nov 3, 2021

Conversation

tuliren
Copy link
Contributor

@tuliren tuliren commented Oct 22, 2021 •
edited
Loading

What

How

Recommended reading order

  • Schema conversion: JsonToAvroSchemaConverter.java
  • Schema and object conversion test cases: json_conversion_test_cases.json
  • The rest

Pre-merge Checklist (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
  • Connector version bumped like described here

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

@tuliren tuliren temporarily deployed to more-secrets October 22, 2021 20:42 Inactive
@github-actions github-actions bot added the area/connectors Connector related issues label Oct 22, 2021
@tuliren tuliren changed the title Support additional properties in json schema 🎉 Destination S3 & GCS: support additional properties Oct 22, 2021
@tuliren tuliren temporarily deployed to more-secrets October 22, 2021 21:54 Inactive
@tuliren tuliren temporarily deployed to more-secrets October 22, 2021 23:12 Inactive
@lmossman lmossman mentioned this pull request Oct 22, 2021
Merged
38 tasks
@tuliren tuliren temporarily deployed to more-secrets October 23, 2021 08:26 Inactive
@tuliren tuliren force-pushed the liren/patch-avro-schema-converter branch from e2f0772 to 083e240 Compare October 23, 2021 10:23
@tuliren tuliren temporarily deployed to more-secrets October 23, 2021 10:25 Inactive
@jrhizor jrhizor temporarily deployed to more-secrets October 23, 2021 10:27 Inactive
@tuliren
Copy link
Contributor Author

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

/test connector=connectors/destination-s3

🕑 connectors/destination-s3 https://github.com/airbytehq/airbyte/actions/runs/1375305615
✅ connectors/destination-s3 https://github.com/airbytehq/airbyte/actions/runs/1375305615
No Python unittests run

@tuliren tuliren temporarily deployed to more-secrets October 23, 2021 10:41 Inactive
@jrhizor jrhizor temporarily deployed to more-secrets October 23, 2021 10:42 Inactive
@github-actions github-actions bot added the area/documentation Improvements or additions to documentation label Oct 23, 2021
@tuliren
Copy link
Contributor Author

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

/test connector=connectors/destination-gcs

🕑 connectors/destination-gcs https://github.com/airbytehq/airbyte/actions/runs/1375313805
✅ connectors/destination-gcs https://github.com/airbytehq/airbyte/actions/runs/1375313805
No Python unittests run

@tuliren tuliren temporarily deployed to more-secrets October 23, 2021 10:45 Inactive
@jrhizor jrhizor temporarily deployed to more-secrets October 23, 2021 10:47 Inactive
@tuliren tuliren marked this pull request as ready for review October 23, 2021 11:11
@tuliren tuliren temporarily deployed to more-secrets October 23, 2021 11:12 Inactive
@tuliren tuliren temporarily deployed to more-secrets October 23, 2021 11:15 Inactive
@tuliren tuliren temporarily deployed to more-secrets October 23, 2021 15:21 Inactive
@tuliren
Copy link
Contributor Author

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

/test connector=connectors/destination-s3

🕑 connectors/destination-s3 https://github.com/airbytehq/airbyte/actions/runs/1376175359
✅ connectors/destination-s3 https://github.com/airbytehq/airbyte/actions/runs/1376175359
No Python unittests run

@tuliren
Copy link
Contributor Author

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

/test connector=connectors/destination-gcs

🕑 connectors/destination-gcs https://github.com/airbytehq/airbyte/actions/runs/1376175739
✅ connectors/destination-gcs https://github.com/airbytehq/airbyte/actions/runs/1376175739
No Python unittests run

@tuliren tuliren temporarily deployed to more-secrets October 23, 2021 19:34 Inactive
@jrhizor jrhizor temporarily deployed to more-secrets October 23, 2021 19:35 Inactive
@jrhizor jrhizor temporarily deployed to more-secrets October 23, 2021 19:35 Inactive
sherifnada
sherifnada reviewed Oct 25, 2021
View reviewed changes
...tination-s3/src/test/resources/parquet/json_schema_converter/json_conversion_test_cases.json
}
]
},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if I'm reading this file correctly, the json schema doesn't actually need to have additionalProperties=true for this column to be added/handled correctly, is that right? That seems like a good idea because it makes it resilient to undocumented/unannounced schema changes

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. Most schemas do not specify additionalProperties=true and many of them include additional fields anyway. Eventually, I think we should handle all invalid cases. The S3 destination does not judge. It only hoards!

.../destination-s3/src/main/java/io/airbyte/integrations/destination/s3/avro/AvroConstants.java Outdated Show resolved Hide resolved
@tuliren tuliren force-pushed the liren/patch-avro-schema-converter branch from 241bb5b to 74457e8 Compare November 3, 2021 08:14
@tuliren tuliren temporarily deployed to more-secrets November 3, 2021 08:16 Inactive
@tuliren tuliren temporarily deployed to more-secrets November 3, 2021 08:50 Inactive
@tuliren tuliren temporarily deployed to more-secrets November 3, 2021 09:06 Inactive
@tuliren tuliren temporarily deployed to more-secrets November 3, 2021 09:08 Inactive
@tuliren
Copy link
Contributor Author

tuliren commented Nov 3, 2021 •
edited by github-actions bot
Loading

/test connector=connectors/destination-s3

🕑 connectors/destination-s3 https://github.com/airbytehq/airbyte/actions/runs/1416033463
✅ connectors/destination-s3 https://github.com/airbytehq/airbyte/actions/runs/1416033463
No Python unittests run

@tuliren
Copy link
Contributor Author

tuliren commented Nov 3, 2021 •
edited by github-actions bot
Loading

/test connector=connectors/destination-gcs

🕑 connectors/destination-gcs https://github.com/airbytehq/airbyte/actions/runs/1416034024
✅ connectors/destination-gcs https://github.com/airbytehq/airbyte/actions/runs/1416034024
No Python unittests run

@tuliren
Copy link
Contributor Author

tuliren commented Nov 3, 2021

Confirmed in a local deployment that the new Json Avro converter can properly handle S3 source with additional properties.

@jrhizor jrhizor temporarily deployed to more-secrets November 3, 2021 09:23 Inactive
@jrhizor jrhizor temporarily deployed to more-secrets November 3, 2021 09:23 Inactive
@tuliren
Copy link
Contributor Author

tuliren commented Nov 3, 2021 •
edited
Loading

  • Bump version once the connectors are published.

@tuliren
Copy link
Contributor Author

tuliren commented Nov 3, 2021 •
edited by github-actions bot
Loading

/publish connector=connectors/destination-s3

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

@tuliren
Copy link
Contributor Author

tuliren commented Nov 3, 2021 •
edited by github-actions bot
Loading

/publish connector=connectors/destination-gcs

🕑 connectors/destination-gcs https://github.com/airbytehq/airbyte/actions/runs/1417500194
✅ connectors/destination-gcs https://github.com/airbytehq/airbyte/actions/runs/1417500194

@jrhizor jrhizor temporarily deployed to more-secrets November 3, 2021 15:46 Inactive
@jrhizor jrhizor temporarily deployed to more-secrets November 3, 2021 15:46 Inactive
@tuliren tuliren temporarily deployed to more-secrets November 3, 2021 17:14 Inactive
@tuliren tuliren temporarily deployed to more-secrets November 3, 2021 17:18 Inactive
@tuliren tuliren merged commit c9c41dc into master Nov 3, 2021
@tuliren tuliren deleted the liren/patch-avro-schema-converter branch November 3, 2021 18:37
@tuliren tuliren linked an issue Nov 4, 2021 that may be closed by this pull request
S3 Avro / Parquet output should handle fields of unknown types #4124
Closed
@VitaliiMaltsev VitaliiMaltsev mentioned this pull request Dec 1, 2021
Closed
@sherifnada sherifnada mentioned this pull request Dec 17, 2021
Closed
schlattk pushed a commit to schlattk/airbyte that referenced this pull request Jan 4, 2022
@tuliren @schlattk
🎉 Destination S3 & GCS: support additional properties (airbytehq#7288)
7baae72 
* Log json schema

* Use patched json avro converter

* Rename schema

* Update unit test cases

* Fix ab ap field schema conversion

* Rename files

* Add unit test cases

* Fix dependency for databricks

* Bump versions

* Update documentations

* Update gcs doc

* Set additional properties field name

* Revert s3 and gcs version

* Specify extra props fields

* Refactor json avro conversion doc

* Update connector doc

* Fix databricks spec typo

* Bump connector versions in seed
@NatElkins NatElkins mentioned this pull request Mar 20, 2024
Open
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Phlair Phlair Phlair approved these changes

@sherifnada sherifnada sherifnada approved these changes

Assignees
No one assigned
Labels
area/connectors Connector related issues area/documentation Improvements or additions to documentation
Projects
None yet
Milestone
No milestone
4 participants
@tuliren @sherifnada @Phlair @jrhizor