Include union type as dictionaries

[chmp]

Update: serialize unions as is, but include an additional {name}_type field that encodes the union type in human readable form.

At the moment field-less enums are serialized as a union of null fields. This is probably not expected (or desired). Probably it would be better to serialize it as a string-dictionary.

test_round_trip!(
    test_name = enums_union,
    tracing_options = TracingOptions::default().allow_null_fields(true),
    field = Field::new(
        "value",
        DataType::Union(
            vec![
                Field::new("A", DataType::Null, true),
                Field::new("B", DataType::Null, true),
            ],
            None,
            UnionMode::Dense
        ),
        false,
    ),
    ty = Item,
    values = [Item::A, Item::B,],
    define = {
        #[derive(Debug, PartialEq, Deserialize, Serialize)]
        enum Item {
            A,
            B,
        }
    },
);

Current idea:

• add fields with strategy "enum_type" that only serialize the type of the enum
• Q: allow both data and type to be stored in different fields? How to deal with inconsistencies?
  • on serialization no inconsistency can happen
  • on deserialization ignore the type if both are present?

[droher]

Not sure whether this should be its own issue or under this one, but I am currently unable to serialize field-less enums - they all panic with a backtrace like this:

thread '<unnamed>' panicked at 'index out of bounds: the len is 2 but the index is 3', /Users/davidroher/.cargo/registry/src/github.com-1ecc6299db9ec823/serde_arrow-0.7.0/src/internal/generic_sinks/union.rs:85:45
stack backtrace:
   0: rust_begin_unwind
             at /rustc/84c898d65adf2f39a5a98507f1fe0ce10a2b8dbc/library/std/src/panicking.rs:579:5
   1: core::panicking::panic_fmt
             at /rustc/84c898d65adf2f39a5a98507f1fe0ce10a2b8dbc/library/core/src/panicking.rs:64:14
   2: core::panicking::panic_bounds_check
             at /rustc/84c898d65adf2f39a5a98507f1fe0ce10a2b8dbc/library/core/src/panicking.rs:159:5
   3: <serde_arrow::internal::generic_sinks::union::UnionArrayBuilder<B> as serde_arrow::internal::sink::EventSink>::accept_variant
   4: <serde_arrow::internal::generic_sinks::struct::StructArrayBuilder<B> as serde_arrow::internal::sink::EventSink>::accept_variant
   5: <serde_arrow::internal::generic_sinks::struct::StructArrayBuilder<B> as serde_arrow::internal::sink::EventSink>::accept_variant
   6: <serde_arrow::internal::sink::EventSerializer<S> as serde::ser::Serializer>::serialize_unit_variant
   7: boxball_rs::event_file::game_state::_::<impl serde::ser::Serialize for boxball_rs::event_file::game_state::GameMetadata>::serialize

As soon as it gets to any struct which has a fieldless enum as one of its fields (when I remove all enum fields from the struct, i no longer get the error).

[chmp]

@droher Thanks for posting. A separate issue would be better (as I have something different in mind for this topic :D). I created one here.

[droher]

I'm not sure if you would want to include this as an intermediate option, but my current workaround for this is to just serialize them as strings (with a serialize_with override) and not worry about the dictionary part. This is especially relevant for the Parquet write use-case, because the parquet crate A) isn't able to handle null union types and B) automatically writes string fields as dictionaries regardless of their arrow representation.

[chmp]

The dictionary part is actually not the tricky part - as it is already implemented :). My issue is rather how to do all of this in a generic way. My feeling is the type as a string could be useful in addition to the data quite some often. So my current thinking is to allow maximum flexibility:

• data only: the current approach
• type only: store variant name as a dictionary encoded string
• data + type: store the enum as separate fields (e.g., {field_name}_type and {field_name}_data)

What is keeping me from implementing it, is mostly, that I'm not sure whether I like this approach :) (plus time)

[chmp]

Started work in #185

[chmp]

See here for the current design: #183 (comment)

[chmp]

Merged #185