file cdk: handle scalar values that resolve to None #35688

erohmensing · 2024-02-28T01:19:42Z

What

How

Handle cases where the python value of a pyarrow scalar is None. This can be due to null values in data, as well as null-like values like NaT (similar to NaN). We previously handled this for None binary types, but now handle this for None of any type.

🚨 User Impact 🚨

No breaking changes. After this CDK version is released we should update the CDK dependency in S3 and any other file sources that parse parquet

Pre-merge Actions

Expand the relevant checklist and delete the others.

New Connector

Community member or Airbyter

Community member? Grant edit access to maintainers (instructions)
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.
Connector version is set to 0.0.1
- Dockerfile has version 0.0.1
Documentation updated
- Connector's README.md
- Connector's bootstrap.md. See description and examples
- docs/integrations/<source or destination>/<name>.md including changelog with an entry for the initial version. See changelog example
- docs/integrations/README.md

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.

Updating a connector

Community member or Airbyter

Grant edit access to maintainers (instructions)
Unit & integration tests added

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.

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:generateScaffolds then checking in your changes
Documentation which references the generator is updated as needed

Updating the Python CDK

Airbyter

Before merging:

Pull Request description explains what problem it is solving
Code change is unit tested
Build and my-py check pass
Smoke test the change on at least one affected connector
- On Github: Run this workflow, passing --use-local-cdk --name=source-<connector> as options
- Locally: airbyte-ci connectors --use-local-cdk --name=source-<connector> test
PR is reviewed and approved

After merging:

Publish the CDK
- The CDK does not follow proper semantic versioning. Choose minor if this the change has significant user impact or is a breaking change. Choose patch otherwise.
- Write a thoughtful changelog message so we know what was updated.
Merge the platform PR that was auto-created for updating the Connector Builder's CDK version
- This step is optional if the change does not affect the connector builder or declarative connectors.

vercel · 2024-02-28T01:19:46Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment

Name	Status	Preview	Comments	Updated (UTC)
airbyte-docs	⬜️ Ignored (Inspect)	Visit Preview		Mar 5, 2024 4:57pm

erohmensing · 2024-02-28T01:19:52Z

file cdk: handle scalar values that resolve to None #35688 👈
file cdk: fix typing, pull out non-scalar handling #35687
master

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @erohmensing and the rest of your teammates on Graphite

clnoll · 2024-02-28T21:54:32Z

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

+@pytest.mark.parametrize(
+    "parquet_type, parquet_format",
+    [
+        pytest.param(pa.bool_(), _default_parquet_format, id="test_parquet_bool"),
+        pytest.param(pa.int8(), _default_parquet_format, id="test_parquet_int8"),
+        pytest.param(pa.int16(), _default_parquet_format, id="test_parquet_int16"),
+        pytest.param(pa.int32(), _default_parquet_format, id="test_parquet_int32"),
+        pytest.param(pa.int64(), _default_parquet_format, id="test_parquet_int64"),
+        pytest.param(pa.uint8(), _default_parquet_format, id="test_parquet_uint8"),
+        pytest.param(pa.uint16(), _default_parquet_format, id="test_parquet_uint16"),
+        pytest.param(pa.uint32(), _default_parquet_format, id="test_parquet_uint32"),
+        pytest.param(pa.uint64(), _default_parquet_format, id="test_parquet_uint64"),
+        pytest.param(pa.float16(), _default_parquet_format, id="test_parquet_float16"),
+        pytest.param(pa.float32(), _default_parquet_format, id="test_parquet_float32"),
+        pytest.param(pa.float64(), _default_parquet_format, id="test_parquet_float64"),
+        pytest.param(pa.time32("s"), _default_parquet_format, id="test_parquet_time32s"),
+        pytest.param(pa.time32("ms"), _default_parquet_format, id="test_parquet_time32ms"),
+        pytest.param(pa.time64("us"), _default_parquet_format, id="test_parquet_time64us"),
+        pytest.param(pa.time64("ns"), _default_parquet_format, id="test_parquet_time64ns"),
+        pytest.param(pa.timestamp("s"), _default_parquet_format, id="test_parquet_timestamps_s"),
+        pytest.param(pa.timestamp("ms"), _default_parquet_format, id="test_parquet_timestamp_ms"),
+        pytest.param(pa.timestamp("s", "utc"), _default_parquet_format, id="test_parquet_timestamps_s_with_tz"),
+        pytest.param(pa.timestamp("ms", "est"), _default_parquet_format, id="test_parquet_timestamps_ms_with_tz"),
+        pytest.param(pa.date32(), _default_parquet_format, id="test_parquet_date32"),
+        pytest.param(pa.date64(), _default_parquet_format, id="test_parquet_date64"),
+        pytest.param(pa.duration("s"), _default_parquet_format, id="test_duration_s"),
+        pytest.param(pa.duration("ms"), _default_parquet_format, id="test_duration_ms"),
+        pytest.param(pa.duration("us"), _default_parquet_format, id="test_duration_us"),
+        pytest.param(pa.duration("ns"), _default_parquet_format, id="test_duration_ns"),
+        pytest.param(pa.month_day_nano_interval(), _default_parquet_format, id="test_parquet_month_day_nano_interval"),
+        pytest.param(pa.binary(), _default_parquet_format, id="test_binary"),
+        pytest.param(pa.binary(2), _default_parquet_format, id="test_fixed_size_binary"),
+        pytest.param(pa.string(), _default_parquet_format, id="test_parquet_string"),
+        pytest.param(pa.utf8(), _default_parquet_format, id="test_utf8"),
+        pytest.param(pa.large_binary(), _default_parquet_format, id="test_large_binary"),
+        pytest.param(pa.large_string(), _default_parquet_format, id="test_large_string"),
+        pytest.param(pa.large_utf8(), _default_parquet_format, id="test_large_utf8"),
+        pytest.param(pa.dictionary(pa.int32(), pa.string()), _default_parquet_format, id="test_dictionary"),
+        pytest.param(pa.struct([pa.field("field", pa.int32())]), _default_parquet_format, id="test_struct"),
+        pytest.param(pa.list_(pa.int32()), _default_parquet_format, id="test_list"),
+        pytest.param(pa.large_list(pa.int32()), _default_parquet_format, id="test_large_list"),
+        pytest.param(pa.decimal128(2), _default_parquet_format, id="test_decimal128"),
+        pytest.param(pa.decimal256(2), _default_parquet_format, id="test_decimal256"),
+        pytest.param(pa.decimal128(2), _decimal_as_float_parquet_format, id="test_decimal128_as_float"),
+        pytest.param(pa.decimal256(2), _decimal_as_float_parquet_format, id="test_decimal256_as_float"),
+        pytest.param(pa.map_(pa.int32(), pa.int32()), _default_parquet_format, id="test_map"),
+        pytest.param(pa.null(), _default_parquet_format, id="test_null"),
+    ])


erohmensing · 2024-03-04T22:10:25Z

@clnoll thanks for looking at this! Think it's ready for review (and it's downstack PR)

girarda

🚢

clnoll

LGTM!

erohmensing · 2024-03-05T16:56:30Z

Merge activity

Mar 5, 11:56 AM EST: @erohmensing started a stack merge that includes this pull request via Graphite.
Mar 5, 11:57 AM EST: Graphite rebased this pull request as part of a merge.
Mar 5, 12:07 PM EST: @erohmensing merged this pull request with Graphite.

