mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Qian Zhang <zhq527...@gmail.com>
Subject Re: Review Request 59987: Added protobuf map support to stout JSON<->protobuf conversion.
Date Fri, 09 Mar 2018 13:59:16 GMT


> On March 1, 2018, 12:10 p.m., Benjamin Mahler wrote:
> > 3rdparty/stout/include/stout/protobuf.hpp
> > Lines 391-487 (patched)
> > <https://reviews.apache.org/r/59987/diff/5/?file=1965083#file1965083line391>
> >
> >     It looks like we can avoid this logic since protobuf's JSON conversion allows
the protobuf key types (bool, integers, strings) to be converted from JSON strings. This means
we could just recurse for both key and value here:
> >     
> >     ```
> >                 // Some comment explaining what map is equivalent to with
> >                 // a reference to the google documentation.
> >                 google::protobuf::Message* entry =
> >                   reflection->AddMessage(message, field);
> >     
> >                 const google::protobuf::FieldDescriptor* key_field =
> >                   entry->GetDescriptor()->FindFieldByNumber(1);
> >                   
> >                 // Maybe passing just 'key' works due to implicit construction?
> >                 // TODO(...): This copies the key, consider avoiding this.
> >                 Try<Nothing> apply =
> >                   boost::apply_visitor(Parser(entry, key_field), JSON::String(key));
> >     
> >                 if (apply.isError()) {
> >                   return Error(apply.error());
> >                 }
> >                   
> >                 const google::protobuf::FieldDescriptor* value_field =
> >                   entry->GetDescriptor()->FindFieldByNumber(2);
> >                   
> >                 Try<Nothing> apply =
> >                   boost::apply_visitor(Parser(entry, value_field), value);
> >     
> >                 if (apply.isError()) {
> >                   return Error(apply.error());
> >                 }
> >     ```
> >     
> >     Now, to make this simplification work, we need to update our JSON conversion
in a separate patch to allow bools and integers to be parsed from JSON strings to match google's
logic for conversion:
> >     
> >     https://github.com/google/protobuf/blob/v3.5.1/src/google/protobuf/util/internal/datapiece.cc#L203
> >     
> >     Documentation here: https://developers.google.com/protocol-buffers/docs/proto3#json
> 
> Qian Zhang wrote:
>     > // Maybe passing just 'key' works due to implicit construction?
>     
>     We cannot pass `key` or even `JSON::String` to `boost::apply_visitor()` because it
cannot pass compilation:
>     ```
>     ../3rdparty/boost-1.65.0/boost/variant/detail/apply_visitor_unary.hpp:84:22: error:
no member named 'apply_visitor' in 'std::__1::basic_string<char>'
>     ```
>     or
>     ```
>     ../3rdparty/boost-1.65.0/boost/variant/detail/apply_visitor_unary.hpp:84:22: error:
no member named 'apply_visitor' in 'JSON::String'
>     ```
>     That is because `std::string` and `JSON::String` do not have a member `apply_visitor`,
so I think we have to use `JSON::Value` like this:
>     ```
>                 google::protobuf::Message* entry =
>                   reflection->AddMessage(message, field);
>     
>                 const google::protobuf::FieldDescriptor* key_field =
>                   entry->GetDescriptor()->FindFieldByNumber(1);
>     
>                 JSON::Value key(name);
>     
>                 Try<Nothing> apply =
>                   boost::apply_visitor(Parser(entry, key_field), key);
>     
>                 if (apply.isError()) {
>                   return Error(apply.error());
>                 }
>     
>                 const google::protobuf::FieldDescriptor* value_field =
>                   entry->GetDescriptor()->FindFieldByNumber(2);
>     
>                 apply = boost::apply_visitor(Parser(entry, value_field), value);
>                 if (apply.isError()) {
>                   return Error(apply.error());
>                 }
>     ```
>     
>     > we need to update our JSON conversion in a separate patch to allow bools and
integers to be parsed from JSON strings to match google's logic for conversion
>     
>     Did you mean we should improve the method below by adding more cases for converting
`JSON::String` to bools and integers?
>     ```
>       Try<Nothing> operator()(const JSON::String& string) const
>       {
>         switch (field->type()) {
>           case google::protobuf::FieldDescriptor::TYPE_STRING:
>           ...
>     ```
>     If so, then that means moving the code here (see below) into the above method. But
I think those code are specific for map support, however `Try<Nothing> operator()(const
JSON::String& string) const` is a generic method for JSON string conversion, so I would
still like to keep those code as where they are now in this patch (i.e., the code path to
handle map).
>     ```
>                   case google::protobuf::FieldDescriptor::TYPE_INT64:
>                   case google::protobuf::FieldDescriptor::TYPE_SINT64:
>                   case google::protobuf::FieldDescriptor::TYPE_SFIXED64:
>                   {
>                     Try<int64_t> key = numify<int64_t>(name);
>                     ...
>                   }
>                   case google::protobuf::FieldDescriptor::TYPE_UINT64:
>                   case google::protobuf::FieldDescriptor::TYPE_FIXED64:
>                   {
>                     Try<uint64_t> key = numify<uint64_t>(name);
>                     ...
>                   }
>                   ...
>     ```
> 
> Chun-Hung Hsiao wrote:
>     Re: `JSON::Value`: It seems that there's no need to declare a new `key` variable,
and just pass `JSON::Value(name)` into `apply_visitor`.
>     
>     Re: `JSON::String` <-> bools and ints: Yes. Ben's actually suggesting that
we should allow this conversion in general, an an enhancement of stout's JSON support, so
these conversions need not to be specific for map support. For example, if we implement this
enhancement, we could parse JSON outputed from Google's JSON function. You could probably
prepend another patch for this enhancement.

