🐛 Destination S3: fixed s3 destination field naming for Parquet and Avro formats #5729

andriikorotkov · 2021-08-30T10:43:46Z

What

The bug is described in detail in task #5149. Issue seems to be that some fields in the source start with a numeric character.

How

To avoid an invalid character bug when writing to avro/parquet, the "_" prefix has been added.

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

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

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.

etsybaev

LGTM, but please make sure all check\tests passed before moving forward

andriikorotkov · 2021-09-02T15:08:45Z

/test connector=connectors/destination-s3

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

…5149-destination-s3-fix

andriikorotkov · 2021-09-02T17:23:39Z

/test connector=connectors/destination-s3

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

andriikorotkov · 2021-09-03T07:26:30Z

/test connector=connectors/destination-s3

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

andriikorotkov · 2021-09-03T09:02:05Z

/test connector=connectors/destination-s3

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

…5149-destination-s3-fix

tuliren

Adding the underscore to every identifier seems unnecessary. Can we do this just for names with invalid characters? I don't want all of our users to have to deal with this extra underscore just because facebook has an API that behaves weirdly. Facebook does not deserve this treatment.

andriikorotkov · 2021-09-07T12:41:48Z

@tuliren I can make the changes you mentioned above. I corrected this in the way the @sherifnada talked about in task #5149. Sherif, could you tell me whether to leave the underscore for all identifiers or do it the way Liren suggested?

sherifnada · 2021-09-07T16:47:47Z

Liren is correct here IMO we should only do it for streams which start with invalid chars

… in the same raw table fix

…5149-destination-s3-fix

…5664-snowflake-s3-copy-fix

sherifnada

LGTM - could you also add a note in the docs about this behavior, and add a unit test for S3NameTransformer?

We might also need to update/publish the GCS destination since it relies on the same S3 utilities

...s/destination-s3/src/main/java/io/airbyte/integrations/destination/s3/S3NameTransformer.java

tuliren

Looks good. Thank you.

Maybe add a unit test?

…5664-snowflake-s3-copy-fix

…5149-destination-s3-fix

andriikorotkov · 2021-09-10T15:08:11Z

@sherifnada, I fixed all the remarks, updated the documentation and added tests. the problem is that not all jobs are successful. can i merge this pull request without it?

sherifnada · 2021-09-10T16:20:08Z

@andriikorotkov yup

…5664-snowflake-s3-copy-fix

…bytehq/airbyte into akorotkov/5149-destination-s3-fix

…5149-destination-s3-fix

andriikorotkov · 2021-09-13T07:40:32Z

/test connector=connectors/destination-s3

🕑 connectors/destination-s3 https://github.com/airbytehq/airbyte/actions/runs/1228615578
❌ connectors/destination-s3 https://github.com/airbytehq/airbyte/actions/runs/1228615578
🐛 https://gradle.com/s/y2lvt3vj4svhu

andriikorotkov · 2021-09-13T08:11:44Z

/publish connector=connectors/destination-s3

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

andriikorotkov · 2021-09-13T13:50:55Z

/test connector=connectors/destination-s3

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

andriikorotkov · 2021-09-13T14:00:33Z

/publish connector=connectors/destination-s3

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

fixed s3 destination field naming for Parquet and Avro formats

34add9a

andriikorotkov requested review from etsybaev and irynakruk August 30, 2021 10:43

github-actions bot added area/connectors Connector related issues area/documentation Improvements or additions to documentation labels Aug 30, 2021

pull request number update

a8fbb9d

andriikorotkov linked an issue Aug 30, 2021 that may be closed by this pull request

Destination S3 SchemaParseException: Illegal initial character #5149

Closed

andriikorotkov changed the title ~~Destination S3: fixed s3 destination field naming for Parquet and Avro formats~~ 🐛 Destination S3: fixed s3 destination field naming for Parquet and Avro formats Aug 31, 2021

etsybaev approved these changes Sep 1, 2021

View reviewed changes

andriikorotkov requested a review from alexandr-shegeda September 2, 2021 12:23

irynakruk approved these changes Sep 2, 2021

View reviewed changes

jrhizor temporarily deployed to more-secrets September 2, 2021 15:11 Inactive

Merge branch 'master' of github.com:airbytehq/airbyte into akorotkov/…

45407f5

…5149-destination-s3-fix

jrhizor temporarily deployed to more-secrets September 2, 2021 17:26 Inactive

jrhizor temporarily deployed to more-secrets September 3, 2021 07:29 Inactive

updated resources for parquet

266928e

jrhizor temporarily deployed to more-secrets September 3, 2021 09:04 Inactive

andriikorotkov requested review from tuliren and subodh1810 September 3, 2021 11:28

andriikorotkov marked this pull request as ready for review September 3, 2021 11:28

Merge branch 'master' of github.com:airbytehq/airbyte into akorotkov/…

feb83c1

…5149-destination-s3-fix

tuliren suggested changes Sep 6, 2021

View reviewed changes

andriikorotkov added 2 commits September 8, 2021 21:39

snowflake s3 destination COPY is writing records from different table…

beb4030

… in the same raw table fix

Merge branch 'master' of github.com:airbytehq/airbyte into akorotkov/…

1c461ec

…5149-destination-s3-fix

andriikorotkov added 4 commits September 9, 2021 16:16

Merge branch 'master' of github.com:airbytehq/airbyte into akorotkov/…

9f740f4

…5149-destination-s3-fix

updated code style

d453953

updated code style

a7cd3ca

Merge branch 'master' of github.com:airbytehq/airbyte into akorotkov/…

d11148a

…5664-snowflake-s3-copy-fix

sherifnada approved these changes Sep 9, 2021

View reviewed changes

...s/destination-s3/src/main/java/io/airbyte/integrations/destination/s3/S3NameTransformer.java Outdated Show resolved Hide resolved

tuliren approved these changes Sep 9, 2021

View reviewed changes

andriikorotkov added 6 commits September 10, 2021 12:58

updated redshift destination

a63832b

Merge branch 'master' of github.com:airbytehq/airbyte into akorotkov/…

90ddd74

…5664-snowflake-s3-copy-fix

added test data for test filed with bad first char

9bcc52b

Merge branch 'master' of github.com:airbytehq/airbyte into akorotkov/…

235a01a

…5149-destination-s3-fix

updated s3 documentation

a2676b1

fixed remarks

e36d1ab

andriikorotkov added 4 commits September 13, 2021 10:06

Merge branch 'master' of github.com:airbytehq/airbyte into akorotkov/…

cd13b97

…5664-snowflake-s3-copy-fix

Merge branch 'akorotkov/5664-snowflake-s3-copy-fix' of github.com:air…

6b66cd5

…bytehq/airbyte into akorotkov/5149-destination-s3-fix

Merge branch 'master' of github.com:airbytehq/airbyte into akorotkov/…

cd043bc

…5149-destination-s3-fix

fixed code style

9ffa857

jrhizor temporarily deployed to more-secrets September 13, 2021 07:42 Inactive

jrhizor temporarily deployed to more-secrets September 13, 2021 08:14 Inactive

fixed s3 tests

e3f783a

jrhizor temporarily deployed to more-secrets September 13, 2021 13:53 Inactive

jrhizor temporarily deployed to more-secrets September 13, 2021 14:02 Inactive

andriikorotkov merged commit 4ffca60 into master Sep 13, 2021

andriikorotkov deleted the akorotkov/5149-destination-s3-fix branch September 13, 2021 14:10

jrhizor mentioned this pull request Sep 16, 2021

Bump Airbyte version from 0.29.17-alpha to 0.29.18-alpha #6125

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🐛 Destination S3: fixed s3 destination field naming for Parquet and Avro formats #5729

🐛 Destination S3: fixed s3 destination field naming for Parquet and Avro formats #5729

andriikorotkov commented Aug 30, 2021 •

edited

Loading

etsybaev left a comment

andriikorotkov commented Sep 2, 2021 •

edited by github-actions bot

Loading

andriikorotkov commented Sep 2, 2021 •

edited by github-actions bot

Loading

andriikorotkov commented Sep 3, 2021 •

edited by github-actions bot

Loading

andriikorotkov commented Sep 3, 2021 •

edited by github-actions bot

Loading

tuliren left a comment

andriikorotkov commented Sep 7, 2021

sherifnada commented Sep 7, 2021

sherifnada left a comment

tuliren left a comment

andriikorotkov commented Sep 10, 2021

sherifnada commented Sep 10, 2021

andriikorotkov commented Sep 13, 2021 •

edited by github-actions bot

Loading

andriikorotkov commented Sep 13, 2021 •

edited by github-actions bot

Loading

andriikorotkov commented Sep 13, 2021 •

edited by github-actions bot

Loading

andriikorotkov commented Sep 13, 2021 •

edited by github-actions bot

Loading

🐛 Destination S3: fixed s3 destination field naming for Parquet and Avro formats #5729

🐛 Destination S3: fixed s3 destination field naming for Parquet and Avro formats #5729

Conversation

andriikorotkov commented Aug 30, 2021 • edited Loading

What

How

Pre-merge Checklist

Community member or Airbyter

Airbyter

Community member or Airbyter

Airbyter

etsybaev left a comment

Choose a reason for hiding this comment

andriikorotkov commented Sep 2, 2021 • edited by github-actions bot Loading

andriikorotkov commented Sep 2, 2021 • edited by github-actions bot Loading

andriikorotkov commented Sep 3, 2021 • edited by github-actions bot Loading

andriikorotkov commented Sep 3, 2021 • edited by github-actions bot Loading

tuliren left a comment

Choose a reason for hiding this comment

andriikorotkov commented Sep 7, 2021

sherifnada commented Sep 7, 2021

sherifnada left a comment

Choose a reason for hiding this comment

tuliren left a comment

Choose a reason for hiding this comment

andriikorotkov commented Sep 10, 2021

sherifnada commented Sep 10, 2021

andriikorotkov commented Sep 13, 2021 • edited by github-actions bot Loading

andriikorotkov commented Sep 13, 2021 • edited by github-actions bot Loading

andriikorotkov commented Sep 13, 2021 • edited by github-actions bot Loading

andriikorotkov commented Sep 13, 2021 • edited by github-actions bot Loading

andriikorotkov commented Aug 30, 2021 •

edited

Loading

andriikorotkov commented Sep 2, 2021 •

edited by github-actions bot

Loading

andriikorotkov commented Sep 2, 2021 •

edited by github-actions bot

Loading

andriikorotkov commented Sep 3, 2021 •

edited by github-actions bot

Loading

andriikorotkov commented Sep 3, 2021 •

edited by github-actions bot

Loading

andriikorotkov commented Sep 13, 2021 •

edited by github-actions bot

Loading

andriikorotkov commented Sep 13, 2021 •

edited by github-actions bot

Loading

andriikorotkov commented Sep 13, 2021 •

edited by github-actions bot

Loading

andriikorotkov commented Sep 13, 2021 •

edited by github-actions bot

Loading