-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
🐛 Destination S3: avro and parquet formats have issues with JsonToAvroSchemaConverter #8574
🐛 Destination S3: avro and parquet formats have issues with JsonToAvroSchemaConverter #8574
Conversation
vmaltsev seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
/test connector=connectors/destination-s3
|
…pace # Conflicts: # docs/integrations/destinations/s3.md
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.
LGTM
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.
@VitaliiMaltsev could you describe the user impact? Is this causing a breaking change for users?
@@ -26,4 +30,12 @@ private String checkFirsCharInStreamName(final String name) { | |||
} | |||
} | |||
|
|||
public String resovleNamespace(String fieldName, List<String> recordFieldNames) { |
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.
typo: resolve
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.
fixed
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.
Also, can you add a test case to DAT to catch this in the future for all destinations?
@sherifnada updated user impact section |
Please advise which test case do you mean for all destinations? This PR makes changes only to the conversion of the Json schema to the Avro schema in the JsonToAvroSchemaConverter class, since the avro schema, according to the documentation, does not allow having records with the same names. JsonToAvroSchemaConverter is only used for Destination S3 and GCS. JsonToAvroSchemaConverter class is covered by unit tests with in the JsonToAvroConverterTest class (test case "schema_with_the_same_object_names") |
/test connector=connectors/destination-s3
|
/test connector=connectors/destination-gcs
|
…port-namespace' into vmaltsev/8242-destination-s3-support-namespace # Conflicts: # airbyte-integrations/connectors/destination-s3/src/main/java/io/airbyte/integrations/destination/s3/avro/JsonToAvroSchemaConverter.java
/test connector=connectors/destination-bigquery-denormalized
|
/publish connector=connectors/destination-s3
|
/publish connector=connectors/destination-gcs
|
formatted |
/publish connector=connectors/destination-bigquery-denormalized
|
What
Sync failed for Github Source (stream commits)
Logs:
How
According to Apache Avro documentation https://avro.apache.org/docs/current/spec.html#names
A schema or protocol may not contain multiple definitions of a fullname.
Added namespace to avro records
Recommended reading order
x.java
y.python
🚨 User Impact 🚨
There should not be any user impact in scope of Airbyte streams processing. This PR only add unique namespace into avro schemas for record types, previous namespace value for avro records was null. Only one potential impact is for further processing avro/parquet files outside of airbyte, cause earlier fullname of avro records was consist from name, with these changes would be consits from namespace.name
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/SUMMARY.md
docs/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 changes