Update of the flex version of openstreetmap-carto

[mboeringa]

This an adjusted osm2pgsql flex 'openstreetmap-carto.lua' script / style that builds upon the good work of Paul Norman and adds in some nice options of the 'compatible.lua' file, part of a the osm2pgsql repository, that has been created by Jochen Topf.

Most notably, from Jochen's work, it adds:

• The ability to specify the table prefix (e.g. 'planet_osm')
• The ability to set the projection of the resulting geometries in the style file
• The option to choose between having Multipolygons or only Polygons created. Note: this also fixes the use of an outdated 'multi' option of the osm2pgsql flex output that was deprecated and finally removed in the latest flex based osm2pgsql versions. This issue caused a failure to run the existing 'flex-master' version of the Lua script using the latest osm2pgsql versions (>= 1.4.x).

On top of this, it adds a few other new configuration options, and extends code documentation:

• Ability to optionally suppress the creation of z-order or way_area columns
• Ability to optionally create the 'roads' and new 'route' table
• Ability to explicitly define columns to add, or alternatively create a minimum schema with all tags in the hstore column only.
• Fix for the outdated and broken link to osm2pgsql's 'pgsql' output, this has now been replaced by a reference to the new 'osm2pgsql.org' website.

Fixes # (id of the issue to be closed)

Changes proposed in this pull request:

Test rendering with links to the example places:

Before

After

[jeisenbe]

The Travis CI build failed

[pnorman]

As noted, this fails CI with errors in the unit tests

lua: ./openstreetmap-carto.lua:336: attempt to index global 'col_definition' (a nil value)
stack traceback:
        ./openstreetmap-carto.lua:336: in main chunk
        [C]: in function 'require'
        scripts/lua/test.lua:40: in main chunk
        [C]: in ?

You probably need to stub out col_definition in tests. In general, all new functionality should have accompanying tests.

The code you're doing seems to add options which could never be used with OpenStreetMap Carto. The goal of the transforms is to create a database for OpenStreetMap Carto, and there shouldn't be options which would break its usage with the style. Most of your options, if used, would break the database.

These might be useful in a general purpose transform for analytic and other purposes, but I want to keep what we're doing focused on our use case.

[mboeringa]

Hi @pnorman ,

Thanks for the review.

The code you're doing seems to add options which could never be used with OpenStreetMap Carto. The goal of the transforms is to create a database for OpenStreetMap Carto, and there shouldn't be options which would break its usage with the style. Most of your options, if used, would break the database.

Yes, I am well aware of the need to stay compatible with openstreetmap-carto, and fully concur these changes should not break such functionality. That is why I put up the big "NOTICE" remark on top of the file, and blocked out the configuration section, to fully make people aware that changing any of these settings likely breaks openstreetmap-carto rendering.

Nonetheless, it is fact of life that many people are using openstreetmap-carto as a basis for developing other styles, or different purposes like the ones I documented in the Lua file, but still want to be "compatible" with openstreetmap-carto in terms of for example the choices made for what constitutes an "area" object or not.

By adding these configuration options, alternative derivative styles are more easily developed, which I think is a bonus of this work. Aren't we always saying that it should be as easy as possible for new users to develop their own styles for OpenStreetMap data, possibly using openstreetmap-carto as example?

E.g. I know I read that at least one of the people active here in the openstreetmap-carto repository, wrote that he ran a rendering database and toolchain with Carto CSS style, with only 'hstore' as column, and no explicit columns.

Such derivative style can now be more easily supported and developed, simply by adjusting the new boolean switch for 'explicit_columns'.

Additionally, it could also help for future development of openstreetmap-carto. E.g., by making all columns for all tables explicit as is now the case, instead of one table inheriting the columns of another, it is easier to have a dedicated schema for each table and reduce database clutter. E.g. I doubt if we currently have 'line' rendering for "shops", yet the list of columns for the planet_osm_line table (which I maintained for compatibility), still contains the "shop" key as one of its attributes. If a future openstreetmap-carto developer felt that this column was superfluous, it can now be easily removed.

I really think this adds value for many people, while minimally impacting the current style. I think the added code complexity is minimal, and doesn't negatively impact maintenance, the code and setup of the Lua file is still very much your work.

If anyone else has input on this subject though, feel free to let your voice heard here as well.

You probably need to stub out col_definition in tests. In general, all new functionality should have accompanying tests.

