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

[fast-avro][deserializer] Populate methods always with 'customization' argument #557

Conversation

krisso-rtb
Copy link
Contributor

@krisso-rtb krisso-rtb commented Apr 29, 2024

Populate methods now always have 'customization' argument because deserialize-method(s) of nested records may generate another populate methods that require 'customization' (at least for compilation purposes).

This may happen when record is deserialized with schema having less fields (usually: older schema) AND populate_...() helper method is generated on a field for which recordAction.getShouldRead() == false, so a bit of bad-luck is also needed.

In the added unit-test, without fix the methods chain is:

deserialize(RecordWithOneNullableText reuse, Decoder decoder, DatumReaderCustomization customization)
--> deserializeRecordWithOneNullableText0((reuse), (decoder), (customization))
 --> populate_RecordWithOneNullableTextAndDeeplyNestedRecord0((RecordWithOneNullableTextAndDeeplyNestedRecord), (customization), (decoder));
  --> deserializeNestedRecord0(null, (decoder), (customization));
   --> populate_NestedRecord0((decoder));
    --> deserializeDeeplyNestedRecord0(null, (decoder), (customization)); // ! unknown 'customization' variable

@krisso-rtb krisso-rtb force-pushed the bugfix/populate-method-always-with-customization-argument branch from ad53cc1 to f0e2368 Compare April 30, 2024 05:34
@codecov-commenter
Copy link

codecov-commenter commented Apr 30, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 46.21%. Comparing base (2fd93ac) to head (ff2c378).
Report is 3 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff              @@
##             master     #557      +/-   ##
============================================
- Coverage     46.24%   46.21%   -0.04%     
+ Complexity     4619     4591      -28     
============================================
  Files           412      410       -2     
  Lines         28679    28610      -69     
  Branches       4686     4677       -9     
============================================
- Hits          13262    13221      -41     
+ Misses        13834    13809      -25     
+ Partials       1583     1580       -3     

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

@krisso-rtb krisso-rtb force-pushed the bugfix/populate-method-always-with-customization-argument branch 2 times, most recently from d8860b6 to b7b6041 Compare April 30, 2024 19:11
deserialize-method(s) of nested records may generate another
populate methods that require 'customization' (at least for compilation).

TDD approach - this commit adds unit test which fails and shows
where the issue actually is.
@krisso-rtb
Copy link
Contributor Author

Hi @radai-rosenblatt , @FelixGV , @gaojieliu
Would you be able to take a look at this fix?

because deserialize-method(s) of nested records may generate another
populate methods that require 'customization' (at least for compilation).

This commits contains the fix.
@krisso-rtb krisso-rtb force-pushed the bugfix/populate-method-always-with-customization-argument branch from b7b6041 to ff2c378 Compare May 6, 2024 09:17
@gaojieliu
Copy link
Collaborator

Looks good to me and I am going to merge it and cut a new release.
Thanks for the fix!

@gaojieliu gaojieliu merged commit b0fc4f9 into linkedin:master May 6, 2024
2 checks passed
@gaojieliu
Copy link
Collaborator

@krisso-rtb
Copy link
Contributor Author

Thank you @gaojieliu !

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