File-based CDK + Source S3 (v4): Pass configured file encoding to stream reader #29110

girarda · 2023-08-04T21:54:18Z

What

Enable S3 v4 to read files with various encoding

How

CDK (csv): Pass the encoding field from the stream config to the stream reader
CDK (json): Pass "utf8" as the encoding to the stream reader
CDK (parquet and avro): Pass None as the encoding to the stream reader
S3 file reader: pass the encoding to smart_open.open

Recommended reading order

airbyte-cdk/python/airbyte_cdk/sources/file_based/file_based_stream_reader.py
airbyte-cdk/python/airbyte_cdk/sources/file_based/file_types/csv_parser.py
airbyte-cdk/python/airbyte_cdk/sources/file_based/file_types/avro_parser.py
airbyte-cdk/python/airbyte_cdk/sources/file_based/file_types/jsonl_parser.py
airbyte-cdk/python/airbyte_cdk/sources/file_based/file_types/parquet_parser.py
airbyte-integrations/connectors/source-s3/source_s3/v4/stream_reader.py

github-actions · 2023-08-04T21:54:37Z

Before Merging a Connector Pull Request

Wow! What a great pull request you have here! 🎉

To merge this PR, ensure the following has been done/considered for each connector added or updated:

PR name follows PR naming conventions
Breaking changes are considered. If a Breaking Change is being introduced, ensure an Airbyte engineer has created a Breaking Change Plan.
Connector version has been incremented in the Dockerfile and metadata.yaml according to our Semantic Versioning for Connectors guidelines
You've updated the connector's metadata.yaml file any other relevant changes, including a breakingChanges entry for major version bumps. See metadata.yaml docs
Secrets in the connector's spec are annotated with airbyte_secret
All documentation files are up to date. (README.md, bootstrap.md, docs.md, etc...)
Changelog updated in docs/integrations/<source or destination>/<name>.md with an entry for the new version. See changelog example
Migration guide updated in docs/integrations/<source or destination>/<name>-migrations.md with an entry for the new version, if the version is a breaking change. See migration guide example
If set, you've ensured the icon is present in the platform-internal repo. (Docs)

If the checklist is complete, but the CI check is failing,

Check for hidden checklists in your PR description
Toggle the github label checklist-action-run on/off to re-run the checklist CI.

…te into alex/file_based_encoding

girarda · 2023-08-04T22:26:59Z

airbyte-integrations/connectors/source-s3/source_s3/v4/stream_reader.py

        try:
            params = {"client": self.s3_client}
        except Exception as exc:
            raise exc

        logger.debug(f"try to open {file.uri}")
        try:
-            result = smart_open.open(f"s3://{self.config.bucket}/{file.uri}", transport_params=params, mode=mode.value)
+            result = smart_open.open(f"s3://{self.config.bucket}/{file.uri}", transport_params=params, mode=mode.value, encoding=encoding)


it's a little tricky to test this until we have the adapter. I tested by merging the codebase with this branch to run the v4 source and reading s3://airbyte-acceptance-test-source-s3/csv_tests/csv_encoded_as_cp1252.csv, which is encoded as cp1252

girarda · 2023-08-04T22:28:07Z

airbyte-cdk/python/airbyte_cdk/sources/file_based/config/csv_format.py

@@ -95,7 +95,7 @@ def validate_quote_char(cls, v: str) -> str:

    @validator("escape_char")
    def validate_escape_char(cls, v: str) -> str:
-        if len(v) != 1:
+        if v is not None and len(v) != 1:


escape_char is an optional field

girarda · 2023-08-04T22:37:15Z

airbyte-cdk/python/airbyte_cdk/sources/file_based/file_types/jsonl_parser.py

@@ -16,6 +16,7 @@
 class JsonlParser(FileTypeParser):

    MAX_BYTES_PER_FILE_FOR_SCHEMA_INFERENCE = 1_000_000
+    ENCODING = "utf8"


encoding isn't configurable in legacy S3 source. We can move this to a config if needed

girarda · 2023-08-04T22:46:29Z

airbyte-cdk/python/airbyte_cdk/sources/file_based/file_types/parquet_parser.py

@@ -50,8 +53,8 @@ def parse_records(
    ) -> Iterable[Dict[str, Any]]:
        parquet_format = config.format[config.file_type] if config.format else ParquetFormat()
        if not isinstance(parquet_format, ParquetFormat):
-            raise ValueError(f"Expected ParquetFormat, got {parquet_format}")  # FIXME test this branch!


added the test https://github.com/airbytehq/airbyte/pull/29110/files#diff-e5776dd8c194a9d538a0a24dabd8256b5eb0b8a2942eea9520d31fc20c4ff951R635

girarda · 2023-08-04T22:57:11Z

airbyte-cdk/python/unit_tests/sources/file_based/file_types/test_csv_parser.py

+        self._expected_encoding = expected_encoding
+
+    def open_file(self, file: RemoteFile, mode: FileReadMode, encoding: Optional[str], logger: logging.Logger) -> io.IOBase:
+        assert encoding == self._expected_encoding


not a great test, but the actual decoding is done outside of the CDK

Why are we defining a class MockFileBasedStreamReader while we could just "Mock(spec=AbstractFileBasedStreamReader)`? Having the mock would allow us to assert calls (and therefore validate arguments like we're doing with this assert

octavia-squidington-iii · 2023-08-04T23:06:17Z

source-s3 test report (commit `8a7af7278c`) - ❌

⏲️ Total pipeline duration: 02mn57s

Step	Result
Validate airbyte-integrations/connectors/source-s3/metadata.yaml	✅
Connector version semver check	✅
Connector version increment check	❌
QA checks	✅
Code format checks	✅
Connector package install	✅
Build source-s3 docker image for platform linux/x86_64	✅
Unit tests	❌

🔗 View the logs here

☁️ View runs for commit in Dagger Cloud

Please note that tests are only run on PR ready for review. Please set your PR to draft mode to not flood the CI engine and upstream service on following commits.
You can run the same pipeline locally on this branch with the airbyte-ci tool with the following command

airbyte-ci connectors --name=source-s3 test

…te into alex/file_based_encoding

octavia-squidington-iii · 2023-08-04T23:38:10Z

source-s3 test report (commit `5fff44ba69`) - ❌

⏲️ Total pipeline duration: 02mn45s

Step	Result
Validate airbyte-integrations/connectors/source-s3/metadata.yaml	✅
Connector version semver check	✅
Connector version increment check	❌
QA checks	✅
Code format checks	❌
Connector package install	✅
Build source-s3 docker image for platform linux/x86_64	✅
Unit tests	❌

🔗 View the logs here

☁️ View runs for commit in Dagger Cloud

Please note that tests are only run on PR ready for review. Please set your PR to draft mode to not flood the CI engine and upstream service on following commits.
You can run the same pipeline locally on this branch with the airbyte-ci tool with the following command

airbyte-ci connectors --name=source-s3 test

octavia-squidington-iii · 2023-08-05T00:19:09Z

source-s3 test report (commit `474de6e105`) - ❌

⏲️ Total pipeline duration: 17mn42s

Step	Result
Validate airbyte-integrations/connectors/source-s3/metadata.yaml	✅
Connector version semver check	✅
Connector version increment check	❌
QA checks	✅
Code format checks	❌
Connector package install	✅
Build source-s3 docker image for platform linux/x86_64	✅
Unit tests	✅
Integration tests	✅
Acceptance tests	✅

🔗 View the logs here

☁️ View runs for commit in Dagger Cloud

Please note that tests are only run on PR ready for review. Please set your PR to draft mode to not flood the CI engine and upstream service on following commits.
You can run the same pipeline locally on this branch with the airbyte-ci tool with the following command

airbyte-ci connectors --name=source-s3 test

octavia-squidington-iii · 2023-08-07T17:10:22Z

source-s3 test report (commit `bf2d50e068`) - ❌

⏲️ Total pipeline duration: 18mn25s

Step	Result
Validate airbyte-integrations/connectors/source-s3/metadata.yaml	✅
Connector version semver check	✅
Connector version increment check	❌
QA checks	✅
Code format checks	❌
Connector package install	✅
Build source-s3 docker image for platform linux/x86_64	✅
Unit tests	✅
Integration tests	✅
Acceptance tests	✅

🔗 View the logs here

☁️ View runs for commit in Dagger Cloud

Please note that tests are only run on PR ready for review. Please set your PR to draft mode to not flood the CI engine and upstream service on following commits.
You can run the same pipeline locally on this branch with the airbyte-ci tool with the following command

airbyte-ci connectors --name=source-s3 test

octavia-squidington-iii · 2023-08-07T18:13:47Z

source-s3 test report (commit `19ce36dcd8`) - ❌

⏲️ Total pipeline duration: 18mn15s

Step	Result
Validate airbyte-integrations/connectors/source-s3/metadata.yaml	✅
Connector version semver check	✅
Connector version increment check	❌
QA checks	✅
Code format checks	❌
Connector package install	✅
Build source-s3 docker image for platform linux/x86_64	✅
Unit tests	✅
Integration tests	✅
Acceptance tests	✅

🔗 View the logs here

☁️ View runs for commit in Dagger Cloud

Please note that tests are only run on PR ready for review. Please set your PR to draft mode to not flood the CI engine and upstream service on following commits.
You can run the same pipeline locally on this branch with the airbyte-ci tool with the following command

airbyte-ci connectors --name=source-s3 test

octavia-squidington-iii · 2023-08-07T18:39:16Z

source-s3 test report (commit `85f8d8e66f`) - ❌

⏲️ Total pipeline duration: 18mn20s

Step	Result
Validate airbyte-integrations/connectors/source-s3/metadata.yaml	✅
Connector version semver check	✅
Connector version increment check	❌
QA checks	✅
Code format checks	✅
Connector package install	✅
Build source-s3 docker image for platform linux/x86_64	✅
Unit tests	✅
Integration tests	✅
Acceptance tests	✅

🔗 View the logs here

☁️ View runs for commit in Dagger Cloud

Please note that tests are only run on PR ready for review. Please set your PR to draft mode to not flood the CI engine and upstream service on following commits.
You can run the same pipeline locally on this branch with the airbyte-ci tool with the following command

airbyte-ci connectors --name=source-s3 test

clnoll

🥇

maxi297

No blocker as my comments only relates to testing

maxi297 · 2023-08-08T13:26:48Z

airbyte-cdk/python/unit_tests/sources/file_based/file_types/test_csv_parser.py

+        self._expected_encoding = expected_encoding
+
+    def open_file(self, file: RemoteFile, mode: FileReadMode, encoding: Optional[str], logger: logging.Logger) -> io.IOBase:
+        assert encoding == self._expected_encoding


Why are we defining a class MockFileBasedStreamReader while we could just "Mock(spec=AbstractFileBasedStreamReader)`? Having the mock would allow us to assert calls (and therefore validate arguments like we're doing with this assert

maxi297 · 2023-08-08T13:28:47Z

airbyte-cdk/python/unit_tests/sources/file_based/scenarios/parquet_scenarios.py

+    )
+    .set_stream_reader(TemporaryParquetFilesStreamReader(files=_single_parquet_file, file_type="parquet"))
+    .set_file_type("parquet")
+    .set_expected_read_error(ConfigValidationError, "Error creating stream config object. Contact Support if you need assistance.")


I guess this is invalid because of the csv, right? Could we have a better error message? Also, could this be a unit test?

octavia-squidington-iii · 2023-08-09T00:28:45Z

source-s3 test report (commit `f0e56fe2a5`) - ❌

⏲️ Total pipeline duration: 23mn00s

Step	Result
Validate airbyte-integrations/connectors/source-s3/metadata.yaml	✅
Connector version semver check	✅
Connector version increment check	❌
QA checks	✅
Code format checks	✅
Connector package install	✅
Build source-s3 docker image for platform linux/x86_64	✅
Unit tests	✅
Integration tests	✅
Acceptance tests	✅

🔗 View the logs here

☁️ View runs for commit in Dagger Cloud

Please note that tests are only run on PR ready for review. Please set your PR to draft mode to not flood the CI engine and upstream service on following commits.
You can run the same pipeline locally on this branch with the airbyte-ci tool with the following command

airbyte-ci connectors --name=source-s3 test

octavia-squidington-iii · 2023-08-09T03:05:17Z

source-s3 test report (commit `b7aed85c08`) - ❌

⏲️ Total pipeline duration: 22mn27s

Step	Result
Validate airbyte-integrations/connectors/source-s3/metadata.yaml	✅
Connector version semver check	✅
Connector version increment check	❌
QA checks	✅
Code format checks	✅
Connector package install	✅
Build source-s3 docker image for platform linux/x86_64	✅
Unit tests	✅
Integration tests	✅
Acceptance tests	✅

🔗 View the logs here

☁️ View runs for commit in Dagger Cloud

Please note that tests are only run on PR ready for review. Please set your PR to draft mode to not flood the CI engine and upstream service on following commits.
You can run the same pipeline locally on this branch with the airbyte-ci tool with the following command

airbyte-ci connectors --name=source-s3 test

octavia-squidington-iii · 2023-08-09T04:41:58Z

source-s3 test report (commit `983b40e46c`) - ❌

⏲️ Total pipeline duration: 20mn12s

Step	Result
Validate airbyte-integrations/connectors/source-s3/metadata.yaml	✅
Connector version semver check	✅
Connector version increment check	❌
QA checks	✅
Code format checks	✅
Connector package install	✅
Build source-s3 docker image for platform linux/x86_64	✅
Unit tests	✅
Integration tests	✅
Acceptance tests	✅

🔗 View the logs here

☁️ View runs for commit in Dagger Cloud

Please note that tests are only run on PR ready for review. Please set your PR to draft mode to not flood the CI engine and upstream service on following commits.
You can run the same pipeline locally on this branch with the airbyte-ci tool with the following command

airbyte-ci connectors --name=source-s3 test

octavia-squidington-iii · 2023-08-09T05:22:59Z

source-s3 test report (commit `78cf5736c6`) - ❌

⏲️ Total pipeline duration: 20mn22s

Step	Result
Validate airbyte-integrations/connectors/source-s3/metadata.yaml	✅
Connector version semver check	✅
Connector version increment check	❌
QA checks	✅
Code format checks	✅
Connector package install	✅
Build source-s3 docker image for platform linux/x86_64	✅
Unit tests	✅
Integration tests	✅
Acceptance tests	✅

🔗 View the logs here

☁️ View runs for commit in Dagger Cloud

Please note that tests are only run on PR ready for review. Please set your PR to draft mode to not flood the CI engine and upstream service on following commits.
You can run the same pipeline locally on this branch with the airbyte-ci tool with the following command

airbyte-ci connectors --name=source-s3 test

girarda · 2023-08-09T14:04:21Z

/approve-and-merge reason="S3 pipeline will not pass because the connector version isn't incremented"

octavia-approvington · 2023-08-09T14:05:03Z

This code is at peace with itself

octavia-squidington-iii · 2023-08-09T14:25:09Z

source-s3 test report (commit `ba6984c841`) - ❌

⏲️ Total pipeline duration: 22mn15s

Step	Result
Validate airbyte-integrations/connectors/source-s3/metadata.yaml	✅
Connector version semver check	✅
Connector version increment check	❌
QA checks	✅
Code format checks	✅
Connector package install	✅
Build source-s3 docker image for platform linux/x86_64	✅
Unit tests	✅
Integration tests	✅
Acceptance tests	✅

🔗 View the logs here

☁️ View runs for commit in Dagger Cloud

Please note that tests are only run on PR ready for review. Please set your PR to draft mode to not flood the CI engine and upstream service on following commits.
You can run the same pipeline locally on this branch with the airbyte-ci tool with the following command

airbyte-ci connectors --name=source-s3 test

…eam reader (airbytehq#29110) * Add encoding to open_file interface * pass the encoding set in the config * cleanup * cleanup * Automated Commit - Formatting Changes * Add missing test * Automated Commit - Formatting Changes * Update infer_schema too * Automated Commit - Formatting Changes * Update unit test * add a unit test * fix * format * format * remove newline * use a mock * fix * format --------- Co-authored-by: girarda <girarda@users.noreply.github.com>

Add encoding to open_file interface

c0fe74f

octavia-squidington-iii added area/connectors Connector related issues CDK Connector Development Kit connectors/source/s3 labels Aug 4, 2023

girarda and others added 10 commits August 4, 2023 15:25

pass the encoding set in the config

6b4d097

cleanup

7f14405

cleanup

bfbf402

Automated Commit - Formatting Changes

109771e

Add missing test

c828d3e

merge

3296be8

Automated Commit - Formatting Changes

fc4f050

Update infer_schema too

806408f

Merge branch 'alex/file_based_encoding' of github.com:airbytehq/airby…

0ef6913

…te into alex/file_based_encoding

Automated Commit - Formatting Changes

8a7af72

girarda changed the title ~~Add encoding to open_file interface~~ File-based CDK + Source S3 (v4): Specify encoding Aug 4, 2023

girarda changed the title ~~File-based CDK + Source S3 (v4): Specify encoding~~ File-based CDK + Source S3 (v4): Pass configured file encoding to stream reader Aug 4, 2023

girarda commented Aug 4, 2023

View reviewed changes

girarda marked this pull request as ready for review August 4, 2023 23:01

girarda requested a review from a team as a code owner August 4, 2023 23:01

girarda added 3 commits August 4, 2023 16:11

Update unit test

908928c

add a unit test

bbc0e2d

Merge branch 'alex/file_based_encoding' of github.com:airbytehq/airby…

5fff44b

…te into alex/file_based_encoding

fix

474de6e

format

bf2d50e

format

19ce36d

remove newline

85f8d8e

girarda requested review from maxi297 and clnoll August 7, 2023 18:45

clnoll approved these changes Aug 8, 2023

View reviewed changes

maxi297 approved these changes Aug 8, 2023

View reviewed changes

girarda added 2 commits August 8, 2023 16:35

use a mock

eca20ae

merge

f0e56fe

fix

b7aed85

format

983b40e

Merge branch 'master' into alex/file_based_encoding

78cf573

girarda enabled auto-merge (squash) August 9, 2023 05:40

girarda disabled auto-merge August 9, 2023 05:40

Merge branch 'master' into alex/file_based_encoding

ba6984c

girarda enabled auto-merge (squash) August 9, 2023 14:01

octavia-approvington approved these changes Aug 9, 2023

View reviewed changes

octavia-approvington merged commit 0aa86cf into master Aug 9, 2023

octavia-approvington deleted the alex/file_based_encoding branch August 9, 2023 14:05

girarda mentioned this pull request Aug 9, 2023

File-based CDK: Support configuring encoding of CSV files #29113

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

File-based CDK + Source S3 (v4): Pass configured file encoding to stream reader #29110

File-based CDK + Source S3 (v4): Pass configured file encoding to stream reader #29110

girarda commented Aug 4, 2023 •

edited

Loading

github-actions bot commented Aug 4, 2023 •

edited by girarda

Loading

girarda Aug 4, 2023

girarda Aug 4, 2023

girarda Aug 4, 2023

girarda Aug 4, 2023

girarda Aug 4, 2023

maxi297 Aug 8, 2023

octavia-squidington-iii commented Aug 4, 2023

octavia-squidington-iii commented Aug 4, 2023

octavia-squidington-iii commented Aug 5, 2023

octavia-squidington-iii commented Aug 7, 2023

octavia-squidington-iii commented Aug 7, 2023

octavia-squidington-iii commented Aug 7, 2023

clnoll left a comment

maxi297 left a comment

maxi297 Aug 8, 2023

maxi297 Aug 8, 2023

octavia-squidington-iii commented Aug 9, 2023

octavia-squidington-iii commented Aug 9, 2023

octavia-squidington-iii commented Aug 9, 2023

octavia-squidington-iii commented Aug 9, 2023

girarda commented Aug 9, 2023

octavia-approvington commented Aug 9, 2023

octavia-squidington-iii commented Aug 9, 2023

File-based CDK + Source S3 (v4): Pass configured file encoding to stream reader #29110

File-based CDK + Source S3 (v4): Pass configured file encoding to stream reader #29110

Conversation

girarda commented Aug 4, 2023 • edited Loading

What

How

Recommended reading order

github-actions bot commented Aug 4, 2023 • edited by girarda Loading

Before Merging a Connector Pull Request

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

octavia-squidington-iii commented Aug 4, 2023

source-s3 test report (commit 8a7af7278c) - ❌

octavia-squidington-iii commented Aug 4, 2023

source-s3 test report (commit 5fff44ba69) - ❌

octavia-squidington-iii commented Aug 5, 2023

source-s3 test report (commit 474de6e105) - ❌

octavia-squidington-iii commented Aug 7, 2023

source-s3 test report (commit bf2d50e068) - ❌

octavia-squidington-iii commented Aug 7, 2023

source-s3 test report (commit 19ce36dcd8) - ❌

octavia-squidington-iii commented Aug 7, 2023

source-s3 test report (commit 85f8d8e66f) - ❌

clnoll left a comment

Choose a reason for hiding this comment

maxi297 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

octavia-squidington-iii commented Aug 9, 2023

source-s3 test report (commit f0e56fe2a5) - ❌

octavia-squidington-iii commented Aug 9, 2023

source-s3 test report (commit b7aed85c08) - ❌

octavia-squidington-iii commented Aug 9, 2023

source-s3 test report (commit 983b40e46c) - ❌

octavia-squidington-iii commented Aug 9, 2023

source-s3 test report (commit 78cf5736c6) - ❌

girarda commented Aug 9, 2023

octavia-approvington commented Aug 9, 2023

octavia-squidington-iii commented Aug 9, 2023

source-s3 test report (commit ba6984c841) - ❌

girarda commented Aug 4, 2023 •

edited

Loading

github-actions bot commented Aug 4, 2023 •

edited by girarda

Loading

source-s3 test report (commit `8a7af7278c`) - ❌

source-s3 test report (commit `5fff44ba69`) - ❌

source-s3 test report (commit `474de6e105`) - ❌

source-s3 test report (commit `bf2d50e068`) - ❌

source-s3 test report (commit `19ce36dcd8`) - ❌

source-s3 test report (commit `85f8d8e66f`) - ❌

source-s3 test report (commit `f0e56fe2a5`) - ❌

source-s3 test report (commit `b7aed85c08`) - ❌

source-s3 test report (commit `983b40e46c`) - ❌

source-s3 test report (commit `78cf5736c6`) - ❌

source-s3 test report (commit `ba6984c841`) - ❌