Skip to content
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

Updating default parsing in AvscParser #546

Merged
merged 5 commits into from
Feb 5, 2024
Merged

Conversation

li-ukumar
Copy link
Member

@li-ukumar li-ukumar commented Feb 2, 2024

What

try to get default values from field definitions after schemas in the same file are resolved.

Why

For a complex schema, with cyclic references, it is possible for field type to be unresolved even though it is fully defined inside the file. So, before checking if we can parse default, wait for all schemas in the file to be resolved.

Test

Added Unit test.

@codecov-commenter
Copy link

codecov-commenter commented Feb 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (9487a8b) 46.12% compared to head (7a73bdd) 46.13%.
Report is 1 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff              @@
##             master     #546      +/-   ##
============================================
+ Coverage     46.12%   46.13%   +0.01%     
  Complexity     4539     4539              
============================================
  Files           407      407              
  Lines         28423    28432       +9     
  Branches       4637     4639       +2     
============================================
+ Hits          13109    13117       +8     
+ Misses        13746    13745       -1     
- Partials       1568     1570       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@li-ukumar li-ukumar marked this pull request as draft February 2, 2024 19:31
Copy link
Collaborator

@xiaoyu-yang-gh xiaoyu-yang-gh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add unit tests?

@li-ukumar li-ukumar changed the title Updating parser to process default for unions where 1st type is resolved Updating default parsing in AvscParser Feb 3, 2024
@li-ukumar li-ukumar marked this pull request as ready for review February 3, 2024 00:40
Comment on lines 196 to 199
boolean wasDefinedBefore = fieldSchema.isFullyDefined();
if (!wasDefinedBefore) {
if (!fieldSchema.isFullyDefined()) {
if (hasDefault) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

line 197 and 198 are same. duplicate check.

Comment on lines 202 to 212
continue;
}
}
//fully defined if we're here
if (!hasDefault) {
continue;
}
if (field.getDefaultValue() instanceof AvscUnparsedLiteral) {
AvscParseUtil.lateParseFieldDefault(field, this);
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can refactor these code to remove these two continue to make the logic more clear.

The logic is like:
for (AvroSchemaField field : fields) {
SchemaOrRef fieldSchema = field.getSchemaOrRef();
boolean hasDefault = field.hasDefaultValue();
boolean wasDefinedBefore = fieldSchema.isFullyDefined();
if (!wasDefinedBefore) {
if (hasDefault) {
fieldsWithUnparsedDefaults.add(field);
}
} else {
if (!hasDefault) {
if (field.getDefaultValue() instanceof AvscUnparsedLiteral) {
AvscParseUtil.lateParseFieldDefault(field, this);
}
}
}
}

Or you can move the the check if (hasDefault) ahead:

for (AvroSchemaField field : fields) {
if (!field.hasDefaultValue()) {
continue;
}

SchemaOrRef fieldSchema = field.getSchemaOrRef();
boolean wasDefinedBefore = fieldSchema.isFullyDefined();
if (!wasDefinedBefore) {
fieldsWithUnparsedDefaults.add(field);
} else if (field.getDefaultValue() instanceof AvscUnparsedLiteral) {
AvscParseUtil.lateParseFieldDefault(field, this);
}
}

@li-ukumar li-ukumar merged commit 2de30d1 into master Feb 5, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants