-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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.
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
[airbyte-cdk] Fix tab delimiter configuration in CSV file type #35901
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Ignored Deployment
|
@marcosmarxm this is a follow-on to the changes from #35246 |
cc @dtiesling |
Thanks @blarghmatey I'm going to take a look today! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you bump the metadata version? This is a CDK change
5aabdc5
to
4bd1142
Compare
@marcosmarxm do you think it will be possible to get this merged and included in a patch release of source-s3 today? |
@maxi297 need your help here. @blarghmatey I'm going try to get this done asap. |
Awesome, thanks! Let me know if there's anything I can help with. |
0085576
to
32e92d1
Compare
@brianjlai can you take a look? |
ffa4d5f
to
a605491
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change makes sense and lines up with how we corrected "\\t"
/r"\t"
into \t
in an older PR for legacy S3.
Thanks for the contribution @blarghmatey !
The tab delimiter is only allowed to be a single character long. In the web UI entering a literal tab character (`\t`) yields an escaped tab character (`\\t`) in the stored configuration. This ensures that the escaped tab character is passed through to the call for `csv.register_dialect` as the proper literal tab `\t`.
@marcosmarxm it looks like your requested change might need to be updated for this to merge? Once this merges, what needs to be done to get the change published in source-s3? |
Running tests to get this merged. @brianjlai will handle the publish action to get this published. |
What
Describe what the change is solving
This fixes use of the tab (
\t
) delimiter for CSV based file sourcesHow
The tab delimiter is only allowed to be a single character long. In the web UI entering a literal tab character (
\t
) yields an escaped tab character (\\t
) in the stored configuration. This ensures that the escaped tab character is passed through to the call forcsv.register_dialect
as the proper literal tab\t
.Recommended reading order
csv_format.py
🚨 User Impact 🚨
Are there any breaking changes? What is the end result perceived by the user?
The end result is that after the CDK change is published and affected sources (e.g. source-s3) are updated then the existing bug of not allowing tab separated files will work again.
For connector PRs, use this section to explain which type of semantic versioning bump occurs as a result of the changes. Refer to our Semantic Versioning for Connectors guidelines for more information. Breaking changes to connectors must be documented by an Airbyte engineer (PR author, or reviewer for community PRs) by using the Breaking Change Release Playbook.
If there are breaking changes, please merge this PR with the 🚨🚨 emoji so changelog authors can further highlight this if needed.
Pre-merge Actions
Expand the relevant checklist and delete the others.
New Connector
Community member or Airbyter
./gradlew :airbyte-integrations:connectors:<name>:integrationTest
.0.0.1
Dockerfile
has version0.0.1
README.md
bootstrap.md
. See description and examplesdocs/integrations/<source or destination>/<name>.md
including changelog with an entry for the initial version. See changelog exampledocs/integrations/README.md
Airbyter
If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.
Updating a connector
Community member or Airbyter
Airbyter
If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.
Connector Generator
-scaffold
in their name) have been updated with the latest scaffold by running./gradlew :airbyte-integrations:connector-templates:generator:generateScaffolds
then checking in your changesUpdating the Python CDK
Airbyter
Before merging:
--use-local-cdk --name=source-<connector>
as optionsairbyte-ci connectors --use-local-cdk --name=source-<connector> test
After merging: