-
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
Bmoric/fix diff #16513
Bmoric/fix diff #16513
Conversation
fieldNamesThatAreOneOfs)) | ||
.map(fieldWithSchema -> { | ||
if (withNamedOneOf && isOneOfField(fieldWithSchema.getRight())) { | ||
final List<String> newPath = new ArrayList<>(fieldWithSchema.getLeft()); |
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.
Nit: this could be reduced to a one liner: final List<String> newPath = List.of(fieldWithSchema.getLeft(), "oneOf");
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.
I reverted the fix. We shouldn't allow it and have the format being enforced instead.
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.
Re-request review because there was several changes since the last approval. |
@@ -334,10 +335,15 @@ private static UpdateStreamTransform getStreamDiff(final AirbyteStream streamOld | |||
final Set<FieldTransform> fieldTransforms = new HashSet<>(); | |||
final Map<List<String>, JsonNode> fieldNameToTypeOld = getFullyQualifiedFieldNamesWithTypes(streamOld.getJsonSchema()) | |||
.stream() | |||
.collect(Collectors.toMap(Pair::getLeft, Pair::getRight)); | |||
.collect( | |||
() -> new HashMap<>(), |
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.
Nit: I think that you can do HashMap::new
here as well.
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.
Done
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.
* Add test on the catalog diff logic * Simplify test case * Fix the bug * Rm useless val * Format * Update test input * revert map change * Add test * Fix test * Format * Use sugar * Fix PMD errors Co-authored-by: Malik Diarra <malik@airbyte.io>
* Add test on the catalog diff logic * Simplify test case * Fix the bug * Rm useless val * Format * Update test input * revert map change * Add test * Fix test * Format * Use sugar * Fix PMD errors Co-authored-by: Malik Diarra <malik@airbyte.io>
What
Fix the catalog diff, Address https://github.com/airbytehq/oncall/issues/463
How
CatalogHelpers.getFullyQualifiedFieldNamesWithTypes
was including the oneOfField which could lead to duplicated key if the one of is contains in an array (one entry for the array, one entry for the one of).This PR is adding a oneOf indicator as part of the key path which makes it unique. This doesn't change the behavior of the diff or gettingAllFieldName.Edit: we now consider the field as updated if there is an issue.