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

Rewrite SQL queries so the COALESCE part works correctly #1038

Conversation

matthijsmelissen
Copy link
Collaborator

@matthijsmelissen
Copy link
Collaborator Author

@pnorman You mentioned:

I see a few rewrites to the query to simplify the SQL that can at the same time fix this problem.

Is this what you had in mind? I can't say it really simplifies the SQL... What do you think?

@pnorman
Copy link
Collaborator

pnorman commented Oct 11, 2014

@pnorman You mentioned:

I see a few rewrites to the query to simplify the SQL that can at the same time fix this problem.

Is this what you had in mind? I can't say it really simplifies the SQL... What do you think?

I said this in #942 (comment), which isn't one of these issues. In that case, I was going to move the UNION ALL deeper, so some common parts could be pulled out and not repeated. It was fairly specific to the roads, as we don't have anywhere else that has a UNION ALL quite like that.

I am quite skeptical about the SQL changes - it removes the WHERE conditions on the columns and replaces them with a WHERE NOT feature IS NULL. Although this gives the same output, it prevents partial index usage and forces the computation of the COALESCE before filtering.

In some ways, this is clearer

(SELECT
    way, name, religion,
    COALESCE(A, B, C) AS feature
  FROM 
  (SELECT
      way, z_order, way_area, name, 
      ('A_' || (CASE WHEN A in ('a','b') THEN A ELSE NULL END)) AS A,
      ('B_' || (CASE WHEN B in ('c','d') THEN B ELSE NULL END)) AS B,
      ('C_' || (CASE WHEN C in ('e','f') THEN C ELSE NULL END)) AS C
    FROM planet_osm_point
  ) AS p
  WHERE NOT COALESCE(A, B, C) IS NULL
  ORDER BY z_order, way_area DESC
) AS bar

(or replace that WHERE condition with WHERE NOT (A IS NULL AND B IS NULL AND C IS NULL)
It avoids having identical A in ('a','b') conditions in the CASE statements and later in the WHERE clause, but comes at the cost of no WHERE conditions that can be used for the filtering. I really don't think we can avoid the duplication.

So, in summary, 👎, primarily because we can't use this query form for other places where we do use partial indexes.

@matthijsmelissen
Copy link
Collaborator Author

I'm still very much on two minds about this. On the one hand, I agree that we should not write code that causes a further performance loss, given that performance is already a problem. On the other hand, having duplicate code is almost guaranteed to at some point lead to errors / inconsistencies when people edit the code.

Perhaps @gravitystorm also has an opinion?

Note that #985, #963, and #1029 depend on this.

@pnorman
Copy link
Collaborator

pnorman commented Dec 8, 2014

Looking at it more, I see this PR as a worse idea - people will copy the form of the query and use it elsewhere, which would be very bad.

@matthijsmelissen
Copy link
Collaborator Author

Would there really be no way in which we can avoid code duplication while still be able to use the partial indexes?

@pnorman
Copy link
Collaborator

pnorman commented Dec 9, 2014

We require a where clause that operates on on columns, not a computed value. Thinking about it, it's not just partial indexes, but query planning and a slew of other things.

@matthijsmelissen
Copy link
Collaborator Author

Yes, makes sense now I think of it. Thanks for the explanation.

@matthijsmelissen
Copy link
Collaborator Author

Closing this as it was rejected.

matthijsmelissen added a commit to matthijsmelissen/openstreetmap-carto that referenced this pull request Mar 4, 2015
* Make sure conditions are checked in both WHERE and COALESCE
  Checking the condition in the WHERE class is necessary for performance.
  Checking the condition in the COALESCE is necessary to prevent unrendered tags
  blocking rendering of tags further down in the COALESCE.
  See also the discussion [here](gravitystorm#1038 (comment)).

  This is an improvement of gravitystorm#1038.

  This resolves gravitystorm#985, resolves gravitystorm#1029, resolves gravitystorm#1336.

* Give amenity priority over shop.

  This resolves gravitystorm#963.

* Give tourism priority over amenity, shop, leisure, landuse, man_made, natural,
  and place.

  This resolves gravitystorm#1269, resolves first issue of gravitystorm#1232.
matthijsmelissen added a commit to matthijsmelissen/openstreetmap-carto that referenced this pull request Mar 4, 2015
* Make sure conditions are checked in both WHERE and COALESCE
  Checking the condition in the WHERE class is necessary for performance.
  Checking the condition in the COALESCE is necessary to prevent unrendered tags
  blocking rendering of tags further down in the COALESCE.
  See also the discussion [here](gravitystorm#1038 (comment)).

  This is an improvement of gravitystorm#1038.

  This resolves gravitystorm#985, resolves gravitystorm#1029, resolves gravitystorm#1336.

* Give amenity priority over shop.

  This resolves gravitystorm#963.

* Give tourism priority over amenity, shop, leisure, landuse, man_made, natural,
  and place.

  This resolves gravitystorm#1269, resolves first issue of gravitystorm#1232.
matthijsmelissen added a commit to matthijsmelissen/openstreetmap-carto that referenced this pull request Mar 4, 2015
* Make sure conditions are checked in both WHERE and COALESCE
  Checking the condition in the WHERE class is necessary for performance.
  Checking the condition in the COALESCE is necessary to prevent unrendered tags
  blocking rendering of tags further down in the COALESCE.
  See also the discussion [here](gravitystorm#1038 (comment)).

  This is an improvement of gravitystorm#1038.

  This resolves gravitystorm#985, resolves gravitystorm#1029, resolves gravitystorm#1336.

* Give amenity priority over shop.

  This resolves gravitystorm#963.

* Give tourism priority over amenity, shop, leisure, landuse, man_made, natural,
  and place.

  This resolves gravitystorm#1269, resolves first issue of gravitystorm#1232.
matthijsmelissen added a commit to matthijsmelissen/openstreetmap-carto that referenced this pull request Mar 9, 2015
* Make sure conditions are checked in both WHERE and COALESCE
  Checking the condition in the WHERE class is necessary for performance.
  Checking the condition in the COALESCE is necessary to prevent unrendered tags
  blocking rendering of tags further down in the COALESCE.
  See also the discussion [here](gravitystorm#1038 (comment)).

  This is an improvement of gravitystorm#1038.

  This resolves gravitystorm#985, resolves gravitystorm#1029, resolves gravitystorm#1336.

* Give amenity priority over shop.

  This resolves gravitystorm#963.

* Give tourism priority over amenity, shop, leisure, landuse, man_made, natural,
  and place.

  This resolves gravitystorm#1269, resolves first issue of gravitystorm#1232.
@matthijsmelissen matthijsmelissen deleted the improve-sql-coalesce branch March 16, 2015 11:17
sommerluk pushed a commit to sommerluk/openstreetmap-carto that referenced this pull request Mar 16, 2015
* Make sure conditions are checked in both WHERE and COALESCE
  Checking the condition in the WHERE class is necessary for performance.
  Checking the condition in the COALESCE is necessary to prevent unrendered tags
  blocking rendering of tags further down in the COALESCE.
  See also the discussion [here](gravitystorm#1038 (comment)).

  This is an improvement of gravitystorm#1038.

  This resolves gravitystorm#985, resolves gravitystorm#1029, resolves gravitystorm#1336.

* Give amenity priority over shop.

  This resolves gravitystorm#963.

* Give tourism priority over amenity, shop, leisure, landuse, man_made, natural,
  and place.

  This resolves gravitystorm#1269, resolves first issue of gravitystorm#1232.
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.

tourism=attraction not rendered when object also has unrendered amenity tag shop=no shouldn't get rendered
2 participants