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 Mon, 12 Mar 2018 07:18:09 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.
> 
> Qian Zhang wrote:
>     > 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.

Here is the patch (https://reviews.apache.org/r/66025/) for MESOS-8656, and I have rebased
this patch chain on top of that patch.


- Qian


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


On March 12, 2018, 3:15 p.m., Qian Zhang wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59987/
> -----------------------------------------------------------
> 
> (Updated March 12, 2018, 3:15 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/8/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>


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