-
Notifications
You must be signed in to change notification settings - Fork 4.6k
Destination-S3: Fix AVRO-JSON Int overflow by changing to Long #14033
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
Conversation
Hi @hbd thanks for the pr. Could you sign the CLA so that we can get continue with the review process and get your change merged? |
@sajarin done! |
Hey @hbd, looks like the change to a long type breaks some of the unit tests and so the build fails. Could you also update the unit tests? |
@sajarin where are you seeing those unit tests fail? I'm seeing all workflows passing |
/test connector=connectors/destination-s3
Build FailedTest summary info:
|
@hbd can you see the failed tests in the run above now? Looks like some tests that perform some type checking are breaking as a result of the change. |
@sajarin thanks for your help! I believe the PR better addresses the issue which this PR aimed to solve: #13483 The primary difference is that PR requires updating the respective schemas (for example, Shopify I'll close this for now, since it's better addressed as a change to the source connector schemas. |
What
For many sources, the
Schema.Type.INT
type will overflow in converting JSON to AVRO. For example, Shopify IDs require a long and overflow theINT
used today.How
This PR proposes to change the
INT
to aLONG
to fix the overflow issue.Recommended reading order
airbyte-integrations/connectors/destination-s3/src/main/java/io/airbyte/integrations/destination/s3/avro/JsonSchemaType.java
🚨 User Impact 🚨
This should not be a breaking change, since generally migrating from Int -> Long is backwards compatible (the reverse not being true).
Pre-merge Checklist
Expand the relevant checklist and delete the others.
New Connector
Community member or Airbyter
airbyte_secret
./gradlew :airbyte-integrations:connectors:<name>:integrationTest
.README.md
bootstrap.md
. See description and examplesdocs/integrations/<source or destination>/<name>.md
including changelog. See changelog exampledocs/integrations/README.md
airbyte-integrations/builds.md
Airbyter
If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.
/test connector=connectors/<name>
command is passing/publish
command described hereUpdating a connector
Community member or Airbyter
airbyte_secret
./gradlew :airbyte-integrations:connectors:<name>:integrationTest
.README.md
bootstrap.md
. See description and examplesdocs/integrations/<source or destination>/<name>.md
including changelog. See changelog exampleAirbyter
If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.
/test connector=connectors/<name>
command is passing/publish
command described hereConnector Generator
-scaffold
in their name) have been updated with the latest scaffold by running./gradlew :airbyte-integrations:connector-templates:generator:testScaffoldTemplates
then checking in your changesTests
Unit
Put your unit tests output here.
Integration
Put your integration tests output here.
Acceptance
Put your acceptance tests output here.