Issue with oneof fields

[balusu-murali]

Hi, I am using gpb create the message definitions and then loading them using enif_protobuf for the codec. The issue I face is the following:

For definition like this:

syntax = "proto3";

message One {
    int32 id = 1;
}

message Two {
    string s = 1;
}

message Three {
    oneof payload {
        One o = 1;
        Two t = 2;
    }
}

Final erlang records for Three would look something like this:

#Three{
    payload = {o, #One{id = 1}}
}

In my case, it is guaranteed that each of these oneof fields will be different record types: in these cases, I would like to get rid of this additional tuple and make it look something like this:

#Three{
    payload = #One{id = 1}
}

I figured out the changes to do in enif_protobuf: https://github.com/HalloAppInc/enif_protobuf/pull/2/commits/01b8c607b248a85ca1a25b890d8323900d3171d7 However, the issue is when encoding we search for the field number using the field_name. [specifically, line 622-626 bsearch function call]. I want to avoid that and do the search using the types of the fields. Is there a way for me to do that? can you please let me know. any pointers would really be appreciated?

If you'd like this change, I can add it to be an option for enif_protobuf and send out a PR for this.

[jinganix]

1. [ep_node.h: line 16] Add type_name field

   struct ep_field_s {
   ERL_NIF_TERM        name;
   occurrence_type_e   o_type;
   field_type_e        type;
   //  ERL_NIF_TERM type_name;
   ep_node_t          *sub_node;
   ERL_NIF_TERM        sub_name;
   ERL_NIF_TERM        defaut_value;
   uint32_t            id;
   uint32_t            fnum;
   uint32_t            rnum;
   uint32_t            proto_v;
   uint32_t            is_oneof;
   uint32_t            packed;
   };

2. [ep_node.c: line 195] Assign field type name

3. [ep_node.c: line 737] Add compare type name function

   int
   get_field_compare_type_name(const void *a, const void *b)
   {
   return (int) (*((ERL_NIF_TERM *) a) - ((ep_field_t *) b)->type_name);
   }

4. Call bsearch function

Hope that helps. However, we cannot find the right field, if we have duplicated types.

[balusu-murali]

Thank you @jg513 for getting back.

2. [ep_node.c: line 195] Assign field type name

Looking at the code there.. it feels like sub_name should basically have the name set in case of oneOf fields.. correct?

• I made the following change: https://github.com/HalloAppInc/enif_protobuf/pull/3/files
• From some tests, it looks like its working fine to me.
• Could you please take a look and let me know.

However, we cannot find the right field, if we have duplicated types.

Yeah, I am okay with this restriction in our usecase.

[balusu-murali]

I think that would work actually.. because for bsearch the input needs to be sorted based on the these names.

• It adds another restriction that the fields in the oneof definition must be alphabetically sorted! (index can be in any order).

let me know what you think?

[jinganix]

1. sub_name can store field type, because oneOf type does not use it. But the field name of sub_name is confusing.
2. You could loop for oneOf fields and compare, without using bsearch. Then field definition can be in any order.

[balusu-murali]

1. sub_name can store field type, because oneOf type does not use it. But the field name of sub_name is confusing.

Okay.

2. You could loop for oneOf fields and compare, without using bsearch. Then field definition can be in any order.

I dont want to since it might hurt performance. can I pre-sort them by the type name somehow?

[jinganix]

I guess if oneOf type contains less than 5 or 6 fields, a loop maybe faster.

If you want to sort the fields, following may help:

struct ep_node_s {
    node_type_e     n_type;
    ERL_NIF_TERM    name;
    uint32_t        id;
    uint32_t        proto_v;
    uint32_t        size;
    uint32_t        v_size;
    void           *fields;
    void           *v_fields;
};

node->fields is sorted by name node->v_fields is sorted by enum value, or by field index number

You may add another sorted fields

[balusu-murali]

I see. will try that and let you know.

[balusu-murali]

node->fields is sorted by name node->v_fields is sorted by enum value, or by field index number

You may add another sorted fields

I am trying to do it.. but I'm not able to sort these fields.. throws me an error and its been very hard to debug.. any suggestions on how to add a new pointer to point to a list of fields sorted based on their type?

this is what i have so far: https://github.com/HalloAppInc/enif_protobuf/pull/4

[jinganix]

I viewed your changes, and maybe found a possible reason. In ep_node.c -> make_node, it not alloc l_fields when n_type is node_enum. But in parse_enum_fields function, the l_fields is used.

If that's not the cause, you may print out the fields and l_fields memory as hex, in ep_encoders.c -> get_oneof_field. And compare the content.

And I think, copy fields to l_fields is much easier, than calculating l_fields everywhere

    qsort(node->fields, node->size, sizeof(ep_enum_field_t), sort_compare_enum_field);
    qsort(node->l_fields, node->size, sizeof(ep_enum_field_t), sort_compare_sub_name_l_field);

    qsort(node->fields, node->size, sizeof(ep_enum_field_t), sort_compare_enum_field);
    // memcopy node->fields to node->l_fields, then sort node->l_fields
    qsort(node->l_fields, node->size, sizeof(ep_enum_field_t), sort_compare_sub_name_l_field);

I am on our national day holiday, may not response in time.

[balusu-murali]

thank you for getting back inspite of your schedule. really appreciate it.

• I went through the code and realized that this variable node->fields is sorted differently for different field type.
• Meaning it is sorted based on numbers for enums, it is sorted based on variable names for oneof fields, it is sorted based on message numbers for other fields.
• So, I went ahead and updated it to sort the field based on types and now we can compare them based on the type.
• The only restriction we'll have is: we cant have two fields of the same type in a oneof field: which is quite reasonable and nice for our usecase.

Here's my fix: https://github.com/HalloAppInc/enif_protobuf/pull/5/commits/3a95f9b38243f2c422341cec21cfe69f42b72c2b let me know what you think?

[jinganix]

Yes, I think that's the most simple way for your usecase.

[balusu-murali]

thank you for your help.