> It seems that there's no need to declare a new key variable, and just pass JSON::Value(name)
into apply_visitor.

I tried it before, but it can not pass the compilation.
```
../../3rdparty/stout/include/stout/protobuf.hpp:408:15: error: no matching function for call
to 'apply_visitor'
              boost::apply_visitor(Parser(entry, key_field), JSON::Value(name));
              ^~~~~~~~~~~~~~~~~~~~
../3rdparty/boost-1.65.0/boost/variant/detail/apply_visitor_unary.hpp:82:1: note: candidate
function [with Visitor = protobuf::internal::Parser, Visitable = JSON::Value] not
      viable: expects an l-value for 2nd argument
apply_visitor(const Visitor& visitor, Visitable& visitable)
```

> Re: JSON::String <-> bools and ints: Yes. Ben's actually suggesting that we should
allow this conversion in general, an an enhancement of stout's JSON support, so these conversions
need not to be specific for map support. For example, if we implement this enhancement, we
could parse JSON outputed from Google's JSON function. You could probably prepend another
patch for this enhancement.

OK, so we need this enhancement to make our stout JSON -> protobuf conversion can handle
more valid JSONs. The followings are valid in JSON which can be successfully parsed by Google's
protobuf utility function `JsonStringToMessage()` but can NOT be parsed our stout (i.e., call
`JSON::parse()` to convert the JSON to a stout JSON object and then call `protobuf::parse()`
to parse the JSON object to a protobuf message).
```
"int32": "-2147483647"
"int64": "-9223372036854775807"
"bool": "true"
```
The second step (`protobuf::parse()`) will fail with an error like this:
```
Not expecting a JSON string for field 'int32'
```
This error comes from `Try<Nothing> operator()(const JSON::String& string)`, so
we need to enhance it to convert `JSON::String` to bools and integers. I created a separate
ticket (https://issues.apache.org/jira/browse/MESOS-8656) for it, let's handle it there.


> On March 1, 2018, 12:10 p.m., Benjamin Mahler wrote:
> > 3rdparty/stout/include/stout/protobuf.hpp
> > Lines 400-401 (patched)
> > <https://reviews.apache.org/r/59987/diff/5/?file=1965083#file1965083line400>
> >
> >     Can you move this out of the loop?
> 
> Qian Zhang wrote:
>     It needs the variable `entry` which is defined inside of the `foreachpair` loop,
so I think we still need to keep `reflection` inside of the loop.
> 
> Chun-Hung Hsiao wrote:
>     The `entry` itself doesn't depend on `name` and `value`, so let's hoist them outside
the loop.

We get `entry` by calling `AddMessage()`:
```
  google::protobuf::Message* entry =
      reflection->AddMessage(message, field);
```
I think we need to call `AddMessage` inside of the `foreachpair` loop rather than outside.


> On March 1, 2018, 12:10 p.m., Benjamin Mahler wrote:
> > 3rdparty/stout/include/stout/protobuf.hpp
> > Lines 1039-1043 (patched)
> > <https://reviews.apache.org/r/59987/diff/5/?file=1965083#file1965083line1040>
> >
> >     Is there a way to abstract out this logic into a lambda here and re-use it?
E.g.
> >     
> >     ```
> >     template <typename T>
> >     JSON::Value value_for_field(
> >         Message* entry,
> >         google::protobuf::Reflection* reflection,
> >         google::protobuf::FieldDescriptor* field)
> >     {
> >       switch (type) {
> >         case ...:
> >         case TYPE_INT64:
> >           return ...;
> >       }
> >     };
> >     ```
> >     
> >     Then you just do the following here and elsewhere:
> >     
> >     ```
> >     map.values[key] = value_for_field(entry, reflection, field);
> >     ```
> >     
> >     I've glossed over some details, but I hope you get the idea and can figure out
how to make it work across the different cases here.
> 
> Qian Zhang wrote:
>     Do you mean moving the big `"switch ... case"` introduced in my patch for map into
a method `value_for_field()`, and use it to replace the other two big `"switch ... case"`
for array and object (see below) too?
>     https://github.com/apache/mesos/blob/1.5.0/3rdparty/stout/include/stout/protobuf.hpp#L864:L923
>     https://github.com/apache/mesos/blob/1.5.0/3rdparty/stout/include/stout/protobuf.hpp#L927:L986
>     
>     I like the idea and I think it is doable for map and object, but for array, it is
a bit different since we will call different protobuf APIs, e.g., `reflection->GetInt32`
(map and object) v.s. `reflection->GetRepeatedInt32()` (array). So I think it may not be
able to cover array :-(
> 
> Chun-Hung Hsiao wrote:
>     Let's do it for maps and objects. It's unfortunate but seems we have to keep the
array case as is.

OK


- Qian


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59987/#review198429
-----------------------------------------------------------


On March 6, 2018, 5:29 p.m., Qian Zhang wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59987/
> -----------------------------------------------------------
> 
> (Updated March 6, 2018, 5:29 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Benjamin Mahler, Chun-Hung Hsiao, and Zhitao
Li.
> 
> 
> Bugs: MESOS-7656
>     https://issues.apache.org/jira/browse/MESOS-7656
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Map is a feature of proto2 syntax, but it can only be compiled
> with 3.0.0+ protobuf compiler, see the following discussion for
> details:
> 
> https://groups.google.com/forum/#!topic/protobuf/p4WxcplrlA4
> 
> We have already upgraded the compiler from 2.6.1 to 3.3.0 in
> MESOS-7228. This patch adds map support in the json <-> protobuf
> conversion in stout.
> 
> 
> Diffs
> -----
> 
>   3rdparty/stout/include/stout/protobuf.hpp 4a1605e5130dbf7e8286dbb43d0d04ab4394e79a

> 
> 
> Diff: https://reviews.apache.org/r/59987/diff/6/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message