The code doesn't add any new programmatic functions, it is just a few 'if...then...else' constructs added. The 'col_definition' variable is also from the existing work.

Bare with me, this is the first time ever I created a pull request on Github, and I am not familiar with CI testing, so I am not entirely sure what to do. How do I create a test, and exactly what needs to be tested and adjusted here??

[mboeringa]

@pnorman

I also have a specific question for you related to the code. In the existing file, you left a "TO DO" remark related to the route table setup (line 876 in the new adjusted file):

-- TODO: Remove this, roads tags don't belong on route relations
-- if roads(object.tags) then
-- add_roads(object)

As I wasn't exactly sure what this code was doing, I decided to simply comment out that section, but not remove it. Is this correct, or is this code section still needed somehow?

By the way, could it be that the Travis failure has to do with the adjustments to the "add*" functions related to the second "TO DO" you left in the code? That "TO DO" referred to the desire to have the "add*" functions take 'object' instead of 'object.tags' as input. I adjusted the code to conform to that. However, I could see how this could cause the Travis test to possibly fail... Could that indeed be the cause?

[mboeringa]

@pnorman and @jeisenbe

The first Travis error seems to have been related to a simple typo, I had col_definition in the code instead of the variable name col_definitions. I have added a commit fixing this.

Travis now emits another warning, that I am not sure how to interpret?:

[mboeringa]

I see it is related to this assert at line 45 in the 'test.lua' script of the flex/master branch:

assert(deepcompare(table_definitions.planet_osm_point.columns[1], { column = 'way', type = 'point' }), "planet_osm_point way column")

I don't think I made changes though to the order of the columns? So 'columns[1]' should still be OK??

[pnorman]

Do the tests work locally for you?

[mboeringa]

Do the tests work locally for you?

To be honest, I have no idea how to run the tests locally, I do not have a full stack of openstreetmap-carto related tools yet.

What is needed to run the tests locally? I do have the repository locally thanks to using GitHub Desktop on Windows. I can see the test.lua script in one of the folders.

I've verified the output of osm2pgsql only trying the different options now build in the style, and that seems OK so far.

[pnorman]

To be honest, I have no idea how to run the tests locally, I do not have a full stack of openstreetmap-carto related tools yet.

What is needed to run the tests locally? I do have the repository locally thanks to using GitHub Desktop on Windows. I can see the test.lua script in one of the folders.

lua scripts/lua/test.lua will run the tests.

You will need to figure out a way to run tests on all the new code behind options.

Before working on that, the questions about the purpose should be resolved. I could accepting options that don't cause the DB to not work for our purposes if they were widely needed, well documented, and didn't have invasive code changes. I can't see accepting options that prevent the style from working.

[mboeringa]

Before working on that, the questions about the purpose should be resolved. I could accepting options that don't cause the DB to not work for our purposes if they were widely needed, well documented, and didn't have invasive code changes. I can't see accepting options that prevent the style from working.

Before I do any more work, I think this needs more input from others then.

I personally don't see these changes as "invasive", just logical options missing and adding flexibility to the wide spread usage of this style.

The whole purpose of switching to the "flex output", and the development of flex output in the first place, was as I understood a desire to be more "flexible" with schema and such... I think this work is a natural extension of this, I cannot see this otherwise.

[mboeringa]

I've pushed another commit that fixes the route table issue, and additionally adds a few more configuration options that I think are valuable, and have minimal impact on the style code itself. Default configuration is still how the existing original flex/master branch would process the data.

[mboeringa]

@pnorman, I think the work I wanted to do on the flex/master Lua style file is finished.

I've added all general configuration options I personally see as having generic value to many people. I have no experience with writing tests for the CI testing framework, so don't think I can be of any help there at this point in time. I also don't run a full stack including updates, so cannot fully test this, but, considering the specific limited changes, I don't think e.g. updates should be affected. But of course, this would need proper testing by someone who does have the ability to do this.

Since you expressed some concerns and doubts about this work, I think it wise to pull in the other maintainers to see what they think about it. I have no problem if this ends up being just some fork, but I still do think these options add value for many users of openstreetmap-carto.

@gravitystorm, @matkoniecz, @matthijsmelissen, @imagico, @kocio-pl, @sommerluk, @jeisenbe

I don't know if any of you has followed some of this discussion regarding this pull request, but I would like to hear your opinions as well.

