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.

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

Flex version of the osm2pgsql configuration #4978

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

joto
Copy link

@joto joto commented Jun 4, 2024

This commit contains one file with an osm2pgsql configuration for the flex output that can be used instead of the old configuration for the pgsql output. It replaces the openstreetmap-carto.style and openstreetmap-carto.lua files.

The configuration is nearly 100% compatible to the old one.

The database layout will be exactly the same with just very little changes. The id columns (osm_id) and geometry columns (way) on all tables will get the NOT NULL flag when using the flex output. These have always been NOT NULL in practice anyway.

The content of the database will be the same with only minor irrelevant differences.

Run like this:
osm2pgsql -O flex --style openstreetmap-carto-flex.lua -d gis ~/path/to/data.osm.pbf

See #4977

@joto
Copy link
Author

joto commented Jun 16, 2024

I have

I have not touched the stuff in scripts/lua. It mentions old-style multipolygons and the last change is from 2020, so I am not sure how relevant that still is. It will not work with the new config file and making it work would be a larger effort.

Copy link
Collaborator

@imagico imagico left a comment

Choose a reason for hiding this comment

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

I have finished a first reading over the code, not tested it yet.

It looks fairly clean and strait away to understand. Some questions and comments inline.

Regarding the stuff in scripts/lua - that was introduced in #2128 but it was never properly documented how style developers are to use it, the readme essentially discourages its use. As indicated in the past i am very much in favor of introducing more automated testing to support style development and detecting unintentional changes in style behavior. But that would primarily be important for the rendering stage and less for the data import (see https://imagico.de/blog/en/systematic-testing-in-map-design/). And i don't really see this realistically being implemented through volunteer work.

For the data import stage testing - with the move to flex backend including customized processing not only of tags but also of geometries, proper testing would need to include that, i.e. it would need to assert match between input OSM data and output database content - and i don't think this is possible from within LUA.

openstreetmap-carto-flex.lua Show resolved Hide resolved
openstreetmap-carto-flex.lua Show resolved Hide resolved
-- ---------------------------------------------------------------------------

-- Needed for use with the Themepark framework
local themepark = ...
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am fine with leaving this in for now.

My reasoning is that - while as is there is no substantial benefit from using that - we will need to evaluate that based on the things we ultimately want to do with the flex backend (like the relations and boundary processing).

Copy link
Author

Choose a reason for hiding this comment

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

Oh, there is a lot of benefit there immediately for that. One of the most often requested features we get is: Can I have one database with both the data for rendering a map and also Nominatim for geocoding in it. This is possible with the flex output, you "just" need both configuations side by side. And the way to do that is using the Themepark framework which will "call" both configurations. The Nominatim config currently is not Themepark-ready, so we are not quite there yet, but with the Themepark support for OSM Carto you can already have the tables for OSM Carto and for the Shortbread scheme for vector tiles side by side in the same database.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Frankly, this is not really our concern here. We have the goal of adaptability and ease of use, but for the database this mainly means we try to ensure that the database schema can be customized easily and that other styles can easily co-use our database. Ensuring that sysadmins have it easier to deploy complex setups with different database schemas together is not our goal.

As said: I am fine with keeping the code for both using and not using themepark in for now and defer the actual decision for when we have a better basis for that. I am not fine with deciding to rely on a framework, which - by its own presentation - is in beta test.

joto added 3 commits July 7, 2024 18:23
This commit contains one file with an osm2pgsql configuration for the
flex output that can be used instead of the old configuration for the
pgsql output. It replaces the openstreetmap-carto.style and
openstreetmap-carto.lua files.

The configuration is nearly 100% compatible to the old one.

The database layout will be exactly the same with just very little
changes. The id columns (`osm_id`) and geometry columns (`way`) on all
tables will get the NOT NULL flag when using the flex output. These have
always been NOT NULL in practice anyway.

The content of the database will be the same with only minor irrelevant
differences.

Run like this:
osm2pgsql -O flex --style openstreetmap-carto-flex.lua -d gis ~/path/to/data.osm.pbf
This commit does the actual switch to the new flex config. It removes
the old config files and updates the documentation and various scripts.
They don't work any more with the new flex Lua output. And they are
have not been maintained anyway.
@joto
Copy link
Author

joto commented Jul 7, 2024

I have rebased the PR to master and added a commit removing the outdated Lua test scripts.

Is there anything else I can do to get this merged?

@imagico
Copy link
Collaborator

imagico commented Jul 8, 2024

Thanks.

I plan to look at this in more detail once we have finished work on #4952 and done a release with that and other pending changes - see #4981.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants