-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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-gcs handling v1 data protocol #20635
🎉 Destination-gcs handling v1 data protocol #20635
Conversation
/test connector=connectors/destination-gcs
Build PassedTest summary info:
|
/test connector=connectors/destination-snowflake
Build PassedTest summary info:
|
/test connector=connectors/destination-bigquery
Build PassedTest summary info:
|
/test connector=connectors/destination-bigquery-denormalized
Build FailedTest summary info:
|
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.
glad to see this didn't require any changes to runtime code - code reuse 💯
@@ -85,7 +91,7 @@ private JsonNode getJsonNode(final AirbyteStream stream, final String name) { | |||
} | |||
|
|||
private Set<Type> getExpectedSchemaType(final JsonNode fieldDefinition) { | |||
final JsonNode typeProperty = fieldDefinition.get("type"); | |||
final JsonNode typeProperty = fieldDefinition.get("type") == null ? fieldDefinition.get("$ref") : fieldDefinition.get("type"); |
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.
Nice work, suggestion to maybe considering a comment here that notes that $ref
is utilized as a result of the version migration and that type
is the original standard since tests are a great way to maintain knowledge of business context
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.
The type
is still valid since we have object and arrays (for example: "type": ["object"]
) in schemas but it will be worth to mentioned the $ref
in the comments since it's not obvious. Thanks for suggesting!
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.
Awesome, both changes look great, and thanks for making this code re-useable
@@ -33,4 +36,23 @@ protected boolean compareDateTimeValues(String airbyteMessageValue, String desti | |||
return super.compareDateTimeValues(airbyteMessageValue, format.format(dateTime)); | |||
} | |||
|
|||
@Override | |||
protected boolean compareTime(final String airbyteMessageValue, final String destinationValue) { | |||
var destinationDate = LocalTime.ofInstant(getInstantFromEpoch(destinationValue), ZoneOffset.UTC); |
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.
Minor nit: instead of var
have this be LocalTime
so it's clear that the difference between this super.compareTime
and this child method is that this is doing a LocalTime
comparison vs String
comparison
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, minor comments but nothing blocking, great usage of code reuse
The fixed tests for bigquery-denormalized #20640 |
/test connector=connectors/destination-gcs
Build PassedTest summary info:
|
What
Destination gcs protocol V1 support
How
The main change is done in Destination s3 protocol update (#20088) plus json-avro-converter (airbytehq/json-avro-converter#17)
This PR has next change:
Recommended reading order
GcsAvroTestDataComparator.java
GcsAvroParquetDestinationAcceptanceTest.java
- V1 protocol dataset$ref
typeGcsCsvDestinationAcceptanceTest.java
- V1 protocol dataset plus$ref
type🚨 User Impact 🚨
No impact