[ELE-489] Support dbt-trino connector

radium226 commented 1 year ago

Is your feature request related to a problem? Please describe. At $WORK, I heavily leverage on Trino to use multiple data sources within my DBT projects (using the dbt-trino connector).

It's also quite useful for storing DBT's test results in a different database than the analytical one (by using the --store-failures flag combined with the +schema and +database config).

Describe the solution you'd like Elementary is IMHO a better solution than the raw --store-failures flag and it could be great to be able to make it work with the dbt-trino connector.

Describe alternatives you've considered For now, none :) I'm still using the --store-failures flag but without the proper visualization, I can't make it very useful for the rest of the teams.

Additional context Nope.

Would you be willing to contribute this feature? Totally! I started to dig a little bit (I seached postgres__ to see what kind of macro needs to be implemented) but I need guidance on how to start and how to test.

_ELE-489

Maayan-s commented 1 year ago

Hi @radium226! We would be happy to support your effort in making Elementary compatible with Trino. To be honest I'm not familiar with it so I don't know how hard it would be.

Generally speaking, we implemented every platform-specific functionality using the adapter.dispatch functionality, as dbt recommends. You can see an example in this macro. However, where there was a dbt_utils macro that we could use, we did. I do see utils in the dbt-trino adapter, so it looks promising in that sense. Anyway, You can see here a workaround we did for a missing util in Spark.

I think you should approach it gradually - Step 1 - Add support for uploading dbt artifacts and run results (in the dbt package). Step 2 - Add support in the CLI for Slack alerts and UI generation. Step 3 - Add support for data anomaly detection test (the most complex and platform-specific part of the code right now).

You can see here a guide for testing: https://docs.elementary-data.com/general/contributions#contributing-to-the-dbt-package

leniartek commented 1 year ago

Hello @radium226 Happy to hear you benefit from Trino! (dbt-trino PM here).

This is the first time I see a request for Elementary integration, I will take a closer look.

Thanks for reaching out. Piotr

ndrluis commented 10 months ago

Hello @leniartek and @Maayan-s,

In the coming weeks, I will be dedicated to implementing this integration, as we need Elementary working with Trino.

@leniartek, I would be very grateful if you have already identified any requirements related to this integration and could share them with me, so I can have a better idea of what I need to focus on.

In any case, I am available if you require help with anything.

haritamar commented 10 months ago

Hi @ndrluis !

That is great to hear. FYI that in the upcoming Elementary release we are going to add Athena support thanks to the contribution of @artem-garmash with the help of @nicor88. I know that Athena v3 is based on Trino so I'm wondering if there will be common code to both.

Also a couple of notes:

Please see our contribution guide here
Please see especially the note about integration tests in the dbt package - in particular these should help you to verify various Elementary features work as expected.
I'll be more than happy to jump on a call to help you get started / assist in any other way.

Cheers, Itamar

Tomme commented 8 months ago

I'm unsure on the status of other peoples work, but I've been wanting to use Elementary with Trino as well so I threw up the two required pull requests:

Hopefully they won't need many changes and we get them merged soon 👀

jicuss commented 7 months ago

@Tomme this is exciting, Elementary supporting trino would be a huge win. What needs to happen to get this into the mainline?

radium226 commented 7 months ago

@haritamar, @Maayan-s: maybe do you have insight about how to move on with the 2 PRs of @Tomme? I think we're all waiting for it :)

haritamar commented 7 months ago

Hi @Tomme @radium226 , I will review this week and update here!

radium226 commented 7 months ago

Amazing, thanks a lot!

Le mar. 13 févr. 2024, 06:55, Itamar Hartstein @.***> a écrit :

Hi @Tomme https://github.com/Tomme @radium226 https://github.com/radium226 , I will review this week and update here!

