> 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
>
>
