Skip to content

Commit

Permalink
Fix hash collision in the string pool (#239)
Browse files Browse the repository at this point in the history 
* Current broken behavior

* More blatant test

* Fix hash collision in string pool

* Update version and changelog
  • Loading branch information
@e-n-f
e-n-f authored Jun 7, 2024
1 parent bb4f220 commit e11583d
Show file tree
Hide file tree
Showing 5 changed files with 46 additions and 8 deletions.
4 changes: 4 additions & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
# 2.55.0

* Fix hash collisions in the string pool

# 2.53.0

* Stop trying to add features to the tile after the feature limit is reached
Expand Down
14 changes: 7 additions & 7 deletions pool.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,15 +9,15 @@
#include "errors.hpp"
#include "text.hpp"

inline int swizzlecmp(const char *a, int atype, unsigned long long ahash, const char *b, int btype, unsigned long long bhash) {
if (ahash == bhash) {
inline long long swizzlecmp(const char *a, int atype, unsigned long long ahash, const char *b, int btype, unsigned long long bhash) {
if ((long long) ahash == (long long) bhash) {
if (atype == btype) {
return strcmp(a, b);
} else {
return atype - btype;
}
} else {
return (int) (ahash - bhash);
return (long long) (ahash - bhash);
}
}

Expand All @@ -44,10 +44,10 @@ long long addpool(struct memfile *poolfile, struct memfile *treefile, const char
}

while (*sp != 0) {
int cmp = swizzlecmp(s, type, hash,
poolfile->map.c_str() + ((struct stringpool *) (treefile->map.c_str() + *sp))->off + 1,
(poolfile->map.c_str() + ((struct stringpool *) (treefile->map.c_str() + *sp))->off)[0],
((struct stringpool *) (treefile->map.c_str() + *sp))->hash);
long long cmp = swizzlecmp(s, type, hash,
poolfile->map.c_str() + ((struct stringpool *) (treefile->map.c_str() + *sp))->off + 1,
(poolfile->map.c_str() + ((struct stringpool *) (treefile->map.c_str() + *sp))->off)[0],
((struct stringpool *) (treefile->map.c_str() + *sp))->hash);

if (cmp < 0) {
sp = &(((struct stringpool *) (treefile->map.c_str() + *sp))->left);
Expand Down
8 changes: 8 additions & 0 deletions tests/overture-235/in.json
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
{"type":"Feature","geometry":{"type":"Point","coordinates":[-73.8620223,-15.744153]},"properties":{"id":"0856c5143fffffff0138e27e7a03f7d8","@name":"Andenes","subtype":"locality","local_type":{"en":"hamlet"},"country":"PE","region":"PE-ARE","hierarchies":[[{"division_id":"085bc6b4ffffffff01fb3b67a2808435","subtype":"country","name":"Perú"},{"division_id":"08559acaffffffff01f27abb0a0cb14b","subtype":"region","name":"Arequipa"},{"division_id":"0856c834ffffffff01fd8ac1c98804cb","subtype":"county","name":"Caravelí"},{"division_id":"0856c53b7fffffff019e8d5b8b047485","subtype":"locality","name":"Chaparra"},{"division_id":"0856c5143fffffff0138e27e7a03f7d8","subtype":"locality","name":"Andenes"}]],"parent_division_id":"0856c53b7fffffff019e8d5b8b047485","perspectives":null,"norms":null,"population":null,"capital_division_id":null,"wikidata":null,"names":{"primary":"Andenes","common":{"en":"Andenes","es":"Andenes"},"rules":null},"version":0,"update_time":"2024-05-11T06:56:01Z","sources":[{"property":"","dataset":"OpenStreetMap","record_id":"N3522957503","confidence":null}]},"id":92526}
{"type":"Feature","geometry":{"type":"Point","coordinates":[-0.2216242,51.4948601]},"properties":{"id":"085070173fffffff014fa86038bb5217","@name":"Hammersmith And Fulham","subtype":"county","local_type":{"en":"county"},"country":"GB","region":"GB-ENG","hierarchies":[[{"division_id":"085eea613fffffff01a8a15aa3da4c44","subtype":"country","name":"United Kingdom"},{"division_id":"085255dd7fffffff011aba243af254bd","subtype":"region","name":"England"},{"division_id":"085070173fffffff014fa86038bb5217","subtype":"county","name":"Hammersmith And Fulham"}]],"parent_division_id":"085255dd7fffffff011aba243af254bd","perspectives":null,"norms":null,"population":null,"capital_division_id":null,"wikidata":null,"names":{"primary":"Hammersmith And Fulham","common":null,"rules":null},"version":0,"update_time":"2024-05-11T06:57:45Z","sources":[{"property":"","dataset":"geoBoundaries","record_id":"GBR9080712B6991151074246","confidence":null}]},"id":1096239}


{"type": "Feature", "properties": {"one": "Poull Bojer"}, "geometry": {"type":"Point","coordinates":[0,0]}}
{"type": "Feature", "properties": {"two":
"[[{\"division_id\":\"085ec5c23fffffff01339442745f3d55\",\"subtype\":\"country\",\"name\":\"Ecuador\"},{\"division_id\":\"085dac297fffffff0147c020488d610d\",\"subtype\":\"region\",\"name\":\"Cotopaxi\"},{\"division_id\":\"085bf1b2ffffffff012715ee575c3475\",\"subtype\":\"county\",\"name\":\"Sigchos\"},{\"division_id\":\"085bf010ffffffff014f248c7b4fe243\",\"subtype\":\"locality\",\"name\":\"Sigchos\"},{\"division_id\":\"085bf03a3fffffff01145a02d706b16f\",\"subtype\":\"locality\",\"name\":\"Tiliguila\"}]]"
}, "geometry": {"type":"Point","coordinates":[0,0]}}
26 changes: 26 additions & 0 deletions tests/overture-235/out/-z0.json
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
{ "type": "FeatureCollection", "properties": {
"antimeridian_adjusted_bounds": "-73.862022,-15.744153,0.000000,51.494860",
"bounds": "-73.862022,-15.744153,0.000000,51.494860",
"center": "0.000000,0.000000,0",
"description": "tests/overture-235/out/-z0.json.check.mbtiles",
"format": "pbf",
"generator_options": "./tippecanoe -q -a@ -f -o tests/overture-235/out/-z0.json.check.mbtiles -z0 tests/overture-235/in.json",
"json": "{\"vector_layers\":[{\"id\":\"in\",\"description\":\"\",\"minzoom\":0,\"maxzoom\":0,\"fields\":{\"@name\":\"String\",\"country\":\"String\",\"hierarchies\":\"String\",\"id\":\"String\",\"local_type\":\"String\",\"names\":\"String\",\"one\":\"String\",\"parent_division_id\":\"String\",\"region\":\"String\",\"sources\":\"String\",\"subtype\":\"String\",\"two\":\"String\",\"update_time\":\"String\",\"version\":\"Number\"}}],\"tilestats\":{\"layerCount\":1,\"layers\":[{\"layer\":\"in\",\"count\":4,\"geometry\":\"Point\",\"attributeCount\":14,\"attributes\":[{\"attribute\":\"@name\",\"count\":2,\"type\":\"string\",\"values\":[\"Andenes\",\"Hammersmith And Fulham\"]},{\"attribute\":\"country\",\"count\":2,\"type\":\"string\",\"values\":[\"GB\",\"PE\"]},{\"attribute\":\"hierarchies\",\"count\":2,\"type\":\"string\",\"values\":[]},{\"attribute\":\"id\",\"count\":2,\"type\":\"string\",\"values\":[\"085070173fffffff014fa86038bb5217\",\"0856c5143fffffff0138e27e7a03f7d8\"]},{\"attribute\":\"local_type\",\"count\":2,\"type\":\"string\",\"values\":[\"{\\\"en\\\":\\\"county\\\"}\",\"{\\\"en\\\":\\\"hamlet\\\"}\"]},{\"attribute\":\"names\",\"count\":2,\"type\":\"string\",\"values\":[\"{\\\"primary\\\":\\\"Andenes\\\",\\\"common\\\":{\\\"en\\\":\\\"Andenes\\\",\\\"es\\\":\\\"Andenes\\\"},\\\"rules\\\":null}\",\"{\\\"primary\\\":\\\"Hammersmith And Fulham\\\",\\\"common\\\":null,\\\"rules\\\":null}\"]},{\"attribute\":\"one\",\"count\":1,\"type\":\"string\",\"values\":[\"Poull Bojer\"]},{\"attribute\":\"parent_division_id\",\"count\":2,\"type\":\"string\",\"values\":[\"085255dd7fffffff011aba243af254bd\",\"0856c53b7fffffff019e8d5b8b047485\"]},{\"attribute\":\"region\",\"count\":2,\"type\":\"string\",\"values\":[\"GB-ENG\",\"PE-ARE\"]},{\"attribute\":\"sources\",\"count\":2,\"type\":\"string\",\"values\":[\"[{\\\"property\\\":\\\"\\\",\\\"dataset\\\":\\\"OpenStreetMap\\\",\\\"record_id\\\":\\\"N3522957503\\\",\\\"confidence\\\":null}]\",\"[{\\\"property\\\":\\\"\\\",\\\"dataset\\\":\\\"geoBoundaries\\\",\\\"record_id\\\":\\\"GBR9080712B6991151074246\\\",\\\"confidence\\\":null}]\"]},{\"attribute\":\"subtype\",\"count\":2,\"type\":\"string\",\"values\":[\"county\",\"locality\"]},{\"attribute\":\"two\",\"count\":1,\"type\":\"string\",\"values\":[]},{\"attribute\":\"update_time\",\"count\":2,\"type\":\"string\",\"values\":[\"2024-05-11T06:56:01Z\",\"2024-05-11T06:57:45Z\"]},{\"attribute\":\"version\",\"count\":1,\"type\":\"number\",\"values\":[0],\"min\":0,\"max\":0}]}]}}",
"maxzoom": "0",
"minzoom": "0",
"name": "tests/overture-235/out/-z0.json.check.mbtiles",
"type": "overlay",
"version": "2"
}, "features": [
{ "type": "FeatureCollection", "properties": { "zoom": 0, "x": 0, "y": 0 }, "features": [
{ "type": "FeatureCollection", "properties": { "layer": "in", "version": 2, "extent": 4096 }, "features": [
{ "type": "Feature", "id": 1096239, "properties": { "id": "085070173fffffff014fa86038bb5217", "@name": "Hammersmith And Fulham", "subtype": "county", "local_type": "{\"en\":\"county\"}", "country": "GB", "region": "GB-ENG", "hierarchies": "[[{\"division_id\":\"085eea613fffffff01a8a15aa3da4c44\",\"subtype\":\"country\",\"name\":\"United Kingdom\"},{\"division_id\":\"085255dd7fffffff011aba243af254bd\",\"subtype\":\"region\",\"name\":\"England\"},{\"division_id\":\"085070173fffffff014fa86038bb5217\",\"subtype\":\"county\",\"name\":\"Hammersmith And Fulham\"}]]", "parent_division_id": "085255dd7fffffff011aba243af254bd", "names": "{\"primary\":\"Hammersmith And Fulham\",\"common\":null,\"rules\":null}", "version": 0, "update_time": "2024-05-11T06:57:45Z", "sources": "[{\"property\":\"\",\"dataset\":\"geoBoundaries\",\"record_id\":\"GBR9080712B6991151074246\",\"confidence\":null}]" }, "geometry": { "type": "Point", "coordinates": [ -0.263672, 51.508742 ] } }
,
{ "type": "Feature", "id": 92526, "properties": { "id": "0856c5143fffffff0138e27e7a03f7d8", "@name": "Andenes", "subtype": "locality", "local_type": "{\"en\":\"hamlet\"}", "country": "PE", "region": "PE-ARE", "hierarchies": "[[{\"division_id\":\"085bc6b4ffffffff01fb3b67a2808435\",\"subtype\":\"country\",\"name\":\"Perú\"},{\"division_id\":\"08559acaffffffff01f27abb0a0cb14b\",\"subtype\":\"region\",\"name\":\"Arequipa\"},{\"division_id\":\"0856c834ffffffff01fd8ac1c98804cb\",\"subtype\":\"county\",\"name\":\"Caravelí\"},{\"division_id\":\"0856c53b7fffffff019e8d5b8b047485\",\"subtype\":\"locality\",\"name\":\"Chaparra\"},{\"division_id\":\"0856c5143fffffff0138e27e7a03f7d8\",\"subtype\":\"locality\",\"name\":\"Andenes\"}]]", "parent_division_id": "0856c53b7fffffff019e8d5b8b047485", "names": "{\"primary\":\"Andenes\",\"common\":{\"en\":\"Andenes\",\"es\":\"Andenes\"},\"rules\":null}", "version": 0, "update_time": "2024-05-11T06:56:01Z", "sources": "[{\"property\":\"\",\"dataset\":\"OpenStreetMap\",\"record_id\":\"N3522957503\",\"confidence\":null}]" }, "geometry": { "type": "Point", "coordinates": [ -73.828125, -15.707663 ] } }
,
{ "type": "Feature", "properties": { "one": "Poull Bojer" }, "geometry": { "type": "Point", "coordinates": [ 0.000000, 0.000000 ] } }
,
{ "type": "Feature", "properties": { "two": "[[{\"division_id\":\"085ec5c23fffffff01339442745f3d55\",\"subtype\":\"country\",\"name\":\"Ecuador\"},{\"division_id\":\"085dac297fffffff0147c020488d610d\",\"subtype\":\"region\",\"name\":\"Cotopaxi\"},{\"division_id\":\"085bf1b2ffffffff012715ee575c3475\",\"subtype\":\"county\",\"name\":\"Sigchos\"},{\"division_id\":\"085bf010ffffffff014f248c7b4fe243\",\"subtype\":\"locality\",\"name\":\"Sigchos\"},{\"division_id\":\"085bf03a3fffffff01145a02d706b16f\",\"subtype\":\"locality\",\"name\":\"Tiliguila\"}]]" }, "geometry": { "type": "Point", "coordinates": [ 0.000000, 0.000000 ] } }
] }
] }
] }
2 changes: 1 addition & 1 deletion version.hpp
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
#ifndef VERSION_HPP
#define VERSION_HPP

#define VERSION "v2.53.0"
#define VERSION "v2.55.0"

#endif

0 comments on commit e11583d

Please sign in to comment.