[Tile-serving] [openstreetmap/osm2pgsql] Add support for --extra-attributes to Gazetteer output (#905)

[Sarah Hoffmann]

lonvia requested changes on this pull request.

The '--extra-attributes' option has two important effects during imports/updates: a) it enables that all OSM objects are parsed, including the ones without tags and b) that metatags are saved in the middle tables. For this change you don't actually need either. Not a) because the gazetteer output only uses objects with interesting tags anyway. And not b) because gazetteer never reads tag (and thus meta) information from the middle table (unlike the other outputs it does not propagate node changes to their ways).

So I would suggest to rework the change to only check the style file for key entries that describe meta tags in order to find out if metatags are requested. The nice side effect is that you don't need the [additional change for Nominatim](https://github.com/openstreetmap/Nominatim/pull/1297). Another advantage is that you can do some sanity checks when reading the style file. Two come to mid immediately: that meta tags are only used with the extra flag and  that there is no value set for them.

The main drawback is that your example 1 does not work anymore. But having to spell out that some odd metatags are included may not be a bad thing.

Thinking that a bit further: if you save in a simple bitfield which pieces of metadata have been requested, then you don't even need the m_metadata vector anymore. You can simply write out the data directly into the buffer in the copy_out_maintag() function. Saves a couple more allocations.

> @@ -502,6 +547,13 @@ bool gazetteer_style_t::copy_out_maintag(pmaintag_t const &tag,
             escape_array_record(entry.second, buffer);
             buffer += "\",";
         }
+        for (auto const &entry : m_metadata) {
+            buffer += "\"";
+            escape_array_record(entry.first.c_str(), buffer);

You can save the escaping here. All entry.first are well-known strings.

> @@ -12,6 +12,7 @@ class gazetteer_style_t
 {
     using flag_t = uint16_t;
     using ptag_t = std::pair<char const *, char const *>;
+    using ptag_str_t = std::pair<const std::string, const std::string>;

'char const *' will do for the first parameter. It is only used with constant strings.

>  private:
     void add_style_entry(std::string const &key, std::string const &value,
                          flag_t flags);
     flag_t parse_flags(std::string const &str);
     flag_t find_flag(char const *k, char const *v) const;
+    void add_metadata_field(const std::string&& field, const std::string&& value);
+
+    template <typename T>
+    void add_metadata_field_num(const std::string&& field, const T value);

Please use the const placement style as in the rest of the file.

> +        if (o.user() && *(o.user()) != '\0') {
+            std::string username = o.user();
+            add_metadata_field("osm_user", std::move(username));
+        }
+
+        if (o.changeset()) {
+            add_metadata_field_num<osmium::changeset_id_type>("osm_changeset", o.changeset());
+        }
+
+        if (o.timestamp()) {
+            add_metadata_field("osm_timestamp", std::move(o.timestamp().to_iso()));
+        }
+    }
+}
+
+void gazetteer_style_t::add_metadata_field(const std::string&& field, const std::string&& value) {

Please run clang-format over your change (see CONTRIBUTING.md).

> +        }
+    }
+}
+
+void gazetteer_style_t::add_metadata_field(const std::string&& field, const std::string&& value) {
+    // We have to work with std::string, not char* because metadata fields converted to char*
+    // would require an allocation on heap and a cleanup at the end.
+    flag_t flag = find_flag(field.c_str(), value.c_str());
+    if (flag & SF_EXTRA) {
+        m_metadata.emplace_back(std::move(field), std::move(value));
+    }
+}
+
+template <typename T>
+void gazetteer_style_t::add_metadata_field_num(const std::string&& field, const T value) {
+    // This method is not linked to from outside this class. Therefore, it can stay in the source file.

I'm not sure, I'm comfortable with that. Better move it to the header.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/openstreetmap/osm2pgsql/pull/905#pullrequestreview-199806253
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstreetmap.org/pipermail/tile-serving/attachments/20190204/dcb0978c/attachment.html>