As stated in the first post, this pull request builds upon the good work of Paul Norman of converting the style's Lua script needed as input for osm2pgsql to the new osm2pgsql flex output framework, but extends it with a series of basic configuration settings that affect the resulting database schema created, and / or where and how the data is stored. Some of that work is based on Jochen Topf's work for e.g. the 'compatible.lua' file part of the osm2pgsql repository, but I also added new options.

The reason why I developed this is that I feel that many users of openstreetmap-carto, including developers wanting to create their own derivative style, users of e.g. QGIS, and users that want to do e.g. statistical analyses on the resulting output, may wish to have some easy, basic configuration options that don't require you to hack deep into the style and change e.g. Lua functions, with all the associated risks of breaking the Lua file's code. This is especially useful for people with little to no experience with programming and coding.

To accomplish this, I have added a new top section to the Lua file noted as 'BEGIN/END CONFIGURATION' in the code, that contains simple boolean options, and a few string keywords, that allows users to quickly adjust some important aspects of the import process. I have added quite extensive code comments to that section as well to properly document what each option does.

As a quick run-down of the changes, I repeat the list at the beginning of this thread, plus the latest changes. Please do take some time to review the code and code documentation as well, to fully understand what these options do (https://github.com/mboeringa/openstreetmap-carto/blob/patch-1/openstreetmap-carto.lua).

As to implementation, although Paul expressed concerns about the changes being "invasive", I actually think they are really restrained and limited in scope. No new Lua functions have been added, it is just a couple of configuration settings, and if..then..else constructs implementing the options. As such, I don't think they negatively influence the maintainability of the code.

Most notably, from Jochen's work, it adds:

• The ability to specify the table prefix (e.g. 'planet_osm')
• The ability to set the projection of the resulting geometries in the style file
• The option to choose between having Multipolygons or only Polygons created. Note: this also fixes the use of an outdated 'multi' option of the osm2pgsql flex output that was deprecated and finally removed in the latest flex based osm2pgsql versions. This issue caused a failure to run the existing 'flex-master' version of the Lua script using the latest osm2pgsql versions (>= 1.4.x).

On top of this, it adds a few other new configuration options based on the new osm2pgsql "flex output", and extends code documentation:

• The ability to specify the PostgreSQL schema for the created tables
• The ability to specify the PostgreSQL tablespaces where the data needs to be stored, including separate options for data and indexes.
• The ability to set either 'geometry' / 'multipolygon' or 'polygon' as the geometry column type for area objects.
• The ability to either automatically split long LineString objects, or maintain them "as is" in the database.
• The ability to optionally suppress the creation of z-order or way_area columns
• The ability to optionally create the 'roads' and new 'route' table
• The ability to explicitly define columns to add, or alternatively create a minimum schema with all tags in the hstore column only.
• Fix for the outdated and broken link to osm2pgsql's 'pgsql' output, this has now been replaced by a reference to the new 'osm2pgsql.org' website.

[mboeringa]

Since it is now 14 days without a response of any of the maintainers, I will close this pull request, and give it another few days before deleting the branch. There is no sense in keeping this open if there is no interest.

[jeisenbe]

@mboeringa you might want to consider given some more time for a response from @pnorman. I know that I can go for a month without time to review PRs, since sometimes other things in real life get in the way.

Unfortunately I am not really able to evaluate the risks and benefits of this proposed change myself, perhaps you could provide some real-world examples of where this would be needed? Would it make it easier to submit PRs for this style in the future?

Although you state "This is especially useful for people with little to no experience with programming and coding,” perhaps I count as “no experience” because I don’t understand how to use this.

[mboeringa]

@mboeringa you might want to consider given some more time for a response from @pnorman. I know that I can go for a month without time to review PRs, since sometimes other things in real life get in the way.

@jeisenbe Fair enough. Yes, I know how life gets in the way at times. That said, giving it another two weeks should be enough for an informed response by any of the maintainers. There is no sense in keeping this open for ever without feedback.

Although you state "This is especially useful for people with little to no experience with programming and coding,” perhaps I count as “no experience” because I don’t understand how to use this.

I can assure you that anyone who has ever run osm2pgsql by itself, should be able to understand these options, especially giving all the code documentation added in the CONFIGURATION section. Most of the options are simple boolean (True/False) options, and it is just a matter of changing them and inspecting the resulting database schema in PostgreSQL, to fully understand what these options do.

Also note that most of the ideas I implemented, are not necessarily mine, but based on observations and remarks I read from osm2pgsql and openstreetmap-carto users who requested such functionality. E.g. a good example of this is the ability to define the PostgreSQL 'schema' or 'tablespaces' the data ends up in. I have seen multiple requests for that kind of functionality by users of osm2pgsql or openstreetmap-carto over the years.

As such, I sincerely believe this functionality has a wider audience, and is not just my personal take on this.

[kocio-pl]

Unfortunately I can't discuss how much these specific examples are useful, because I don't tweak these things, but the idea is very good and helps a lot with customization.

For example Linux systems are full of configuration files that one can easily comment/uncomment and fill with values, without digging in the documentation for typical tasks. The comments inside act as a documentation and take a lot of space, even more than actual configuration.

[pnorman]

@mboeringa you might want to consider given some more time for a response from @pnorman. I know that I can go for a month without time to review PRs, since sometimes other things in real life get in the way.

I gave my response in a review earlier.

[mboeringa]

The code you're doing seems to add options which could never be used with OpenStreetMap Carto. The goal of the transforms is to create a database for OpenStreetMap Carto, and there shouldn't be options which would break its usage with the style. Most of your options, if used, would break the database.

Before working on that, the questions about the purpose should be resolved. I could accepting options that don't cause the DB to not work for our purposes if they were widely needed, well documented, and didn't have invasive code changes. I can't see accepting options that prevent the style from working.

@pnorman, @jeisenbe, @kocio-pl, I think these remarks sum up Paul's current view on this.

This makes me slightly hesitant to put further work in this for now. There is no point in further work if it is going to be rejected. So unless some of the other maintainers share their view, whether in line with Paul's, or more supportive of this pull, I don't know if it is worthwhile to keep this open.

[mboeringa]

@gravitystorm,

Maybe you as an experienced style developer having worked on multiple different styles, have some final words to say on this proposition, and whether or not you consider this valuable to the wider community that often use openstreetmap-carto as a base style or example, and possibly also taking into account potential secondary uses of the derived database.

I'd sincerely appreciate your take on this as well, also taking into account Paul's remarks and question about the purpose that needs to be resolved.

[gravitystorm]

I'm certainly not going to have any "final words" to say on this, since I'm currently inactive here and it's entirely up to the active maintainers to make these decisions.

With that said, yes, I've got experience with multiple different styles, and experience with creating and maintaining flex definitions. My views are strongly influenced by my lack of experience with Lua, which means I prefer to keep the lua files as simple as possible. So I am naturally wary of anything that introduces options that lead to 'if option ...' branching statements (as opposed to having an option replacing repeated usage of a magic number, or an option that's simply passed along into a function, neither of which add additional logic).

Also, there comes a point when a derivative project is no longer "based on" openstreetmap-carto, rather just "inspired by" it (or perhaps "originally based on" it). So changing a colour value is one thing, changing some style rules, changing the queries or layers rules but keeping the db structure - all of these are increasingly major surgery. But changing the entire database structure, as exemplified by the 'explicit columns' option - I don't think that's anything to do with openstreetmap-carto at that point, instead it becomes a totally different project. And I don't think it's the job of the openstreetmap-carto project to have a 'every conceivable database structure' lua file. I think it's fine for different projects to have their own flex files, or to have someone maintaining a 'all-options-possible' flex config as a standalone thing, or for osm2psql to have more example files; meanwhile, openstreetmap-carto has just the one it needs.

I do see some options that could be useful in limited circumstances (e.g. schema and table prefixes, where they are deployment concerns rather than development alternatives), but changing them would need options for all the other places they crop up (like indexes, layer definitions, etc etc) and that would add significant complexity to the whole project.

The whole purpose of switching to the "flex output", and the development of flex output in the first place, was as I understood a desire to be more "flexible" with schema and such... I think this work is a natural extension of this, I cannot see this otherwise.

I just want to pick up on this comment - using the "flex backend" is not the same as "being more flexible about schemas", it's about achieving goals that are not available with the other osm2pgsql backends. If the backend was called the 'chocolate' backend we wouldn't all need to eat chocolate to use it! It's entirely reasonable to use the osm2pgsql flex backend without providing any flexibility in the configuration files.

[mboeringa]

@gravitystorm,

Thanks for taking the time for commenting. It's quite clear to me now.

I will leave this branch linger for another two weeks after pushing one last fix, so anyone who feels a need could pick it up for personal usage, but then delete it. There's no use keeping it open for longer after that, just cluttering the repository.