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

chore(sf|h3): reimplement basic h3 functions #489

Merged

Conversation

DeanSherwin
Copy link
Contributor

@DeanSherwin DeanSherwin commented Mar 22, 2024

Description

Shortcut

Type of change

  • Refactor

Change our H3 functions to use Snowflakes new native H3 functions.

Acceptance

View or run the snowflake_test.sh benchmark test script (available here - https://app.shortcut.com/cartoteam/story/391762/run-benchmarks-with-new-at-snowflake-h3-functions). It runs each of the re-implemented functions on 'main' and this branch and dumps execution times to CSV.

Copy link

@CLAassistant
Copy link

CLAassistant commented Mar 22, 2024

CLA assistant check
All committers have signed the CLA.

@DeanSherwin DeanSherwin requested a review from vdelacruzb March 22, 2024 07:48
@DeanSherwin DeanSherwin force-pushed the chore/sc-391760/reimplement-basic-h3-functions-in-at-snowflake branch from 9579ad0 to 84485ae Compare March 22, 2024 07:54
@DeanSherwin DeanSherwin force-pushed the chore/sc-391760/reimplement-basic-h3-functions-in-at-snowflake branch from 8ccc3d6 to 3594117 Compare March 25, 2024 08:16
@vdelacruzb
Copy link
Contributor

I would update the Description -> Shortcut on this PR to contain the shortcut story and autolink.

Also I recommend taking a look to the CONTRIBUTING.md. You will find more details about conventions for PR titles.

In this case would be more like: chore(sf|h3): reimplement basic h3 functions

@DeanSherwin DeanSherwin changed the title Chore/sc 391760/reimplement basic h3 functions in at snowflake chore(sf|h3): reimplement basic h3 functions Mar 26, 2024
@Jesus89
Copy link
Member

Jesus89 commented Mar 26, 2024

Hi @DeanSherwin @vdelacruzb.

I think renaming the input parameters is not needed (that way we avoid the need to update the docs).

I've tested with index, INDEX, "INDEX", and it worked fine. What problem did you face @DeanSherwin?

@DeanSherwin
Copy link
Contributor Author

Hi @DeanSherwin @vdelacruzb.

I think renaming the input parameters is not needed (that way we avoid the need to update the docs).

I've tested with index, INDEX, "INDEX", and it worked fine. What problem did you face @DeanSherwin?

Not a technical problem, Just a habit. I like to avoid using reserved SQL keywords like INDEX or SIZE as parameter names.

I will change it back now. Thanks!

@Jesus89
Copy link
Member

Jesus89 commented Mar 26, 2024

Got it. Let's keep the interface for now to minimize the changes, but good point to consider in the future

@DeanSherwin DeanSherwin force-pushed the chore/sc-391760/reimplement-basic-h3-functions-in-at-snowflake branch from 5d6ff2b to 0f8cc80 Compare March 27, 2024 08:33
@DeanSherwin DeanSherwin force-pushed the chore/sc-391760/reimplement-basic-h3-functions-in-at-snowflake branch from 0f8cc80 to 2c68f1b Compare March 27, 2024 08:44
@DeanSherwin
Copy link
Contributor Author

@Jesus89 @vdelacruzb Guys - my thinking now is that H3_KDISTANCE_RING should be reverted to the original UDF as on main branch.

A single call to a JS library is much faster than using the new H3_GRID_DISK and then passing that array to a UDF for processing. My attempts at a better algorithm in JS failed. And any SQL I can create to get a better performance isn't compatible with a UDF. Thoughts?

Unlike H3_BOUNDARY I don't think this warrants a follow-up task as this function doesn't have a native SF implementation and is custom to CARTO's UDF library anyways.

@Jesus89
Copy link
Member

Jesus89 commented Mar 27, 2024

I agree. We could create a ticket to research in the future (same as H3_BOUNDARY) if H3_GRID_DISK gets improved or they provide a more useful function for our H3_KDISTANCE_RING.

@DeanSherwin DeanSherwin self-assigned this Mar 27, 2024
@DeanSherwin
Copy link
Contributor Author

I agree. We could create a ticket to research in the future (same as H3_BOUNDARY) if H3_GRID_DISK gets improved or they provide a more useful function for our H3_KDISTANCE_RING.

Agreed: https://app.shortcut.com/cartoteam/story/398983/research-an-improved-h3-kring-distance-for-at-sf

Copy link
Contributor

@vdelacruzb vdelacruzb left a comment

Choose a reason for hiding this comment

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

Nice job. LGTM

Once you have removed this file: clouds/snowflake/results.csv
You can proceed with merging

@DeanSherwin DeanSherwin force-pushed the chore/sc-391760/reimplement-basic-h3-functions-in-at-snowflake branch from 6603c08 to b83c587 Compare March 27, 2024 17:35
@DeanSherwin DeanSherwin merged commit f2920b7 into main Mar 27, 2024
10 checks passed
@vdelacruzb vdelacruzb mentioned this pull request Apr 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants