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

[Enhancement] Strict Staged Builders Support Primitive Optimized Collections #2464

Open
wants to merge 1 commit into
base: bmarcaurele/new-native-collection-apis
Choose a base branch
from

Conversation

bmarcaur
Copy link
Member

Before this PR

When using useStrictStagedBuilders and nonNullCollections the generated builders did not allow for the optimal (non-boxing) path (#2389), nor did it optimally handle Jackson deserialization (#2431).

After this PR

Strict staged builders now expose additional, optimized setters and also leverage these setters for Jackson deserialization.

==COMMIT_MSG==
==COMMIT_MSG==

Possible downsides?

IMO this class is becoming unwieldy. It has two similar, but somewhat divergent code paths between strict and non-strict builders. You cannot simply hoist the strictness to the top level because it has entry points like addStagedBuilder which you can call even when useStrictStagedBuilders is set to true? Kinda feels like we should have different builder implementations for each of the paths through and we then factor out the bit of shared behavior. IDK it felt bad adding all these boolean strict all over the place.

In addition, I added some complexity to support adding the collection reset. If we are comfortable allowing people to cast into the base builder and break shit.. Then I can remove some of these unwanted boolean stricts.

Type type = enriched.conjureDef().getType();
Optional<LogSafety> safety = safetyEvaluator.getUsageTimeSafety(enriched.conjureDef());

ImmutableList.Builder<MethodSpec> builder = ImmutableList.builder();

if (type.accept(TypeVisitor.IS_LIST)) {
if (isPrimitiveOptimized(type)) {
Copy link
Member Author

@bmarcaur bmarcaur Feb 26, 2025

Choose a reason for hiding this comment

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

Hoisted this up as having it nested in the auxiliary setters was a hold over from a time before isPrimitiveOptimized properly guarded for collection types only. Hence it needing to be nested inside a type.accept(TypeVisitor.IS_LIST). However, this is not strictly a refactor for refactor's sake. I did need to add the primitive setter in more cases than what this method is used for.

@bmarcaur bmarcaur force-pushed the bmarcaurele/strict-staged-optimization branch from 07e5ae9 to 6341c75 Compare February 26, 2025 02:54
@carterkozak
Copy link
Contributor

I think this will need a rebase once #2463 merges given we've replaced some of the integrationInput test infrastructure.

@mpritham Could you give this a final review + approval once the test refactoring PR has merged and this has rebased?

@bmarcaur
Copy link
Member Author

I think this will need a rebase once #2463 merges given we've replaced some of the integrationInput test infrastructure.

@mpritham Could you give this a final review + approval once the test refactoring PR has merged and this has rebased?

Yup, chatted with Pritham about this. I'm not in a rush so would rather not disrupt Kunal.

@kunalrkak
Copy link
Contributor

fysa #2463 has merged in, this should be good to rebase! running the test here (with the recreate flag set) will regenerate PrimitiveStrictExample with your changes

@bmarcaur bmarcaur force-pushed the bmarcaurele/strict-staged-optimization branch from 6341c75 to 2bcf64a Compare March 6, 2025 18:16
@bmarcaur
Copy link
Member Author

bmarcaur commented Mar 6, 2025

fysa #2463 has merged in, this should be good to rebase! running the test here (with the recreate flag set) will regenerate PrimitiveStrictExample with your changes

@kunalrkak @mpritham It rebased eerily cleanly. I still ran gw :conjure-java-core:test -Drecreate=true but it produced no changes so I think this is clean and GTG.

edit: I think I should still address #2464 (comment) before this merges. Its a good suggestion. It will only be a one line change in the generation code. So a review on this need not wait, but you can if you want.

@bmarcaur bmarcaur force-pushed the bmarcaurele/strict-staged-optimization branch from 183925a to 1f6bc08 Compare March 7, 2025 20:58
@palantir palantir deleted a comment from changelog-app bot Mar 7, 2025
@bmarcaur bmarcaur changed the base branch from develop to bmarcaurele/new-native-collection-apis March 7, 2025 21:00
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