— Reply to this email directly, view it on GitHub https://github.com/elementary-data/elementary/issues/739#issuecomment-1940471219, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABQFYUGOYSHICOUXX2AAOU3YTL55DAVCNFSM6AAAAAAVRDZQUCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNBQGQ3TCMRRHE . You are receiving this because you were mentioned.Message ID: @.***>

radium226 commented 7 months ago

Hello @haritamar! Do you have any news? Thanks!

Firstero commented 7 months ago

Hello @haritamar! Do you have any news? Thanks!

haritamar commented 7 months ago

Hi @Firstero @radium226 , Apologies for the delay, more urgent things came up, but hoping to get it done this week.

radium226 commented 7 months ago

Yay! Thanks!

radium226 commented 6 months ago

I hope I'm not too pushy... But do you have any news @haritamar? I can't wait to try it, that's why I'm asking :)

mtk12 commented 6 months ago

Hi @haritamar any news, can we please expedite this much needed Component 🙏

haritamar commented 5 months ago

Hi @Tomme @radium226 @mtk12 ! Apologies for the super long delay here, thank you for your patience.

I've actually reviewed both PRs, and also tested locally and in our CI. Created a PR on top of the dbt package PR that includes CI tests and a small fix.

Overall it looks really great, left a question here and a comment here that is actually on our end to fix as it's a fix in FE code.

I believe we'll be able to merge it in the next few days. and then have it in the next official version.

Cheers, Itamar

radium226 commented 5 months ago

This is one of the best news of the week so far :) Thanks a lot!

Le mar. 9 avr. 2024, 21:40, Itamar Hartstein @.***> a écrit :

Hi @Tomme https://github.com/Tomme @radium226 https://github.com/radium226 @mtk12 https://github.com/mtk12 ! Apologies for the super long delay here, thank you for your patience.

I've actually reviewed both PRs, and also tested locally and in our CI. Created a PR https://github.com/elementary-data/dbt-data-reliability/pull/687 on top of the dbt package PR that includes CI tests and a small fix.

Overall it looks really great, left a question here https://github.com/elementary-data/dbt-data-reliability/pull/652/files#r1558189324 and a comment here https://github.com/elementary-data/elementary/pull/1378/files#r1558190813 that is actually on our end to fix as it's a fix in FE code.

I believe we'll be able to merge it in the next few days. and then have it in the next official version.

Cheers, Itamar

— Reply to this email directly, view it on GitHub https://github.com/elementary-data/elementary/issues/739#issuecomment-2045931435, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABQFYUBCKEWVEEDICO2T32LY4Q72PAVCNFSM6AAAAAAVRDZQUCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANBVHEZTCNBTGU . You are receiving this because you were mentioned.Message ID: @.***>

Tomme commented 5 months ago

@haritamar Thank you for taking a look Itamar and getting the PR merged 🥳

If you have any questions please feel free to send me a message!

jicuss commented 5 months ago

@haritamar this is huge for me personally, thank you so much for getting this into the codebase

radium226 commented 5 months ago

@haritamar @Tomme: Thanks a lot, this is amazing!

haritamar commented 5 months ago

Hi @Tomme , thanks a lot for the contribution!

Yup just got it merged :) I made some small changes:

The main one is adding Trino tests to our dbt package CI.
Made the temp tables name have a maximum of 128 chars rather than 255, since it seemed to be an issue with the Hive connector.
Reduced the minimum version to 1.5.0 to make it a bit less strict - lmk if you think that's an issue, it seemed to work for me.

I'm still curious about this (e.g. making should_commit always false for Trino) - though all tests pass and it doesn't affect other adapters so didn't feel like a blocker to me in any case.

I'll update when this gets into an official version!

mtk12 commented 5 months ago

@haritamar @Tomme Thanks a lot for this

ndrluis commented 2 months ago

Hello everyone,

I have opened a PR to fix a problem with Trino and Elementary. I just copied the Athena behavior.

You can check it out here: https://github.com/elementary-data/dbt-data-reliability/pull/735

elementary-data / elementary

[ELE-489] Support dbt-trino connector #739