🎉 Destination Local CSV: add custom delimiter #17998

natalyjazzviolin · 2022-10-14T15:31:54Z

What

Adds a dropdown with 5 delimiter options.
Closes #6734

Screen.Recording.2022-12-14.at.5.14.33.PM.mov

How

Adds a delimiter field to the config
Changes the CSVFormat type based on the entered delimiter.

🚨 User Impact 🚨

Are there any breaking changes? What is the end result perceived by the user? If yes, please merge this PR with the 🚨🚨 emoji so changelog authors can further highlight this if needed.

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
If new credentials are required for use in CI, add them to GSM. 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
If new credentials are required for use in CI, add them to GSM. Instructions.
/test connector=connectors/<name> command is passing
New Connector version released on Dockerhub and connector version bumped 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

Tests

Unit

Put your unit tests output here.

Integration

Put your integration tests output here.

Acceptance

Put your acceptance tests output here.

github-actions · 2022-12-14T22:12:48Z

Affected Connector Report

The latest commit has removed all connector-related changes. There are no more dependent connectors for this PR.

grishick

Please add test cases for all new delimiter variants and also bump the version in Dockerfile

...t-integration/java/io/airbyte/integrations/destination/csv/CsvDestinationAcceptanceTest.java

grishick

Shipit!

natalyjazzviolin · 2022-12-16T14:51:48Z

/test connector=connectors/destination-csv

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

Build Passed

Test summary info:

All Passed

grishick · 2022-12-22T01:27:30Z

@natalyjazzviolin I think you need to change this code in CSVDataArgumentsProvider:

ProtocolVersion protocolVersion = getProtocolVersion();

to this:

ProtocolVersion protocolVersion = ProtocolVersion.V0;

because argument providers are instantiated before the test class is instantiated and so they do not have access to non-static methods of the test class.

edgao · 2022-12-22T16:02:26Z

static

heh.... we're doing some spicy reflection things to make this work (there's good reasons for this, @suhomud maybe remembers the details but basically this lets us write the tests without a ton of extra boilerplate. This was a change for the data types stuff).

I'm fine hardcoding it to unblock this PR, but ideally we'd figure out how to make getProtocolVersion work, since other destinations also rely on this behavior.

@suhomud do you know why it's trying to instantiate the argument provider class CSVDataArgumentsProvider, rather than CsvDestinationAcceptanceTest? (also, it's complaining about not having a no-args constructor, but there is - https://github.com/airbytehq/airbyte/pull/17998/files#diff-b97df4466ad6da2af53b421710c1a8c6d2f79abb5f0ce69b318ba142dc898b99R135)

org.junit.platform.commons.JUnitException: Failed to find a no-argument constructor for ArgumentsProvider [io.airbyte.integrations.destination.csv.CsvDestinationAcceptanceTest$CSVDataArgumentsProvider]. Please ensure that a no-argument constructor exists and that the class is either a top-level class or a static nested class

suhomud · 2022-12-22T16:07:01Z

...t-integration/java/io/airbyte/integrations/destination/csv/CsvDestinationAcceptanceTest.java

+    public CSVDataArgumentsProvider(){};
+    @Override
+    public Stream<? extends Arguments> provideArguments(final ExtensionContext context) {
+      ProtocolVersion protocolVersion = getProtocolVersion();


the method io.Airbyte.integrations.standardtest.destination.argproviders.util.ArgumentProviderUtil.getProtocolVersion should be used with test context as a param. So it will looks like:

public Stream<? extends Arguments> provideArguments(final ExtensionContext context) throws Exception { ProtocolVersion protocolVersion = getProtocolVersion(context);

Thanks for the pointer @suhomud! Could you mention ArgumentProviderUtil.getProtocolVersion in the comment for DestinationAcceptanceTest.getProtocolVersion? The current comment says something about reflection, but it does not mention where we use reflection and it also does not provide guidance on how to use getProtocolVersion

grishick · 2022-12-28T21:49:03Z

@natalyjazzviolin I think you need to change this code in CSVDataArgumentsProvider:
ProtocolVersion protocolVersion = getProtocolVersion();
to this:
ProtocolVersion protocolVersion = ProtocolVersion.V0;
because argument providers are instantiated before the test class is instantiated and so they do not have access to non-static methods of the test class.

I think, based on @suhomud 's comment, instead of what I suggested before, this should look like the following:

  public static class CSVDataArgumentsProvider extends DataArgumentsProvider {
    @Override
    public Stream<? extends Arguments> provideArguments(final ExtensionContext context) throws Exception {
      ProtocolVersion protocolVersion = ArgumentProviderUtil.getProtocolVersion(context);
      return Stream.of(
              Arguments.of(EXCHANGE_RATE_CONFIG.getMessageFileVersion(protocolVersion), EXCHANGE_RATE_CONFIG.getCatalogFileVersion(protocolVersion), "\\u002c"),
              Arguments.of(EXCHANGE_RATE_CONFIG.getMessageFileVersion(protocolVersion), EXCHANGE_RATE_CONFIG.getCatalogFileVersion(protocolVersion), "\\u003b"),
              Arguments.of(EXCHANGE_RATE_CONFIG.getMessageFileVersion(protocolVersion), EXCHANGE_RATE_CONFIG.getCatalogFileVersion(protocolVersion), "\\u007c"),
              Arguments.of(EXCHANGE_RATE_CONFIG.getMessageFileVersion(protocolVersion), EXCHANGE_RATE_CONFIG.getCatalogFileVersion(protocolVersion), "\\u0009"),
              Arguments.of(EXCHANGE_RATE_CONFIG.getMessageFileVersion(protocolVersion), EXCHANGE_RATE_CONFIG.getCatalogFileVersion(protocolVersion), "\\u0020")
      );
    }
  }

@suhomud does the above make sense?
P.S.: I ran /gradlew :airbyte-integrations:connectors:destination-csv:integrationTest with the above change and it worked

… class static.

natalyjazzviolin · 2023-01-10T14:48:29Z

/test connector=connectors/destination-csv

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

Build Passed

Test summary info:

All Passed

natalyjazzviolin · 2023-01-10T14:52:25Z

Thank you for the insight, @edgao and @suhomud ! TIL about reflection!

natalyjazzviolin · 2023-01-10T14:56:55Z

/publish connector=connectors/destination-csv

🕑 Publishing the following connectors:
connectors/destination-csv
https://github.com/airbytehq/airbyte/actions/runs/3884506415

Connector	Did it publish?	Were definitions generated?
connectors/destination-csv	✅	✅

if you have connectors that successfully published but failed definition generation, follow step 4 here ▶️

* Adds delimeter to the spec file. Adds function to get delimeter from the config. * New delimiter works, but only checked raw airbyte tables. * Fixes testWriteFailure(), testWriteSuccess() still broken. * Corrects CSVFormat and now all tests pass. * Implements tab separator. * Corrects tooltip on destination settings page. * Creates CSV file with delimiters and prints it as stirng. * Adds try catch block for assertion. Deletes file after test run. * Removes separate format for tab dleimiter, it is not needed. * Cleans up code. * Adds missing bracket. * Adds files from incorrect rebase. * Corrects imports. * Fixes connectors base build. * Corrects Dockerfile version bump. Adds changelog. * Corrects getProtocolVersion method and makes CSVDataArgumentsProvider class static. * auto-bump connector version Co-authored-by: Octavia Squidington III <octavia-squidington-iii@users.noreply.github.com>

github-actions bot added the area/connectors Connector related issues label Oct 14, 2022

octavia-squidington-iv added the connectors/destination/csv label Oct 14, 2022

natalyjazzviolin requested a review from grishick October 14, 2022 15:32

natalyjazzviolin marked this pull request as ready for review October 18, 2022 12:36

natalyjazzviolin marked this pull request as draft October 21, 2022 12:06

natalyjazzviolin marked this pull request as ready for review December 14, 2022 22:11

grishick suggested changes Dec 15, 2022

View reviewed changes

...t-integration/java/io/airbyte/integrations/destination/csv/CsvDestinationAcceptanceTest.java Outdated Show resolved Hide resolved

octavia-squidington-iv added area/documentation Improvements or additions to documentation connectors/source/github labels Dec 15, 2022

natalyjazzviolin temporarily deployed to more-secrets December 15, 2022 21:06 — with GitHub Actions Inactive

natalyjazzviolin temporarily deployed to more-secrets December 15, 2022 21:07 — with GitHub Actions Inactive

natalyjazzviolin temporarily deployed to more-secrets December 15, 2022 21:09 — with GitHub Actions Inactive

natalyjazzviolin temporarily deployed to more-secrets December 15, 2022 21:10 — with GitHub Actions Inactive

octavia-squidington-iv removed the area/documentation Improvements or additions to documentation label Dec 15, 2022

natalyjazzviolin temporarily deployed to more-secrets December 15, 2022 22:37 — with GitHub Actions Inactive

natalyjazzviolin temporarily deployed to more-secrets December 15, 2022 22:38 — with GitHub Actions Inactive

grishick approved these changes Dec 15, 2022

View reviewed changes

natalyjazzviolin temporarily deployed to more-secrets December 16, 2022 14:33 — with GitHub Actions Inactive

octavia-squidington-iv added the area/documentation Improvements or additions to documentation label Dec 16, 2022

natalyjazzviolin temporarily deployed to more-secrets December 16, 2022 16:03 — with GitHub Actions Inactive

natalyjazzviolin removed the connectors/source/github label Dec 16, 2022

natalyjazzviolin requested review from a team as code owners December 17, 2022 00:13

natalyjazzviolin marked this pull request as draft December 20, 2022 19:39

Corrects imports.

2bc591b

natalyjazzviolin force-pushed the nataly/destination-csv-add-delimiter branch from 8f36dc3 to 2bc591b Compare December 20, 2022 19:53

natalyjazzviolin temporarily deployed to more-secrets December 20, 2022 19:58 — with GitHub Actions Inactive

Fixes connectors base build.

a5259f3

natalyjazzviolin temporarily deployed to more-secrets December 20, 2022 21:05 — with GitHub Actions Inactive

natalyjazzviolin temporarily deployed to more-secrets December 20, 2022 21:06 — with GitHub Actions Inactive

Corrects Dockerfile version bump. Adds changelog.

45e4fb9

octavia-squidington-iv added the area/documentation Improvements or additions to documentation label Dec 20, 2022

natalyjazzviolin marked this pull request as ready for review December 20, 2022 22:56

This comment was marked as outdated.

Sign in to view

suhomud reviewed Dec 22, 2022

View reviewed changes

nataly and others added 2 commits January 9, 2023 15:39

Corrects getProtocolVersion method and makes CSVDataArgumentsProvider…

2763cad

… class static.

Merge branch 'master' into nataly/destination-csv-add-delimiter

20480d9

This comment was marked as duplicate.

Sign in to view

auto-bump connector version

fbf4177

octavia-squidington-iii temporarily deployed to more-secrets January 10, 2023 15:10 — with GitHub Actions Inactive

octavia-squidington-iii temporarily deployed to more-secrets January 10, 2023 18:14 — with GitHub Actions Inactive

octavia-squidington-iii temporarily deployed to more-secrets January 10, 2023 18:15 — with GitHub Actions Inactive

natalyjazzviolin merged commit 364973e into master Jan 10, 2023

natalyjazzviolin deleted the nataly/destination-csv-add-delimiter branch January 10, 2023 18:40

octavia-squidington-iii mentioned this pull request Jan 12, 2023

Bump Airbyte version from 0.40.27 to 0.40.28 #21359

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🎉 Destination Local CSV: add custom delimiter #17998

🎉 Destination Local CSV: add custom delimiter #17998

natalyjazzviolin commented Oct 14, 2022 •

edited

Loading

github-actions bot commented Dec 14, 2022 •

edited

Loading

grishick left a comment

grishick left a comment

natalyjazzviolin commented Dec 16, 2022 •

edited by github-actions bot

Loading

This comment was marked as outdated.

grishick commented Dec 22, 2022 •

edited

Loading

edgao commented Dec 22, 2022

suhomud Dec 22, 2022

grishick Dec 28, 2022

grishick commented Dec 28, 2022

This comment was marked as duplicate.

natalyjazzviolin commented Jan 10, 2023 •

edited by github-actions bot

Loading

natalyjazzviolin commented Jan 10, 2023

natalyjazzviolin commented Jan 10, 2023 •

edited by github-actions bot

Loading

🎉 Destination Local CSV: add custom delimiter #17998

🎉 Destination Local CSV: add custom delimiter #17998

Conversation

natalyjazzviolin commented Oct 14, 2022 • edited Loading

What

How

Recommended reading order

🚨 User Impact 🚨

Pre-merge Checklist

Community member or Airbyter

Airbyter

Community member or Airbyter

Airbyter

Tests

github-actions bot commented Dec 14, 2022 • edited Loading

Affected Connector Report

grishick left a comment

Choose a reason for hiding this comment

grishick left a comment

Choose a reason for hiding this comment

natalyjazzviolin commented Dec 16, 2022 • edited by github-actions bot Loading

Build Passed

This comment was marked as outdated.

grishick commented Dec 22, 2022 • edited Loading

edgao commented Dec 22, 2022

suhomud Dec 22, 2022

Choose a reason for hiding this comment

grishick Dec 28, 2022

Choose a reason for hiding this comment

grishick commented Dec 28, 2022

This comment was marked as duplicate.

natalyjazzviolin commented Jan 10, 2023 • edited by github-actions bot Loading

Build Passed

natalyjazzviolin commented Jan 10, 2023

natalyjazzviolin commented Jan 10, 2023 • edited by github-actions bot Loading

natalyjazzviolin commented Oct 14, 2022 •

edited

Loading

github-actions bot commented Dec 14, 2022 •

edited

Loading

natalyjazzviolin commented Dec 16, 2022 •

edited by github-actions bot

Loading

grishick commented Dec 22, 2022 •

edited

Loading

natalyjazzviolin commented Jan 10, 2023 •

edited by github-actions bot

Loading

natalyjazzviolin commented Jan 10, 2023 •

edited by github-actions bot

Loading