You are viewing a plain text version of this content. The canonical link for it is here.

Posted to dev@avro.apache.org by GitBox <gi...@apache.org> on 2020/10/08 14:35:12 UTC

[GitHub] [avro] epalace opened a new pull request #965: AVRO-2937: Add some missing options in SpecificCompilerTool

epalace opened a new pull request #965:
URL: https://github.com/apache/avro/pull/965


   ### Jira
   
   - https://issues.apache.org/jira/browse/AVRO-2937
   
   ### Description
   
   Currently these are the flags available in java SpecificCompilerTool: 
   
    ```java -jar avro-tools-1.10.0.jar compile
   
   [-encoding <outputencoding>] [-string] [-bigDecimal] [-fieldVisibility <visibilityType>] [-templateDir <templateDir>]
   ```
   This PR adds the following options, which already SpecificCompiler accepts, but they are missing in the tool:
   
   - -noSetters
   - -optionalGetters [all_fields | only_nullable_fields]
   - -addExtraOptionalGetters
   
   ### Tests
   
   - 4 new tests added in TestSpecificCompilerTool.java


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [avro] epalace commented on pull request #965: AVRO-2937: Add some missing options in SpecificCompilerTool

Posted by GitBox <gi...@apache.org>.

epalace commented on pull request #965:
URL: https://github.com/apache/avro/pull/965#issuecomment-708512156

Thanks for the comments @RyanSkraba,

