Parse table annotations param correctly #5050

ghoshkaj · 2018-04-23T18:30:49Z

Issue

The annotations parameter for the table plugin does not parse as documented in #4990.

This PR fixes the following:

Throws an error if annotations=distance or annotations=distance,duration is requested for MLD in ./osrm-routed.
Only returns distance table if annotations=distance is requested (as opposed to returning both distances table and durations table in ./osrm-routed.
Fixes both of the above for node bindings as well

Tasklist

Requirements / Relations

#4990

oxidase · 2018-04-23T19:39:10Z

include/server/api/table_parameter_grammar.hpp

@@ -66,8 +66,12 @@ struct TableParametersGrammar : public BaseParametersGrammar<Iterator, Signature
        annotations.add("duration", AnnotationsType::Duration)("distance",
                                                               AnnotationsType::Distance);

+        const auto reset_annotations = [](engine::api::TableParameters &table_parameters) {


@ghoshkaj minor thing: you can avoid using explicit lambdas by using a rule for an annotations list as

annotations.add ("duration", AnnotationsType::Duration) ("distance", AnnotationsType::Distance) ; annotations_list = annotations[qi::_val = qi::_val | qi::_1] % ',' ; base_rule = BaseGrammar::base_rule(qi::_r1) | (qi::lit("annotations=") > annotations_list [ph::bind(&engine::api::TableParameters::annotations, qi::_r1) = qi::_1]) ;

and below in the members section

qi::rule<Iterator, engine::api::TableParameters::AnnotationsType()> annotations_list;

It is also possible to use qi::_val |= qi::_1 instead of qi::_val = qi::_val | qi::_1 in the semantic action for annotations_list, but it requires a fix at

osrm-backend/include/engine/api/table_parameters.hpp

Line 138 in ce5cfc4

inline TableParameters::AnnotationsType operator|=(TableParameters::AnnotationsType lhs,

as

inline TableParameters::AnnotationsType& operator|=(TableParameters::AnnotationsType& lhs,

chaupow

looks good! 👍 only some doc phrasing english questions in here

chaupow · 2018-04-23T19:22:42Z

docs/http.md

@@ -236,7 +236,7 @@ In addition to the [general options](#general-options) the following options are
 |------------|--------------------------------------------------|---------------------------------------------|
 |sources     |`{index};{index}[;{index} ...]` or `all` (default)|Use location with given index as source.     |
 |destinations|`{index};{index}[;{index} ...]` or `all` (default)|Use location with given index as destination.|
-|annotations |`duration` (default), `distance`, or `duration,distance`|Return additional table with distances to the response. Whether requested or not, the duration table is always returned.|
+|annotations |`duration` (default), `distance`, or `duration,distance`|Return additional table with distances to the response. Whether requested or not, the duration table is always returned. Note that `distance` is only currently implemented for CH. Requesting it with MLD algorithm flag will return a `NotImplemented` error.|


with MLD algorithm flag?

is that English?
with the MLD algorithm flag?
with the MLD algorithm?
with MLD?

chaupow · 2018-04-23T19:23:17Z

docs/http.md

@@ -279,7 +279,7 @@ curl 'http://router.project-osrm.org/table/v1/driving/13.388860,52.517037;13.397
 - `durations` array of arrays that stores the matrix in row-major order. `durations[i][j]` gives the travel time from
  the i-th waypoint to the j-th waypoint. Values are given in seconds. Can be `null` if no route between `i` and `j` can be found.
 - `distances` array of arrays that stores the matrix in row-major order. `distances[i][j]` gives the travel distance from
-  the i-th waypoint to the j-th waypoint. Values are given in meters. Can be `null` if no route between `i` and `j` can be found.
+  the i-th waypoint to the j-th waypoint. Values are given in meters. Can be `null` if no route between `i` and `j` can be found. Note that `distance` is only currently implemented for CH. Requesting it with MLD algorithm flag will return a `NotImplemented` error.


chaupow · 2018-04-23T19:24:24Z

docs/nodejs/api.md

@@ -126,7 +126,7 @@ tables. Optionally returns distance table.
        location with given index as source. Default is to use all.
    -   `options.destinations` **[Array](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array)?** An array of `index` elements (`0 <= integer <
        #coordinates`) to use location with given index as destination. Default is to use all.
-    -   `options.annotations` **[Array](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array)?** An array of the table types to return. Values can be `duration` or `distance` or both. Default is to return only duration table.
+    -   `options.annotations` **[Array](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array)?** An array of the table types to return. Values can be `duration` or `distance` or both. Default is to return only duration table. Note: Currently, requesting a `distance` annotations is only supported for CH. If `annotations=distance` or `annotations=duration,distance` is requested, a `NotImplemented` error will be returned.


If `annotations=distance` or `annotations=duration,distance` is requested, a `NotImplemented` error will be returned.

to

If `annotations=distance` or `annotations=duration,distance` is requested when running a MLD router, a `NotImplemented` error will be returned.

chaupow · 2018-04-23T19:24:45Z

docs/nodejs/api.md

@@ -152,6 +152,8 @@ osrm.table(options, function(err, response) {
 Returns **[Object](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object)** containing `durations`, `sources`, and `destinations`.
 **`durations`**: array of arrays that stores the matrix in row-major order. `durations[i][j]` gives the travel time from the i-th waypoint to the j-th waypoint.
                 Values are given in seconds.
+**`distances`**: array of arrays that stores the matrix in row-major order. `distances[i][j]` gives the travel time from the i-th waypoint to the j-th waypoint.
+                 Values are given in meters. Note: Currently, returning a `distances` result is only supported for CH. If `annotations=distance` or `annotations=duration,distance` is requested, a `NotImplemented` error will be returned.


change grammer parsing according to review

…n table plugin format

chaupow

lgtm

fix incorrect parameter parsing for node osrm and add tests

4dbe65b

ghoshkaj force-pushed the fix/parse-table-annotations-param-correctly branch 4 times, most recently from 51a634d to 32c42f8 Compare April 23, 2018 19:05

ghoshkaj changed the title ~~Fix/parse table annotations param correctly~~ Parse table annotations param correctly Apr 23, 2018

ghoshkaj requested a review from chaupow April 23, 2018 19:06

oxidase reviewed Apr 23, 2018

View reviewed changes

chaupow requested changes Apr 23, 2018

View reviewed changes

TheMarex added the Work In Progress label Apr 23, 2018

TheMarex added this to the distance-matrix milestone Apr 24, 2018

TheMarex assigned ghoshkaj Apr 24, 2018

ghoshkaj added 2 commits April 23, 2018 23:45

fix boost spirit grammar parsing for annotations

d4a1569

change grammer parsing according to review

return NotImplemented when distance annotation is requested for MLD i…

248a786

…n table plugin format

ghoshkaj force-pushed the fix/parse-table-annotations-param-correctly branch from 32c42f8 to efd426d Compare April 24, 2018 03:46

update docs

1f21b86

ghoshkaj force-pushed the fix/parse-table-annotations-param-correctly branch from efd426d to 1f21b86 Compare April 24, 2018 03:53

ghoshkaj added Review and removed Work In Progress labels Apr 24, 2018

chaupow approved these changes Apr 24, 2018

View reviewed changes

ghoshkaj merged commit 89f6e2d into master Apr 24, 2018

ghoshkaj deleted the fix/parse-table-annotations-param-correctly branch April 24, 2018 15:05

ghoshkaj removed the Review label Apr 24, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Parse table annotations param correctly #5050

Parse table annotations param correctly #5050

ghoshkaj commented Apr 23, 2018 •

edited

Loading

oxidase Apr 23, 2018

chaupow left a comment

chaupow Apr 23, 2018

chaupow Apr 23, 2018

chaupow Apr 23, 2018

chaupow Apr 23, 2018

chaupow left a comment

Parse table annotations param correctly #5050

Parse table annotations param correctly #5050

Conversation

ghoshkaj commented Apr 23, 2018 • edited Loading

Issue

Tasklist

Requirements / Relations

oxidase Apr 23, 2018

Choose a reason for hiding this comment

chaupow left a comment

Choose a reason for hiding this comment

chaupow Apr 23, 2018

Choose a reason for hiding this comment

chaupow Apr 23, 2018

Choose a reason for hiding this comment

chaupow Apr 23, 2018

Choose a reason for hiding this comment

chaupow Apr 23, 2018

Choose a reason for hiding this comment

chaupow left a comment

Choose a reason for hiding this comment

ghoshkaj commented Apr 23, 2018 •

edited

Loading