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

Support geometry collection in Quadint polyfill #150

Merged

Conversation

vdelacruzb
Copy link
Contributor

No description provided.

@shortcut-integration
Copy link

@vdelacruzb vdelacruzb requested a review from Jesus89 August 10, 2021 14:55
@vdelacruzb
Copy link
Contributor Author

Made tests in SF use ST_ASQUADINT_POLYFILL and also added duplicated features to tests to double check that the resulting array doesn't change as we remove duplicates.

Copy link
Member

@Jesus89 Jesus89 left a comment

Choose a reason for hiding this comment

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

LGTM!

@github-actions
Copy link
Contributor

Deployed in project bqcartost-core-150: https://console.cloud.google.com/bigquery?project=bqcartost-core-150

@Jesus89
Copy link
Member

Jesus89 commented Aug 11, 2021

I've tested this in staging and found the following error:

SELECT quadkey.ST_ASQUADINT_POLYFILL(geom,16) FROM `cartobq.testtables.polygons_800k` LIMIT 1000
-- Error: NULL argument passed to UDF at UDF$1(STRING, INT64) line 3, columns 8-9

This is because the dataset contains NULL geographies and this ends up in NULL GeoJSON. I think we should cover this case to avoid the bug, and return [] in that case. cc @martejpad

@vdelacruzb
Copy link
Contributor Author

vdelacruzb commented Aug 11, 2021

I've tested this in staging and found the following error:

SELECT quadkey.ST_ASQUADINT_POLYFILL(geom,16) FROM `cartobq.testtables.polygons_800k` LIMIT 1000
-- Error: NULL argument passed to UDF at UDF$1(STRING, INT64) line 3, columns 8-9

This is because the dataset contains NULL geographies and this ends up in NULL GeoJSON. I think we should cover this case to avoid the bug, and return [] in that case. cc @martejpad

We are returning this Error in purpose, I remember we talked about that months ago but it can be removed for sure, and replace it by an empty array. We should take the same approach in other functions then TOPARENT, TOCHILDREN...

@vdelacruzb
Copy link
Contributor Author

The NULL issue is related with #148.

I thought Bigquery's Safe Prefix was also working with UDFs. Should we request that? If it was supported all the functions should return Error by default.

@Jesus89
Copy link
Member

Jesus89 commented Aug 11, 2021

OK then. We can merge it as it is and create a ticket in CH to update this behavior in all the functions if required and possible.

@Jesus89
Copy link
Member

Jesus89 commented Aug 11, 2021

But remember to update the changelog first

@vdelacruzb vdelacruzb merged commit 9576dc4 into master Aug 11, 2021
@vdelacruzb vdelacruzb deleted the bug/ch173145/st-asquadint-polyfill-not-supporting-geometry branch August 11, 2021 08:00
@github-actions
Copy link
Contributor

Project bqcartost-core-150 deleted! 🔥

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