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

feat: plain map and slice #464

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Conversation

0xjac
Copy link

@0xjac 0xjac commented Oct 7, 2024

  • Add options to generate:
    • A plain map for a nullable union resulting in a map.
    • A plain slice for a nullable union resulting in a slice.
  • Handle encoding and decoding of nullable maps, similar to nullable slices.

Go map and slice types are already nullable and do not need an extra pointer to represent a nullable union.

This new behavior is hidden behind the PainMap, PlainSlice options and corresponding -plain-map, -plain-slice flags for the CLI to preserve compatibility.

@0xjac
Copy link
Author

0xjac commented Oct 7, 2024

@nrwiersma As promised, quick lunch break fix. 😉 This also handles map in a similar way (with a separate option for it of course).

@@ -25,6 +25,8 @@ type config struct {
FullName bool
Encoders bool
FullSchema bool
PlainMap bool
Copy link
Member

Choose a reason for hiding this comment

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

The decoder will not work for map like it does for slice currently.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for your feedback, however I'm not sure what you are referring to here. What would need to change in the decoder for this to work?

Copy link
Author

Choose a reason for hiding this comment

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

@nrwiersma I'm not familiar with the whole codec logic yet but I had a look and tried to add support for nullable maps for encoding and decoding. Have a look at my latest commits and let me know what you think.

@0xjac 0xjac force-pushed the feat/461-plain-map-slice branch from ef47dfc to 4d8a8e2 Compare October 8, 2024 14:50
0xjac added 6 commits October 8, 2024 16:51
Go maps and slices can be nil. An avro nullable union can thus be
represented directly by a map or a slice without using an explicit
pointer (`*`).

This feature is hidden being the `PlainMap` and `PlainSlice` options to
preserve the existing behavior.

Closes hamba#461
Adds `-plain-map` and `-plain-slice` flags to expose the corresponding
options.
Similar to how nullable slices are handles, this adds support for using
plain `map[string]any` for nullable unions of maps instead of using an
explicit pointer to a map.
@0xjac 0xjac force-pushed the feat/461-plain-map-slice branch from 4d8a8e2 to a733118 Compare October 8, 2024 14:51
Copy link
Member

@nrwiersma nrwiersma left a comment

Choose a reason for hiding this comment

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

Just one unhandled edge case that I can see. Perhaps it would be better to split this PR into codec and avrogen PRs. It would likely make them simpler.

Comment on lines +20 to +23
if typ.(reflect2.MapType).Elem().Kind() != reflect.Interface {
if !schema.Nullable() {
break
}
Copy link
Member

Choose a reason for hiding this comment

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

There is an edge case here being:
["null", {"type": "map", "values": ["null", "string"]}]
or something like this. In the traditional struct layout this could be *map[string]any which is specifically excluded here. In the new layout being proposed, this would be map[string]any, and the en/decoder would not be able to tell the cases apart.

To deal with this case, IMO the en/decoder should always error towards the map union (existing functionality) and failing that, check the new non-pointer version. This would also need to be documented.

Copy link
Author

Choose a reason for hiding this comment

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

@nrwiersma sorry my use case has changed and I no longer need this feature. Sadly I don't have time to fix it at the moment. Feel free to fix it, hand it over to another volunteer or close it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants