Skip to content

Conversation

shangm2
Copy link

@shangm2 shangm2 commented May 1, 2025

  1. Add support for Java optional to IDL generator
  2. Optional field will be marked as optional even without requireness annotation

@shangm2 shangm2 requested a review from a team as a code owner May 1, 2025 21:55
@shangm2 shangm2 requested a review from jaystarshot May 1, 2025 21:55
Copy link

@NikhilCollooru NikhilCollooru left a comment

Choose a reason for hiding this comment

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

Ideally it would be great if we have test that does the entire loop
Java class --> thrift idl --> Cpp/Java class.
that way we can catch any bugs like spelling mistake etc,.

@shangm2 shangm2 changed the title Add support for Java optional to IDL generator [Drift] Add support for Java optional to IDL generator May 1, 2025
@shangm2
Copy link
Author

shangm2 commented May 1, 2025

Ideally it would be great if we have test that does the entire loop Java class --> thrift idl --> Cpp/Java class. that way we can catch any bugs like spelling mistake etc,.

Chatted offline. the current test is good enough.

public Optional<String> optionalString;

@ThriftField(value = 2, requiredness = OPTIONAL)
public Optional<Long> optionalLong;
Copy link
Member

Choose a reason for hiding this comment

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

Could you please check if primitive flavours of Optional work (e.g.: OptionalInt, OptionalLong, OptionalDouble, etc.)

Copy link
Author

Choose a reason for hiding this comment

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

@arhimondr primitive flavours of Optional works if their corresponding codecs are loaded, pls see this pr #112

@NikhilCollooru NikhilCollooru merged commit a8e8622 into prestodb:master May 5, 2025
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