Clickhouse profile mapping

[roadan]

Description

This PR adds Clickhouse profile mapping using a generic connection type. To prevent cosmos from attaching all generic connections, it uses a required field named clickhouse mapped to extra.clickhouse.

To ensure the profile is claimed, users must add the following JSON to the extra field in the connection:

{
    "clickhouse": "True"
}

Related Issue(s)

closes #95

Breaking Change?

Checklist

[ x] I have made corresponding changes to the documentation (if required)
[ x] I have added tests that prove my fix is effective or that my feature works

[netlify]

👷 Deploy Preview for amazing-pothos-a3bca0 processing.

Name	Link
🔨 Latest commit	59c4225
🔍 Latest deploy log	https://app.netlify.com/sites/amazing-pothos-a3bca0/deploys/649e36946e5c4500089d1783

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.07%. Comparing base (1ebee49) to head (59c4225).
Report is 327 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #353      +/-   ##
==========================================
+ Coverage   88.91%   89.07%   +0.15%     
==========================================
  Files          39       41       +2     
  Lines        1299     1318      +19     
==========================================
+ Hits         1155     1174      +19     
  Misses        144      144              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[jlaneve]

This is looking great! Question for you - it looks like there’s a recommendation to use a SQLite connection for Clickhouse with Airflow (https://github.com/bryzgaloff/airflow-clickhouse-plugin). Do you think this should use generic or SQLite connection type?

[Gaploid]

Wow! Clickhouse should defiantly support! Im voting up for this!

[epikhinm]

Sorry for intervention but I don't think that using airflow-clickhouse-plugin) for dbt-clickhouse is a good idea actually.
dbt-clickhouse is github repo from original company ClickHouse Inc and airflow-cosmos should use bindings from official company, with a great team behind their backs.

Hi @silentsokolov . Could somebody from your company make a decision about future of this feature? All of us thrilled to ha ve the dbt-clickhouse in airflow-cosmos :)

[silentsokolov]

I am completely in favor of this feature. More tools for working with ClickHouse - that's awesome.

I'm not part of ClickHouse company, so it's better to ask @genzgd or @guykoh

[genzgd]

I don't claim to fully understand this project :), but I think this PR correctly uses dbt-clickhouse and not the airflow-clickhouse-plugin? Mapping values to the dbt-profile seems perfectly acceptable based on what I see.

[CorsettiS]

@jlaneve since clickhouse is has no official support on airflow it makes more sense from my perspective to use a generic http connector instead of SQLlite, which is somehow hacky. This implementation looks very solid for me as of now.

[jlaneve]

Hey folks, curious if we still want this profile mapping now that we have support for user-provided profiles.

Also worth noting that the Airflow Clickhouse integration recommends using a sqlite connection, so IMO this profile mapping should translate an Airflow sqlite connection to a dbt clickhouse profile, especially now that the recommendation is to explicitly import a profile mapping. This could look like:

from cosmos.profiles import ClickhouseUserPasswordProfileMapping

profile_config = ProfileConfig(
    profile_name="my_profile_name",
    target_name="my_target_name",
    profile_mapping=ClickhouseUserPasswordProfileMapping(
        conn_id="my_sqlite_connection", # use sqlite bc that's what airflow-clickhouse-plugin uses
        profile_args={
            "additional_arg": "my_value",
        },
    ),
)

[CorsettiS]

The sqlite conn and HTTP generic conn both have the same args if I am not wrong, therefore it is not a big deal to use one or another. In my current company we use the http conn for clickhouse to avoid mixing it up. In the end it is just a naming convention.
I would advocate for keeping this PR relevant since it is easier, at least from a users perspective, to store the creds in airflow instead of in a profiles.yml file.

[tatiana]

Please, could you add the mock_profile function & a test for it?
https://github.com/astronomer/astronomer-cosmos/blob/3786703609e69c1e8f4b2db1475fe8b6ea00a117/cosmos/profiles/base.py#L97C7-L97C7

This was introduced after this PR was created and is now required.

[tatiana]

Please, could you add the attribute:

is_community = True

https://github.com/astronomer/astronomer-cosmos/blob/3786703609e69c1e8f4b2db1475fe8b6ea00a117/cosmos/profiles/base.py#L33C31-L33C31

[tatiana]

@roadan, sorry for the delay. Could you please rebase this branch and address the comments?

@jlaneve, at least three community members would like this feature; what do you think about approving it once the feedback has been addressed and tests passed? It seems valuable, it is well documented and tested.

[alexisvannier]

Would be very nice to see this PR merged. 👍
Is it possible to help for testing ?

[vargacypher]

Would be very nice to see this PR merged. 👍
Is it possible to help in something ???

[vargacypher]

@roadan could i colaborate on Unresolved conversations ?
We really want this feature.

[pankajastro]

Hey @roadan, It looks like we're very close to merging! Could you please rebase and add the mock_profile property? Let me know if there's anything I can do to help. thanks!

[pankajastro]

Hi @roadan, I was wondering if you had a chance to read my previous comment. It seems like this feature is very important for the community. If it's alright with you, can I take it forward from here? Thanks!

[tatiana]

@roadan @vargacypher @alexisvannier @CorsettiS @genzgd @epikhinm @Gaploid

Since this work is still relevant, and the PR got stale, @pankajastro is rebasing the original implementation in #1016, so we can get this work merged and released as part of Cosmos 1.5. If we release an alpha, would one of you be able to help us testing this feature?

[vargacypher]

I could help on tests @tatiana

[tatiana]

@vargacypher, thanks a lot; we just merged #1016 and let you know once we have an alpha with this feature!