Reducing the Use of oneof in Protos #3527

kevinkreiser · 2022-02-02T14:24:17Z

In a previous PR #3457, we migrated our protobuf definitions to proto3. When we did that we needed to make use of a trick that uses the oneof keyword to emulate the ability to do optional that was removed from the language in proto3. We took the conservative approach by simply adding oneof to any previously optional parameter. This is pretty ugly and quite verbose but it allowed us to make the migration without having to think about each individual field.

This PR significantly reduces the use of oneof. I accomplished this in the following ways:

in proto3 default values for all primative types are 0 (strings are empty string), this means that if the field has no semantic use for the 0 value (or empty string) we can simply allow these values to communicate them not being set. many many cases in the code worked that way so i was able to remove a great many oneofs this way
enums which make use of the default value 0 but do need differentiate being unset get a new default value denoting its unset-ness
deleteing unused options such as all the address stuff in location

Cases where we cannot remove oneof are those in which we need to differentiate between being set and not set (optional) but also the default value is not 0 in the case its not set. For example our node snap tolerance defaults to 5 meters when you dont set it, but if we removed the oneof this would end up making 0 the new default which is a breaking change. In those cases we need to keep using oneof. I s hould note that this only matters for pbf input, json input allows us to check whether something was or wasnt set.

…gearhart

…gearhart

…r and use it during serialization

mandeepsandhu · 2022-02-02T23:41:41Z

src/baldr/attributes_controller.cc


 #include <string>

 namespace valhalla {
-namespace thor {
+namespace baldr {


Whats the reason for moving this to baldr?

this was so that we could re-use this class in odin when we serialize the route out. im not sure its the best place, we could hvae argued to put it in tyr also i think. if you like tyr better we could totally move that in the next pr. also sorry for not answering this question earlier somehow i didnt see it!

mandeepsandhu · 2022-02-02T23:48:05Z

For string types, does this PR switch to using !string.empty() where previously we'd do has_string() ?

mandeepsandhu

Thanks for the cleanup! Too many diffs, I'll base my review on all tests passing! 🙂

…onal

kevinkreiser · 2022-02-10T17:06:53Z

@mandeepsandhu its ready for another look!

mandeepsandhu · 2022-02-10T19:50:49Z

proto/common.proto

-  oneof has_distance_from_leg_origin {
-    double distance_from_leg_origin = 55;
-  }
+  repeated PathEdge edges = 1;


any concerns renumbering the fields here and few other places? I remember we store the trip leg pbf for some of the pinpoint tests which would fail to read back the correct data if the pbf fields were renumbered.

I guess since the tests are passing the renumbering here does not affect the few places we've stored these protobufs on disk?

oh it broke the tests, but i fixed them by leaving the old defs there and copying them to the new defs and overwiiting the files. ive been doing this alot for all the protobuf changes, its a huge pain in the butt but its worth it to get the code cleaner. you see how it did it over here (takes a long time to load because of diff size): https://github.com/valhalla/valhalla/pull/3527/files#diff-836ee7bf7651f8100e6b6072ab85387a7fee2ea9178762aabf200726bd31a0d9R32

the other stuff i renumbered were things that were parsed from the request, so i could just call ParseApi and then again overwrite the proto after i've parsed the request. Also a pain in the butt but something ive been doing a lot with all these changes and organizational stuff

ah ok I missed some of those diffs at first glance (as you said, because of the size of the change github had issues expanding all the diffs).

thanks for updating the tests! 👍

mandeepsandhu · 2022-02-11T03:55:17Z

src/loki/worker.cc

@@ -44,7 +44,7 @@ void loki_worker_t::parse_locations(google::protobuf::RepeatedPtrField<valhalla:
      else if (location.radius() > max_radius)
        location.set_radius(max_radius);

-      if (!location.has_heading_tolerance_case())
+      if (location.has_heading_tolerance_case())


Not sure I follow this. We're now overriding heading_tolerance_case even if it explicitly set?

i do not know how that got in here, i didnt even change heading tolarance in the proto. ill add a unit test because obviously this isnt protected by existing tests

mandeepsandhu · 2022-02-11T04:02:13Z

src/thor/triplegbuilder.cc

@@ -445,9 +443,9 @@ void CopyLocations(TripLeg& trip_path,
    valhalla::Location* tp_intermediate = trip_path.add_location();
    tp_intermediate->CopyFrom(intermediate);
    // we can grab the right edge index in the path because we temporarily set it for trimming
-    if (!intermediate.correlation().has_leg_shape_index_case()) {
+    /*if (!intermediate.correlation().has_leg_shape_index_case()) {


Do we leave this commented?

this was basically an assertion, i think ill add a different check that every index is larger than the next or something so at least a check still exists.

mandeepsandhu

had a few clarifying question. rest lgtm. Thx for heavy lifting! 🙂

…check for route_leg_shape_index

mandeepsandhu

Thx for adding new tests. lgtm

kevinkreiser added 5 commits January 30, 2022 21:02

remove enum oneofs from options.proto and info.proto

52e185d

remove oneof from directions proto completely, too easy, thank you @d…

3de7c82

…gearhart

no need for oneof in sign.proto

6e155c9

remove oneof from trip.proto, also move attributes controller to bald…

e2a7136

…r and use it during serialization

remove more oneof's this time from common.proto

a19cb44

mandeepsandhu reviewed Feb 2, 2022

View reviewed changes

mandeepsandhu previously approved these changes Feb 2, 2022

View reviewed changes

fix trace logs

902a5ae

kevinkreiser dismissed mandeepsandhu’s stale review via 902a5ae February 3, 2022 15:58

kevinkreiser added 4 commits February 9, 2022 13:51

Merge remote-tracking branch 'origin/master' into kk_less_oneof

c57065c

costing can have a default of none more correlation stuff is non opti…

295ee65

…onal

move more outputs into the correlated structure

ffe4723

Merge branch 'master' into kk_less_oneof

0cf0b8a

kevinkreiser requested a review from mandeepsandhu February 10, 2022 17:06

mandeepsandhu reviewed Feb 10, 2022

View reviewed changes

mandeepsandhu reviewed Feb 11, 2022

View reviewed changes

mandeepsandhu previously approved these changes Feb 11, 2022

View reviewed changes

kevinkreiser added 2 commits February 11, 2022 22:19

add a unit test for typo in heading tolerance, reimplement a similar …

1a1000f

…check for route_leg_shape_index

Merge remote-tracking branch 'origin/kk_less_oneof' into kk_less_oneof

5b50b7a

kevinkreiser dismissed mandeepsandhu’s stale review via 5b50b7a February 12, 2022 03:19

mandeepsandhu approved these changes Feb 14, 2022

View reviewed changes

kevinkreiser merged commit b821ce2 into master Feb 14, 2022

kevinkreiser deleted the kk_less_oneof branch February 14, 2022 02:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reducing the Use of oneof in Protos #3527

Reducing the Use of oneof in Protos #3527

kevinkreiser commented Feb 2, 2022

mandeepsandhu Feb 2, 2022

kevinkreiser Feb 14, 2022

mandeepsandhu commented Feb 2, 2022

mandeepsandhu left a comment

kevinkreiser commented Feb 10, 2022

mandeepsandhu Feb 10, 2022

kevinkreiser Feb 10, 2022

kevinkreiser Feb 10, 2022

mandeepsandhu Feb 10, 2022

mandeepsandhu Feb 11, 2022

kevinkreiser Feb 12, 2022

kevinkreiser Feb 12, 2022

mandeepsandhu Feb 11, 2022

kevinkreiser Feb 12, 2022

kevinkreiser Feb 12, 2022

mandeepsandhu left a comment

mandeepsandhu left a comment

Reducing the Use of oneof in Protos #3527

Reducing the Use of oneof in Protos #3527

Conversation

kevinkreiser commented Feb 2, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mandeepsandhu commented Feb 2, 2022

mandeepsandhu left a comment

Choose a reason for hiding this comment

kevinkreiser commented Feb 10, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mandeepsandhu left a comment

Choose a reason for hiding this comment

mandeepsandhu left a comment

Choose a reason for hiding this comment