Skip to content

AVRO-2471: Generate specific conversions from LogicalType#938

Merged
iemejia merged 2 commits intoapache:masterfrom
RyanSkraba:AVRO-2471-logical-type-collision
Mar 4, 2021
Merged

AVRO-2471: Generate specific conversions from LogicalType#938
iemejia merged 2 commits intoapache:masterfrom
RyanSkraba:AVRO-2471-logical-type-collision

Conversation

@RyanSkraba
Copy link
Contributor

Instead of collecting the types of primitive datum that appear in a generated class, use the model to find the appropriate Conversion<?> instances that should be used for logical types.

Jira

Tests

  • My PR adds the following unit tests OR does not need testing for this extremely good reason:

Commits

  • My commits all reference Jira issues in their subject lines. In addition, my commits follow the guidelines from "How to write a good git commit message":
    1. Subject is separated from body by a blank line
    2. Subject is limited to 50 characters (not including Jira issue reference)
    3. Subject does not end with a period
    4. Subject uses the imperative mood ("add", not "adding")
    5. Body wraps at 72 characters
    6. Body explains "what" and "why", not "how"

Documentation

  • In case of new functionality, my PR adds documentation that describes how to use it.
    • All the public functions and the classes in the PR contain Javadoc that explain what it does

@probot-autolabeler probot-autolabeler bot added the Java Pull Requests for Java binding label Jul 31, 2020
@@ -287,15 +287,52 @@ public void addCustomConversion(Class<?> conversionClass) {
}

public Collection<String> getUsedConversionClasses(Schema schema) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's some dead code as a consequence of this change: getClassNamesOfPrimitiveFields -- I'm taking a look.

I'm also thinking that if the user explicitly called addCustomConversion, it should be considered "used" and included no matter what, but that currently isn't the case.

@RyanSkraba RyanSkraba requested a review from nandorKollar August 3, 2020 13:12
@RyanSkraba RyanSkraba force-pushed the AVRO-2471-logical-type-collision branch from 3ee0d02 to 35491a6 Compare August 11, 2020 07:52
@RyanSkraba RyanSkraba requested a review from iemejia March 3, 2021 08:50
@RyanSkraba RyanSkraba force-pushed the AVRO-2471-logical-type-collision branch from 35491a6 to b32ef59 Compare March 3, 2021 08:52
Copy link
Member

@iemejia iemejia left a comment

Choose a reason for hiding this comment

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

LGTM Thanks for fixing this Ryan using the registered logical types seems like the most reasonable approach.

@iemejia iemejia merged commit bfe1d03 into apache:master Mar 4, 2021
@RyanSkraba RyanSkraba deleted the AVRO-2471-logical-type-collision branch March 4, 2021 13:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Java Pull Requests for Java binding

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants