Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Save Atlas to ProtobufAtlas similar to TextAtlas #99

Merged
merged 39 commits into from
Mar 29, 2018
Merged

Save Atlas to ProtobufAtlas similar to TextAtlas #99

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
39 commits
Select commit Hold shift + click to select a range
bb7706a
Fully integrated protobuf build flow with gradle. See ECLIPSE_README.…
lucaspcram Mar 19, 2018
1890bc8
Fixed spotless application to ignore generated source code.
lucaspcram Mar 19, 2018
a03cade
Initial, naive implementation of proto specs for Tags and Points.
lucaspcram Mar 19, 2018
4e05e34
Small updates to the README files.
lucaspcram Mar 20, 2018
18611a3
Started implementation of naive ProtoAtlasBuilder
lucaspcram Mar 20, 2018
f382466
Implemented Point and Line packing.
lucaspcram Mar 21, 2018
fd9af27
Stylistic and structural changes per PR notes.
lucaspcram Mar 22, 2018
fbdaacc
Trying to get Areas working with the serializer.
lucaspcram Mar 22, 2018
c9720fe
Fixed tab indentation to use spaces in suppressions.xml
lucaspcram Mar 22, 2018
bc15496
Trying to fix the Area serialization. Currently failing due to Polygo…
lucaspcram Mar 22, 2018
a804b72
Fixed failing tests, which were due to bad copy paste.
lucaspcram Mar 22, 2018
40437f7
Integrated a few PR style suggestions.
lucaspcram Mar 22, 2018
70abe50
Added an area count to the metadata.
lucaspcram Mar 22, 2018
1eba0c9
Last of the copy paste shenanigans that I messed up. I hope. lol
lucaspcram Mar 22, 2018
d08f1a8
Ok. If this is not the last copy-paste bug then I resign.
lucaspcram Mar 22, 2018
5aa04a8
Initial drafts of Node and Edge protos.
lucaspcram Mar 22, 2018
539337e
Added unit tests for the proto converter classes.
lucaspcram Mar 23, 2018
0bfec87
Changed packages on the converter classes per PR suggestions.
lucaspcram Mar 23, 2018
5ce39d5
Forgot to stage this file.
lucaspcram Mar 23, 2018
6359a6f
Implemented support for Nodes and Edges.
lucaspcram Mar 23, 2018
ba3b136
Almost done with relations. Small bug with the OSM identifier. See un…
lucaspcram Mar 23, 2018
6db5ee7
Fixed bug with relation packing. Relations work!
lucaspcram Mar 24, 2018
a7266a6
Lots of ProtoAtlas testing.
lucaspcram Mar 26, 2018
27aa4e2
Added some commands for converting between Packed and ProtoAtlases. T…
lucaspcram Mar 26, 2018
4cd48d0
Fixed spotless formatting issue.
lucaspcram Mar 26, 2018
4a06a5d
Wrote a quick test class to investigate the proto size discrepancies.
lucaspcram Mar 26, 2018
cd9fb95
Fixed bug where tags containing delimiter characters were not deseria…
lucaspcram Mar 26, 2018
f8db3ff
Added a ProtoAtlas file suffix to FileSuffix per Mike suggestion
lucaspcram Mar 26, 2018
307b015
Changed hardcoded protoc value in build.gradle
lucaspcram Mar 27, 2018
0e19a0b
Tweak to buildflow. Gradle project refresh still requires a rebuild, but
lucaspcram Mar 28, 2018
2678b77
Small tweak to prevent Gradle Refresh from deleting the generated code.
lucaspcram Mar 28, 2018
bc6f404
Added a task to actually clear the generated code.
lucaspcram Mar 29, 2018
fe1e9a1
Slight tweak to build.gradle to fix beforeMerged
lucaspcram Mar 29, 2018
3918bb8
Hopefully ready for merge
lucaspcram Mar 29, 2018
ee43801
Finalized fixes to gradle. Everything should work now.
lucaspcram Mar 29, 2018
eeeb127
Slight change to README
lucaspcram Mar 29, 2018
0d4e3f0
Fixed checkstyle and spotless suppressions to deal with new generated…
lucaspcram Mar 29, 2018
9657bac
Final changes before merge. Hopefully we are ready to go.
lucaspcram Mar 29, 2018
79e7450
Fixed some nits.
lucaspcram Mar 29, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Almost done with relations. Small bug with the OSM identifier. See un… 
…it test.
  • Loading branch information
@lucaspcram
lucaspcram committed Mar 23, 2018
commit ba3b136ffdf1381001ab453b47d2cdd763635963
82 changes: 73 additions & 9 deletions src/main/java/org/openstreetmap/atlas/geography/atlas/builder/proto/ProtoAtlasBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,15 @@
import org.openstreetmap.atlas.geography.atlas.Atlas;
import org.openstreetmap.atlas.geography.atlas.AtlasMetaData;
import org.openstreetmap.atlas.geography.atlas.builder.AtlasSize;
import org.openstreetmap.atlas.geography.atlas.builder.RelationBean;
import org.openstreetmap.atlas.geography.atlas.items.Area;
import org.openstreetmap.atlas.geography.atlas.items.Edge;
import org.openstreetmap.atlas.geography.atlas.items.ItemType;
import org.openstreetmap.atlas.geography.atlas.items.Line;
import org.openstreetmap.atlas.geography.atlas.items.Node;
import org.openstreetmap.atlas.geography.atlas.items.Point;
import org.openstreetmap.atlas.geography.atlas.items.Relation;
import org.openstreetmap.atlas.geography.atlas.items.RelationMember;
import org.openstreetmap.atlas.geography.atlas.packed.PackedAtlas;
import org.openstreetmap.atlas.geography.atlas.packed.PackedAtlasBuilder;
import org.openstreetmap.atlas.geography.converters.proto.ProtoLocationConverter;
Expand All @@ -30,6 +34,7 @@
import org.openstreetmap.atlas.proto.ProtoLocation;
import org.openstreetmap.atlas.proto.ProtoNode;
import org.openstreetmap.atlas.proto.ProtoPoint;
import org.openstreetmap.atlas.proto.ProtoRelation;
import org.openstreetmap.atlas.streaming.resource.Resource;
import org.openstreetmap.atlas.streaming.resource.WritableResource;
import org.openstreetmap.atlas.utilities.collections.Maps;
Expand Down Expand Up @@ -74,14 +79,10 @@ public PackedAtlas read(final Resource resource)
}

// initialize atlas metadata
final long numberOfPoints = protoAtlasContainer.getNumberOfPoints();
final long numberOfLines = protoAtlasContainer.getNumberOfLines();
final long numberOfAreas = protoAtlasContainer.getNumberOfAreas();
final long numberOfNodes = protoAtlasContainer.getNumberOfNodes();
final long numberOfEdges = protoAtlasContainer.getNumberOfEdges();
final long numberOfRelations = 0;
final AtlasSize atlasSize = new AtlasSize(numberOfEdges, numberOfNodes, numberOfAreas,
numberOfLines, numberOfPoints, numberOfRelations);
final AtlasSize atlasSize = new AtlasSize(protoAtlasContainer.getNumberOfEdges(),
protoAtlasContainer.getNumberOfNodes(), protoAtlasContainer.getNumberOfAreas(),
protoAtlasContainer.getNumberOfLines(), protoAtlasContainer.getNumberOfPoints(),
protoAtlasContainer.getNumberOfRelations());
// TODO it would be nice to programmatically determine which protobuf version is in use
final AtlasMetaData atlasMetaData = new AtlasMetaData(atlasSize, true, "unknown",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wondering if we should have any kind of tag to specify this atlas was built from Proto?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll take a look at that class and see if there's something I can add.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I just mean add an entry into the Metadata tags saying source=proto and proto_version=xx (not sure if you can read that dynamically, but assuming yes).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah ok yeah can do. Did you have some fancy method in mind or should I just write code that greps build.gradle?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought there might be an easier way! If there's not, I wouldn't worry about it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll mark it as TODO and if some simple way presents itself I'll be sure to implement it.

"ProtoAtlas", "unknown", "unknown", Maps.hashMap());
Expand All @@ -94,6 +95,7 @@ public PackedAtlas read(final Resource resource)
parseAreas(builder, protoAtlasContainer.getAreasList());
parseNodes(builder, protoAtlasContainer.getNodesList());
parseEdges(builder, protoAtlasContainer.getEdgesList());
parseRelations(builder, protoAtlasContainer.getRelationsList());

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there no relations being added to the new packed Atlas or am I missing something?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're getting there. It's still in development :)

return (PackedAtlas) builder.get();
}
Expand All @@ -110,12 +112,13 @@ public void write(final Atlas atlas, final WritableResource resource)
{
final ProtoAtlasContainer.Builder protoAtlasBuilder = ProtoAtlasContainer.newBuilder();

// get the Atlas features into the ProtoAtlasBuilder
// put the Atlas features into the ProtoAtlasBuilder
writePointsToBuilder(atlas, protoAtlasBuilder);
writeLinesToBuilder(atlas, protoAtlasBuilder);
writeAreasToBuilder(atlas, protoAtlasBuilder);
writeNodesToBuilder(atlas, protoAtlasBuilder);
writeEdgesToBuilder(atlas, protoAtlasBuilder);
writeRelationsToBuilder(atlas, protoAtlasBuilder);

final ProtoAtlasContainer protoAtlas = protoAtlasBuilder.build();
resource.writeAndClose(protoAtlas.toByteArray());
Expand Down Expand Up @@ -199,6 +202,37 @@ private void parsePoints(final PackedAtlasBuilder builder, final List<ProtoPoint
});
}

private RelationBean parseRelationBean(final ProtoRelation protoRelation)
{
final RelationBean bean = new RelationBean();

protoRelation.getBeansList().forEach(protoRelationBean ->
{
final long memberId = protoRelationBean.getMemberId();
final String memberRole = protoRelationBean.getMemberRole();
final ItemType memberType = ItemType
.forValue(protoRelationBean.getMemberType().getNumber());
bean.addItem(memberId, memberRole, memberType);
});

return bean;
}

private void parseRelations(final PackedAtlasBuilder builder,
final List<ProtoRelation> relations)
{
final ProtoTagListConverter protoTagListConverter = new ProtoTagListConverter();
relations.forEach(protoRelation ->
{
final long identifier = protoRelation.getId();
final RelationBean bean = parseRelationBean(protoRelation);
final Map<String, String> tags = protoTagListConverter
.convert(protoRelation.getTagsList());
// TODO should identifier and OSMIdentifier be the same?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They shouldn't be the same, one is the Atlas identifier, and one is the OSM identifier. Atlas identifier is padded with the country-slicing and way-sectioning digits. We should make a design decision on which one is passed in. This is somewhat tricky - since atlas identifier must be unique per type of entity (a Relation and an Edge can have the same atlas identifier, but no two Relations can have the same atlas identifier). So if this is a true representation of an Atlas, they should pass in the Atlas Identifier and we can leverage our existing factories to figure out the OSM Identifier based on the atlas identifier (remove the 6 digit padding). Let me know if that made sense!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm I see. I guess my question is, what should I be passing in for that second parameter? Will I need to have some additional code right here in this function to calculate the correct OSM id? Is there a reason the other builder 'add' methods for Node, Edge, Point, Line, etc. do not have an OSM id parameter?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check out ReverseIdentifierFactory - it can return the OSM identifier given an Atlas identifier. There is currently an issue for the other "add" methods to be consistent with this one. Sent you a pointer offline.

builder.addRelation(identifier, identifier, bean, tags);
});
}

private void writeAreasToBuilder(final Atlas atlas, final Builder protoAtlasBuilder)
{
long numberOfAreas = 0;
Expand Down Expand Up @@ -318,4 +352,34 @@ private void writePointsToBuilder(final Atlas atlas,
}
protoAtlasBuilder.setNumberOfPoints(numberOfPoints);
}

private void writeRelationsToBuilder(final Atlas atlas, final Builder protoAtlasBuilder)
{
long numberOfRelations = 0;
final ProtoTagListConverter protoTagListConverter = new ProtoTagListConverter();

for (final Relation relation : atlas.relations())
{
final ProtoRelation.Builder protoRelationBuilder = ProtoRelation.newBuilder();
protoRelationBuilder.setId(relation.getIdentifier());
for (final RelationMember member : relation.members())
{
final ProtoRelation.RelationBean.Builder beanBuilder = ProtoRelation.RelationBean
.newBuilder();
beanBuilder.setMemberId(member.getRelationIdentifier());
beanBuilder.setMemberRole(member.getRole());
final ItemType type = ItemType.forEntity(member.getEntity());
beanBuilder.setMemberType(ProtoRelation.ProtoItemType.valueOf(type.getValue()));

// TODO there may be a problem here if there are more relations in this atlas than
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doubt we will have 2 billion relations in an atlas. I would be more concerned with number of edges (due to way-sectioning and bi-directionality)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. This would be a problem as well, since the ProtoAtlasContainer, when deserialized into an object, stores each of the feature types in an ArrayList - this is built into protobuf. So in memory you have a List of Nodes, Points, Lines, Edges, etc. I would imagine you would never have 2 billion features in one of those categories in a single atlas.

Copy link
Contributor

@MikeGost MikeGost Mar 29, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's hope not, but theoretically this could happen! This is worth noting and identifying as a limitation of the current proto implementation.

// possible int32 values. This is because protobuf internally stores the protobeans
// list as an ArrayList
protoRelationBuilder.addBeans(beanBuilder.build());

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: extra line

}
numberOfRelations++;
protoAtlasBuilder.addRelations(protoRelationBuilder.build());
}
protoAtlasBuilder.setNumberOfRelations(numberOfRelations);
}
}
4 changes: 4 additions & 0 deletions src/main/proto/AtlasContainer.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import "Line.proto";
import "Area.proto";
import "Node.proto";
import "Edge.proto";
import "Relation.proto";

message ProtoAtlasContainer {
optional int64 numberOfPoints = 1;
Expand All @@ -26,4 +27,7 @@ message ProtoAtlasContainer {

optional int64 numberOfEdges = 9;
repeated ProtoEdge edges = 10;

optional int64 numberOfRelations = 11;
repeated ProtoRelation relations = 12;
}
30 changes: 30 additions & 0 deletions src/main/proto/Relation.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
syntax = "proto2";

option java_multiple_files = true;
option java_outer_classname = "ProtoRelationWrapper";

package org.openstreetmap.atlas.proto;

import "Tag.proto";

message ProtoRelation {
optional int64 id = 1;
repeated ProtoTag tags = 2;

enum ProtoItemType {
NODE = 0;
EDGE = 1;
AREA = 2;
LINE = 3;
POINT = 4;
RELATION = 5;
}

message RelationBean {
optional int64 memberId = 1;
optional string memberRole = 2;
optional ProtoItemType memberType = 3;
}

repeated RelationBean beans = 3;
}
7 changes: 7 additions & 0 deletions ...est/java/org/openstreetmap/atlas/geography/atlas/builder/proto/ProtoAtlasBuilderTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,13 @@ private PackedAtlasBuilder setUpTestAtlasBuilder()
shapePoints.add(Location.forString("48.34204,10.55844"));
packedAtlasBuilder.addEdge(getNextId(), new PolyLine(shapePoints), tags);

// add relations
// tags.clear();
// tags.put("relationtag", "somevalue");
// final RelationBean bean = new RelationBean();
// bean.addItem(1L, "This is the Eiffel Tower Point", ItemType.POINT);
// packedAtlasBuilder.addRelation(getNextId(), idCounter, bean, tags);

return packedAtlasBuilder;
}
}