zed icon indicating copy to clipboard operation
zed copied to clipboard

finish union support in zson/zng marshaler

Open mccanne opened this issue 3 years ago • 4 comments

The recent reddit thread exposed a few problems in the marshaling implementation for unions. In particular, unmarshaling a ZSON array of union types can only work in Go for interface values so the unmarshaler needs to create an array of empty interface values of the proper type in which to unmarshal (or the logic might want to be turned around where the concrete values are created from below then fit into an array of interface values).

PR #4007 was a first attempt at this. While we got the marshal direction much improved for unions, we encountered some complexity in the unmarshal path (as described above). It's probably not too difficult to get this right but I didn't find the solution and a day's work and need to get back to higher priority items.

So, we're closing #4007 and we'll leave the fix-array-marshal branch in the tree to get picked up later. That branch has a failing test that illustrates the problem.

Moving this to Back Burner for now.

mccanne avatar Jul 19 '22 23:07 mccanne

We're merging branch fix-array-marshal in an upcoming PR as it is needed by the upcoming ZST changes and partially addresses this issue.

mccanne avatar Jul 24 '22 15:07 mccanne

Added a TestSimpleUnionUnmarshal in branch fix-array-marshal which is skipped and should be addressed in this issue.

mccanne avatar Jul 24 '22 15:07 mccanne

To complete this issue we should add a recursive test (where implied union is an array of unions of unions) and a "combinatoric" test, where there are multiple interface values with combos otherwise creating combinatoric problems but I think Zed solve this. becomes the variations become unions. That said, we can get combinatoric behavior when all the type mixtures don't appear in one array so we should have a way to declare the complete union as part of the Go package, like Bind but upfront declarations of what bindings go in what unions.

mccanne avatar Jul 24 '22 15:07 mccanne

@mccanne confirmed there's some overlap between this issue and #2575.

philrz avatar Aug 17 '22 19:08 philrz