>Hello! I noticed that the flag names are different than the ones used in the schema compiler maven goal. Do you think it's worthwhile trying to "unify" them?
- I unified the two flags `gettersReturnOptional` and `optionalGettersForNullableFieldsOnly` into an optional enum in order to avoid the unfortunate case of setting the former to false while the second to true. (By the way I'm new to this, what's the point of allowing to generate getters for _non_ nullable fields? is due to backwards compatibility issues?)
- I also renamed `createOptionalGetters` to `addExtraOptionalGetters` in order to make more explicit that this is adding some extra getters (not modifying the original ones), and also to avoid confusion with the quite similar flag `gettersReturnOptional``
.

That said, you're right that unifying them would be a good idea. Do you think this new nomenclature makes sense or we should stick to the original one?

>I really like the separation into CompilerOptions for readability! What do you think about moving it into the SpecificCompiler class directly, so it could be reused as "the source of truth" for configuration options and the Java code generation?

I makes perfect sense to do that and I would be happy to do that refactor, but I have a few concerns:
- This would be a breaking change in SpecificCompiler API, would that be an issue? Should we maintain compatibility with the old getters and setters and deprecate them?
- This refactor would be an unrelated change to the subject of this PR, leading to potentially many irrelevant changes. For example, the template `record.vm` would need to change, to access inner fields within the CompierOptions object. Would it make sense to create a separate PR?

Regards.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [avro] RyanSkraba commented on pull request #965: AVRO-2937: Add some missing options in SpecificCompilerTool

Posted by GitBox <gi...@apache.org>.

RyanSkraba commented on pull request #965:
URL: https://github.com/apache/avro/pull/965#issuecomment-708438641


   Hello!  I noticed that the flag names are different than the ones used in the schema compiler maven goal.  Do you think it's worthwhile trying to "unify" them?
   
   I _really_ like the separation into `CompilerOptions` for readability!  What do you think about moving it into the `SpecificCompiler` class directly, so it could be reused as "the source of truth" for configuration options and the Java code generation?
   
   I'm taking a look as it is, but I wanted to open the discussion -- thanks for proposing and implementing this feature!


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [avro] epalace commented on pull request #965: AVRO-2937: Add some missing options in SpecificCompilerTool

Posted by GitBox <gi...@apache.org>.

epalace commented on pull request #965:
URL: https://github.com/apache/avro/pull/965#issuecomment-709511428


   > A non-nullable field can still be null if it's unset, so None for that field would indicate that the record isn't (yet) valid. The semantics around this is pretty inelegant -- I don't think it's ever possible to distinguish between an unset field and field set to null.
   
   Oh, I see, that's because of the mutable nature of SpecificRecord and when they are built they might still not be valid. [Avrohugger](https://github.com/julianpeeters/avrohugger) addresses this initializing always the non-nullable fields with a default value in its non-args constructor (do not know what are the implications in performance in case that inner objects are not reused in serialization).
   
   > You are correct, a separate CompilerOpt object and class would be a nice refactor and cleanup, but not necessary to add this feature. If you feel like taking it on, please feel free!
   
   I could tackle that in a different PR.
   
   > It's a bit irritating that the tests add 2K lines of code. If anyone can think of a way to improve this in terms of readability and maintainability, I'd love to hear it.
   
   Similarly to how the maven plugin works, what about compiling those tests avscs and make the generated output be part of the test sources? Tests would be able to access them in compile time and check the existence and signature of some methods of those generated classes. Something similar to what that they do in sbt-avro  [sbt-avro](https://github.com/sbt/sbt-avro/pull/41/files) but without requiring reflection. 


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [avro] epalace edited a comment on pull request #965: AVRO-2937: Add some missing options in SpecificCompilerTool

Posted by GitBox <gi...@apache.org>.

epalace edited a comment on pull request #965:
URL: https://github.com/apache/avro/pull/965#issuecomment-708512156

Thanks for the comments @RyanSkraba,

>Hello! I noticed that the flag names are different than the ones used in the schema compiler maven goal. Do you think it's worthwhile trying to "unify" them?
- I unified the two flags `gettersReturnOptional` and `optionalGettersForNullableFieldsOnly` into an optional enum in order to avoid the unfortunate case of setting the former to false while the latter to true. (By the way I'm new to this, what's the point of allowing to generate getters for _non_ nullable fields? is due to backwards compatibility issues?)
- I also renamed `createOptionalGetters` to `addExtraOptionalGetters` in order to make more explicit that this is adding some extra getters (not modifying the original ones), and also to avoid confusion with the quite similar flag `gettersReturnOptional`
.

That said, you're right that unifying them would be a good idea. Do you think this new nomenclature makes sense or we should stick to the original one?

I makes perfect sense to do that and I would be happy to do that refactor, but I have a few concerns:
- This would be a breaking change in SpecificCompiler API, would that be an issue? Should we maintain compatibility with the old getters and setters and deprecate them?
- This refactor would be unrelated to the subject of this PR, leading to potentially many irrelevant changes. For example, the template `record.vm` would need to change, to access inner fields within the CompierOptions object. Would it make sense to create a separate PR?

Regards.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [avro] RyanSkraba edited a comment on pull request #965: AVRO-2937: Add some missing options in SpecificCompilerTool

Posted by GitBox <gi...@apache.org>.

RyanSkraba edited a comment on pull request #965:
URL: https://github.com/apache/avro/pull/965#issuecomment-717906373


   I missed this!  Thanks for the reminder!
   
   Cherry-picked to [branch-1.10](https://github.com/apache/avro/commit/9239dcc040b24ab768c7de900795cbba9005a312).


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [avro] epalace edited a comment on pull request #965: AVRO-2937: Add some missing options in SpecificCompilerTool

Posted by GitBox <gi...@apache.org>.

epalace edited a comment on pull request #965:
URL: https://github.com/apache/avro/pull/965#issuecomment-709511428


   > A non-nullable field can still be null if it's unset, so None for that field would indicate that the record isn't (yet) valid. The semantics around this is pretty inelegant -- I don't think it's ever possible to distinguish between an unset field and field set to null.
   
   Oh, I see, that's because of the mutable nature of SpecificRecord and when they are built they might still not be valid. [Avrohugger](https://github.com/julianpeeters/avrohugger) addresses this initializing always the non-nullable fields with a default value in its non-args constructor (do not know what are the implications in performance in case that inner objects are not reused in serialization).
   
   > You are correct, a separate CompilerOpt object and class would be a nice refactor and cleanup, but not necessary to add this feature. If you feel like taking it on, please feel free!
   
   I could tackle that in a different PR.
   
   > It's a bit irritating that the tests add 2K lines of code. If anyone can think of a way to improve this in terms of readability and maintainability, I'd love to hear it.
   
   Similarly to how the maven plugin works, what about compiling those tests avscs and make the generated output be part of the test sources? Tests would be able to access them in compile time and check the existence and signature of some methods of those generated classes. Something similar to what that they do in sbt-avro  [here](https://github.com/sbt/sbt-avro/pull/41/files) but without requiring reflection. 


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [avro] RyanSkraba commented on pull request #965: AVRO-2937: Add some missing options in SpecificCompilerTool

Posted by GitBox <gi...@apache.org>.

RyanSkraba commented on pull request #965:
URL: https://github.com/apache/avro/pull/965#issuecomment-717906373


   I missed this!  Thanks for the reminder!


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [avro] epalace edited a comment on pull request #965: AVRO-2937: Add some missing options in SpecificCompilerTool

Posted by GitBox <gi...@apache.org>.

epalace edited a comment on pull request #965:
URL: https://github.com/apache/avro/pull/965#issuecomment-708512156

Thanks for the comments @RyanSkraba,

>Hello! I noticed that the flag names are different than the ones used in the schema compiler maven goal. Do you think it's worthwhile trying to "unify" them?
- I unified the two flags `gettersReturnOptional` and `optionalGettersForNullableFieldsOnly` into an optional enum in order to avoid the unfortunate case of setting the former to false while the latter to true. (By the way I'm new to this, what's the point of allowing to generate getters for _non_ nullable fields? is due to backwards compatibility issues?)
- I also renamed `createOptionalGetters` to `addExtraOptionalGetters` in order to make more explicit that this is adding some extra getters (not modifying the original ones), and also to avoid confusion with the quite similar flag `gettersReturnOptional`
.

That said, you're right that unifying them would be a good idea. Do you think this new nomenclature makes sense or we should stick to the original one?

Regards.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [avro] RyanSkraba merged pull request #965: AVRO-2937: Add some missing options in SpecificCompilerTool

Posted by GitBox <gi...@apache.org>.

RyanSkraba merged pull request #965:
URL: https://github.com/apache/avro/pull/965


   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [avro] RyanSkraba commented on pull request #965: AVRO-2937: Add some missing options in SpecificCompilerTool

Posted by GitBox <gi...@apache.org>.

RyanSkraba commented on pull request #965:
URL: https://github.com/apache/avro/pull/965#issuecomment-709389513


   > what's the point of allowing to generate getters for non nullable fields? is due to backwards compatibility issues
   
   A non-nullable field can still be null if it's unset, so `None` for that field would indicate that the record isn't (yet) valid.  The semantics around this is pretty inelegant -- I don't think it's ever possible to distinguish between an unset field and field set to null.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [avro] epalace commented on pull request #965: AVRO-2937: Add some missing options in SpecificCompilerTool

Posted by GitBox <gi...@apache.org>.

epalace commented on pull request #965:
URL: https://github.com/apache/avro/pull/965#issuecomment-716039621


   @RyanSkraba will this be finally merged?


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org