## What * Fix typing and handling of different types in `_to_output_value` - we don't always get a `Scalar`. We already handle the different cases correctly, but the typing doesn't reflect this. * Splitting out the methods to do the scalar separately is a helpful precursor to #35688, as the `DictionaryArray` object doesn't have an `as_py()` method. ## 🚨 User Impact 🚨 None ## Pre-merge Actions *Expand the relevant checklist and delete the others.* <details><summary>New Connector</summary> ### Community member or Airbyter - **Community member?** Grant edit access to maintainers ([instructions](https://docs.github.com/en/github/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork#enabling-repository-maintainer-permissions-on-existing-pull-requests)) - 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`. - Connector version is set to `0.0.1` - `Dockerfile` has version `0.0.1` - Documentation updated - Connector's `README.md` - Connector's `bootstrap.md`. See [description and examples](https://docs.google.com/document/d/1ypdgmwmEHWv-TrO4_YOQ7pAJGVrMp5BOkEVh831N260/edit?usp=sharing) - `docs/integrations/<source or destination>/<name>.md` including changelog with an entry for the initial version. See changelog [example](https://docs.airbyte.io/integrations/sources/stripe#changelog) - `docs/integrations/README.md` ### 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](https://docs.airbyte.io/connector-development#using-credentials-in-ci). </details> <details><summary>Updating a connector</summary> ### Community member or Airbyter - Grant edit access to maintainers ([instructions](https://docs.github.com/en/github/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork#enabling-repository-maintainer-permissions-on-existing-pull-requests)) - Unit & integration tests added ### 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](https://docs.airbyte.io/connector-development#using-credentials-in-ci). </details> <details><summary>Connector Generator</summary> - Issue acceptance criteria met - PR name follows [PR naming conventions](https://docs.airbyte.com/contributing-to-airbyte/resources/pull-requests-handbook) - If adding a new generator, add it to the [list of scaffold modules being tested](https://github.com/airbytehq/airbyte/blob/master/airbyte-integrations/connector-templates/generator/build.gradle#L41) - 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:generateScaffolds` then checking in your changes - Documentation which references the generator is updated as needed </details> <details><summary>Updating the Python CDK</summary> ### Airbyter Before merging: - Pull Request description explains what problem it is solving - Code change is unit tested - Build and my-py check pass - Smoke test the change on at least one affected connector - On Github: Run [this workflow](https://github.com/airbytehq/airbyte/actions/workflows/connectors_tests.yml), passing `--use-local-cdk --name=source-<connector>` as options - Locally: `airbyte-ci connectors --use-local-cdk --name=source-<connector> test` - PR is reviewed and approved After merging: - [Publish the CDK](https://github.com/airbytehq/airbyte/actions/workflows/publish-cdk-command-manually.yml) - The CDK does not follow proper semantic versioning. Choose minor if this the change has significant user impact or is a breaking change. Choose patch otherwise. - Write a thoughtful changelog message so we know what was updated. - Merge the platform PR that was auto-created for updating the Connector Builder's CDK version - This step is optional if the change does not affect the connector builder or declarative connectors. </details>

## What * Closes #34151 * Closes https://github.com/airbytehq/oncall/issues/4386 ## How Handle cases where the python value of a pyarrow scalar is None. This can be due to null values in data, as well as null-like values like `NaT` (similar to `NaN`). We previously handled this for `None` binary types, but now handle this for `None` of any type. ## 🚨 User Impact 🚨 No breaking changes. After this CDK version is released we should update the CDK dependency in S3 and any other file sources that parse parquet ## Pre-merge Actions *Expand the relevant checklist and delete the others.* <details><summary>New Connector</summary> ### Community member or Airbyter - **Community member?** Grant edit access to maintainers ([instructions](https://docs.github.com/en/github/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork#enabling-repository-maintainer-permissions-on-existing-pull-requests)) - 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`. - Connector version is set to `0.0.1` - `Dockerfile` has version `0.0.1` - Documentation updated - Connector's `README.md` - Connector's `bootstrap.md`. See [description and examples](https://docs.google.com/document/d/1ypdgmwmEHWv-TrO4_YOQ7pAJGVrMp5BOkEVh831N260/edit?usp=sharing) - `docs/integrations/<source or destination>/<name>.md` including changelog with an entry for the initial version. See changelog [example](https://docs.airbyte.io/integrations/sources/stripe#changelog) - `docs/integrations/README.md` ### 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](https://docs.airbyte.io/connector-development#using-credentials-in-ci). </details> <details><summary>Updating a connector</summary> ### Community member or Airbyter - Grant edit access to maintainers ([instructions](https://docs.github.com/en/github/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork#enabling-repository-maintainer-permissions-on-existing-pull-requests)) - Unit & integration tests added ### 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](https://docs.airbyte.io/connector-development#using-credentials-in-ci). </details> <details><summary>Connector Generator</summary> - Issue acceptance criteria met - PR name follows [PR naming conventions](https://docs.airbyte.com/contributing-to-airbyte/resources/pull-requests-handbook) - If adding a new generator, add it to the [list of scaffold modules being tested](https://github.com/airbytehq/airbyte/blob/master/airbyte-integrations/connector-templates/generator/build.gradle#L41) - 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:generateScaffolds` then checking in your changes - Documentation which references the generator is updated as needed </details> <details><summary>Updating the Python CDK</summary> ### Airbyter Before merging: - Pull Request description explains what problem it is solving - Code change is unit tested - Build and my-py check pass - Smoke test the change on at least one affected connector - On Github: Run [this workflow](https://github.com/airbytehq/airbyte/actions/workflows/connectors_tests.yml), passing `--use-local-cdk --name=source-<connector>` as options - Locally: `airbyte-ci connectors --use-local-cdk --name=source-<connector> test` - PR is reviewed and approved After merging: - [Publish the CDK](https://github.com/airbytehq/airbyte/actions/workflows/publish-cdk-command-manually.yml) - The CDK does not follow proper semantic versioning. Choose minor if this the change has significant user impact or is a breaking change. Choose patch otherwise. - Write a thoughtful changelog message so we know what was updated. - Merge the platform PR that was auto-created for updating the Connector Builder's CDK version - This step is optional if the change does not affect the connector builder or declarative connectors. </details>

erohmensing mentioned this pull request Feb 28, 2024

file cdk: fix typing, pull out non-scalar handling #35687

Merged

octavia-squidington-iii added the CDK Connector Development Kit label Feb 28, 2024

erohmensing force-pushed the ella/reproduce-parquet-issue branch from 3b26b28 to faf4d1b Compare February 28, 2024 17:05

erohmensing mentioned this pull request Feb 28, 2024

🐛 Source S3: fix bug when importing parquet files with null datetimes #35650

Closed

clnoll reviewed Feb 28, 2024

View reviewed changes

erohmensing force-pushed the ella/parquet-mypy branch from 498a264 to 0d5d206 Compare March 4, 2024 21:31

erohmensing force-pushed the ella/reproduce-parquet-issue branch from b586f42 to b6af5da Compare March 4, 2024 21:31

erohmensing requested a review from clnoll March 4, 2024 22:09

erohmensing marked this pull request as ready for review March 4, 2024 22:53

erohmensing requested a review from a team March 4, 2024 22:53

girarda approved these changes Mar 5, 2024

View reviewed changes

clnoll reviewed Mar 5, 2024

View reviewed changes

Base automatically changed from ella/parquet-mypy to master March 5, 2024 16:56

erohmensing added 7 commits March 5, 2024 16:57

add test to reproduce error

6ae95b5

add none handling

25966cc

remove extra handling for none binary values

b4e3fb2

add test for none handling for everything, remove specific tests

95dffec

use scalar instead of array

46d877d

use parquet format from test fixture

8ad231d

fix flakecheck

2801326

erohmensing force-pushed the ella/reproduce-parquet-issue branch from b6af5da to 2801326 Compare March 5, 2024 16:57

erohmensing merged commit a090088 into master Mar 5, 2024

erohmensing deleted the ella/reproduce-parquet-issue branch March 5, 2024 17:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

file cdk: handle scalar values that resolve to None #35688

file cdk: handle scalar values that resolve to None #35688

Uh oh!

erohmensing commented Feb 28, 2024 •

edited

Loading

Uh oh!

vercel bot commented Feb 28, 2024 •

edited

Loading

Uh oh!

erohmensing commented Feb 28, 2024 •

edited

Loading

Uh oh!

clnoll Feb 28, 2024

Uh oh!

erohmensing commented Mar 4, 2024

Uh oh!

girarda left a comment

Uh oh!

clnoll left a comment

Uh oh!

erohmensing commented Mar 5, 2024 •

edited

Loading

Uh oh!

Uh oh!

file cdk: handle scalar values that resolve to None #35688

file cdk: handle scalar values that resolve to None #35688

Uh oh!

Conversation

erohmensing commented Feb 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What

How

🚨 User Impact 🚨

Pre-merge Actions

Community member or Airbyter

Airbyter

Community member or Airbyter

Airbyter

Airbyter

Uh oh!

vercel bot commented Feb 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

erohmensing commented Feb 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

clnoll Feb 28, 2024

Choose a reason for hiding this comment

Uh oh!

erohmensing commented Mar 4, 2024

Uh oh!

girarda left a comment

Choose a reason for hiding this comment

Uh oh!

clnoll left a comment

Choose a reason for hiding this comment

Uh oh!

erohmensing commented Mar 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merge activity

Uh oh!

Uh oh!

erohmensing commented Feb 28, 2024 •

edited

Loading

vercel bot commented Feb 28, 2024 •

edited

Loading

erohmensing commented Feb 28, 2024 •

edited

Loading

erohmensing commented Mar 5, 2024 •

edited

Loading