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

Remove unneeded the_geom_webmercator param from ST_AsMVT #704

Merged
merged 1 commit into from
Sep 16, 2019

Conversation

rafatower
Copy link
Contributor

That parameter is not needed. As per the documentation:

geom_name is the name of the geometry column in the row
data. Default is the first geometry column.

This will let us push-down the whole ST_AsMVT expression to foreign
servers through FDW's.

That parameter is not needed. As per the documentation:

> geom_name is the name of the geometry column in the row
> data. Default is the first geometry column.

This will let us push-down the whole ST_AsMVT expression to foreign
servers through FDW's.
@Algunenano
Copy link
Contributor

This will let us push-down the whole ST_AsMVT expression to foreign
servers through FDW's.

Can you explain why, both here and in the code itself so we keep that information for future changes?

@rafatower
Copy link
Contributor Author

I just noticed this is a partial solution to a wider problem. When trying to push-down the ST_AsMVT to the remote end, I was getting this from the foreign end:

ERROR:  parse_column_keys: no geometry column found

The root cause is that, at some point in the process, the query is replacing column/field names by arbitrary f1, f2, ... aliases. When the foreign end checks for that geometry column and does not really find any column matching the_geom_webmercator, it fails.

But patching that yields to other problems down the line: as the column/field names are changed, the client cannot really process the obtained MVT. This is how it may look through vt2geojson:

{
  "type": "FeatureCollection",
  "features": [
    {
      "type": "Feature",
      "geometry": {
        "type": "LineString",
        "coordinates": [
          [
            -122.38100051879883,
            37.735969208590504
          ],
          [
            -122.38065719604492,
            37.7355619376922
          ],
          [
            -122.38027095794678,
            37.73535830140294
          ],
          [
            -122.3798418045044,
            37.73525648304823
          ],
          [
            -122.37941265106201,
            37.73525648304823
          ]
        ]
      },
      "properties": {
        "f2": "RD",
        "f3": 5
      }
    },
//...

instead of the expected:

{
  "type": "FeatureCollection",
  "features": [
    {
      "type": "Feature",
      "geometry": {
        "type": "LineString",
        "coordinates": [
          [
            -122.39580631256104,
            37.78835270558375
          ],
          [
            -122.39546298980713,
            37.78808138412046
          ]
        ]
      },
      "properties": {
        "st_type": "ST",
        "cartodb_id": 238
      }
    },
//...

so, let's put this on hold, and hopefully I'll find a way that avoids it altogether.

@Algunenano Algunenano closed this Sep 13, 2019
@rafatower
Copy link
Contributor Author

thanks!

@rafatower rafatower reopened this Sep 16, 2019
@rafatower rafatower changed the title [DNM] Remove unneeded the_geom_webmercator param from ST_AsMVT Remove unneeded the_geom_webmercator param from ST_AsMVT Sep 16, 2019
@rafatower
Copy link
Contributor Author

The root cause of this is an issue with type checking, FDW's and search paths, apparently fixed in recent versions of postgis: postgis/postgis@e096795#diff-86bc92dfea8fd30b975e278bd323067f

In order to increase compatibility with other postgis install, we need this patch.

@rafatower
Copy link
Contributor Author

For the record, the problem with column names / attributes is dealt with here: CartoDB/postgres#25

@Algunenano
Copy link
Contributor

In order to increase compatibility with other postgis install, we need this patch.

Go ahead, but in my opinion this isn't fixing the problem in the proper place.

@rafatower rafatower changed the title Remove unneeded the_geom_webmercator param from ST_AsMVT [DNM] Remove unneeded the_geom_webmercator param from ST_AsMVT Sep 16, 2019
@rafatower rafatower changed the title [DNM] Remove unneeded the_geom_webmercator param from ST_AsMVT Remove unneeded the_geom_webmercator param from ST_AsMVT Sep 16, 2019
@rafatower rafatower merged commit bce20f6 into master Sep 16, 2019
@rafatower rafatower deleted the remove-st-as-mvt-geom-param branch September 16, 2019 11:33
@rafatower rafatower restored the remove-st-as-mvt-geom-param branch September 16, 2019 14:30
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