🙌 Thanks @bastienboutonnet!

This repo contains another project which imports dbt-utils and runs integration tests against seed files. Check out https://github.com/fishtown-analytics/dbt-utils/pull/334/files for an example of configuring tests

Thanks for the pointers @joellabes I'll see about writing a test for this function at some point this week

@joellabes I added some tests and they seem to pass locally and on circle CI except for snowflake and BQ. I'm assuming because I'm a 3rd party these tests are bound to fail? Either way, let me know if that's the issue of if there is anything I should do (or not do) in the code.

Regarding the changelog todo on the PR template my question still stands let me know what I should do when we get closer to merge I guess?

I'm assuming because I'm a 3rd party these tests are bound to fail?

I don't think that's true - I've had all the adapters succeed on past PRs. My best guess is that BQ and SF do something differently - perhaps their column type inference isn't playing nicely with the seed, or the conversion_rate multiplication doesn't work for some reason. I only have access to a Redshift warehouse so can't muck around to find out why, and I'm external to Fishtown too so I can't see the CircleCI errors - @clrcrl will have to let you know the specifics.

In integration_tests/models/geo/test_haversine_distance_mi.sql, it might be nice to union that model and run it again without passing in the mi argument to test the default/backwards-compatible case as well (unless you have that covered somewhere else that I missed).

0.7.0 is the next significant (intentionally avoiding semver words) version of dbt-utils, so I expect that this sort of net-new functionality would go there as opposed to a patch version. Again a good q for Claire

@joellabes with the union part of your message I'm not sure I understand. I think I understand you'd like a test where we call the macro without the unit argument which makes tons of sense. But I don't know why union. Did you have something in mind?

Btw yeah you're right the Snowflake issue seems to be with the fact that it doesn't like ^. I think it probably uses pow() or something but then I'm curious if there is a snowflake adaptor specific version of the macro somewhere @joellabes @clrcrl ?

Did you have something in mind?

Broadly, I was thinking something like

select convert(lat_1, lon_1, lat_2, lon_2, ‘mi’)
from table

union all

select convert(lat_1, lon_1, lat_2, lon_2)
from table

(written on my phone so only very broadly accurate)

By doing that, you’d get 0, 1 or 2 failing rows depending on which part broke. It’s probably bad practice to overload the test, but I was trying to avoid having to make a third separate query 😬

not sure if there’s a SF-friendly version of the macro sorry! Weird that it got this far without breaking. Maybe it’s never worked but no one uses this on BQ or SF

@joellabes Ok I see what you meant. I can get that done in that way I think it's not that bad actually coming to think about it.

So yeah I checked into my team's macro for haversine and we have used the pow() functions on our Snowflake warehouse. I think there is a way to distribute warehouse specific macros and let the macro adaptor choose it at runtime, I'll see if there is an example of this already on the repo or wait till you or @clrcrl let me know how best to do it.

But YES! very strange people didn't complain so far, or maybe they have been like us and have used their own macros for haversine. Anyway, would be a great idea to fix that for everyone right?

UPDATE: I checked the postgres documentation and it looks like they also have a pow() function which could be also a solution, not sure if it works for both Redshift and BQ so we probably still want to go down the route of distributing adapted macros for some warehouses. Let me know what your thoughts are on this.

UPDATE 2 actually BQ fails on the radians so that's another issue and they don't seem to have a builtin math function to convert degrees to radians which is gonna call for an adapted macro for BQ for sure.

@joellabes I just implemented the unioned test and it seems to work fine on pg and redshift.

What do you think should happen qua the other dbs? Do we want to make adapters part of this PR?

I think if so, maybe we shouldn't as it's gonna be a pretty haphazard PR but I'd be happy to contribute to a snowflake adapter if I can figure out how the adapters macros are done here. It's been a while since my materialisation macros that I contributed in core.

I think I may just be good with creating a new macro and name it snowflake__haversine_distance with the db specific function in it and dbt would take care of selecting it based on the adapter type?

For BQ I was looking and they don't seem to have radians function to convert something that's in degrees + I cannot test it as I can't play with a BQ warehouse. Looks like if we implemented it for BQ we'd also have to make some maths happen to convert radians to degrees. It's probably doable though.

Let me know what makes sense.

Sorry for the delay getting back to you!

I think I may just be good with creating a new macro and name it snowflake__haversine_distance with the db specific function in it and dbt would take care of selecting it based on the adapter type?

Yes I think that's right!

Re BigQuery, if it was already broken and you don't use BQ, I don't think you need to feel obliged to bend over backwards to make it work. You could either add an implementation that no-ops and returns null, or throw the equivalent of a NotImplementedException, or do nothing at all and it will throw a more confusing error. When a BQ user comes along who wants this, they can always open another PR. I think there are other macros that do this but I don't remember which offhand.

no worries at all @joellabes

Ok I'll have a go at a snowflake implementation for sure because I can actually test it out. For BQ I think what you propose makes sense, however not sure why you're pointing me to the Microsoft website here:

or throw the equivalent of a NotImplementedException,

I'd actually like to be able to throw a proper error if BQ users were to use this macro rather than a null. How best do you think we can do that?

not sure why you're pointing me to the Microsoft website here

In a past life I was a C# developer, so that was the concept I knew it as - I'm not sure what the dbtonic way of doing this is.

The closest equivalent I could find was https://github.com/fishtown-analytics/dbt-utils/blob/c64b52068070023bc3524525e06a14ad72d485a4/macros/cross_db_utils/datediff.sql#L55. I think you'd basically have to make a stub of a bigquery implementation that threw the exception.

Then there would have to be some way of excluding that one from CI, because an exception that's intentionally thrown is still an exception... Hopefully @clrcrl has some way of excluding them somehow 🤔

@joellabes so I looked into the postgres and redshift documentations and they also have the pow() function that is the only way to make snowflake calculate power. So in the interest of not duplicating too much code I went ahead and made the default__ implementation use pow(). Judging by the tests this seems to work fine for PG, redshift and snowflake which is really nice.

I'll work on making the BQ exception raising happen soon.

@joellabes @clrcrl in the end I went ahead and also worked an adaptor macro for big query, it was a bit of a pain but I managed. I created so many failing CI runs I hope you folks don't get notified on each failures.

Anyway, let me know if you have any additional feedback

Incredible, thanks so much @bastienboutonnet! Sorry that this turned out to be so much more work than first thought 😭

Ah no worries, if I didn't want to do it I would have said it. I was stuck on my other open source projects today so it was something fun and useful to do.

Is there a changelog entry I should be writing btw? And or some documentation?

Also, not sure if this should be merged into the main branch or develop.

I guess by this point this PR is more of a feature than a bugfix so I rebase on dev/ and change the target of the PR.

Not sure on those logistics - Claire's got "clear the packages backlog" on her sprint for this fortnight so I imagine she'll be checking this out pretty soon though!

Hey @clrcrl thanks a lot for the feedback all done. Hope this works 🥳

@clrcrl Thanks for merging the PR. Btw, I had not added any changelog entries yet because I wasn't sure whether this was bug or feature. Would you like me to add something to the changelog or you will do it when you release?