Making mssql provider to support only mssql+pymssql dialect

[rawwar]

Description

Currently, we allow users to pass dialect via sqlalchemy_scheme extra field of the mssql connections.

airflow/airflow/providers/microsoft/mssql/hooks/mssql.py

Line 80 in 3873b09

def sqlalchemy_scheme(self) -> str:

I am proposing to remove this field as it is not possible to support multiple dialects without installing the relevant dialect driver. Also, the documentation page does not even mention about sqlalchemy_scheme or the possibility of using other dialects.

Use case/motivation

Supporting multiple dialects requires the installation of multiple drivers. Since we only install pymssql, this allows the usage of the mssql+pymssql dialect alone and not others.

I am also interested in updating the documentation to reflect this.

Related issues

No response

Are you willing to submit a PR?

• Yes I am willing to submit a PR!

Code of Conduct

• I agree to follow this project's Code of Conduct

[rawwar]

@Taragolis , requesting your comments on this.

I was working on updating mssql provider to use sqlalchemy_url property in get_uri and got the idea for this issue.

[eladkal]

It looks like a small code change? If so it's better to simply raise PR we can have the discussion on the PR itself

[shahar1]

@rawwar Would you still like to work on this issue?

[dabla]

@Taragolis , requesting your comments on this.

I was working on updating mssql provider to use sqlalchemy_url property in get_uri and got the idea for this issue.

I've already fixed this issue in PR 40669. I've tested it with JdbcHook and OdbcHook on multiple databases and also with MsSqlHook and all works fine. You can also define the 'sqlalchemy_scheme' parameter in the extra field of the Connection when using JdbcHook so SQLAlchemy get's the correct dialect instead of the 'jdbc' protocol value.

[dabla]

An overview of all enhancements and fixes I did regarding hooks:

40665
40669
40751
40836

We already patched our Airflow production env with those changes and all works great.

[shahar1]

Thanks @dabla , I'll assign this issue on you then.

[dabla]

Description

Currently, we allow users to pass dialect via sqlalchemy_scheme extra field of the mssql connections.

airflow/airflow/providers/microsoft/mssql/hooks/mssql.py

Line 80 in 3873b09

def sqlalchemy_scheme(self) -> str:

I am proposing to remove this field as it is not possible to support multiple dialects without installing the relevant dialect driver. Also, the documentation page does not even mention about sqlalchemy_scheme or the possibility of using other dialects.

Use case/motivation

Supporting multiple dialects requires the installation of multiple drivers. Since we only install pymssql, this allows the usage of the mssql+pymssql dialect alone and not others.

I am also interested in updating the documentation to reflect this.

Related issues

No response

Are you willing to submit a PR?

• Yes I am willing to submit a PR!

Code of Conduct

• I agree to follow this project's Code of Conduct

We could "remove" that property in MsSqlHook, but we for example are actually using is as we have installed multiple drivers (pymssql/odbc/jdbc/...) to access multiple databases. Indeed the 'sqlalchemy_scheme' schould be documented, especially in the case of the JdbcHook are there it is required to define it if you want to use the sqlalchemy engine. Maybe we should generalize that property and move it to the DbApiHook, as this code is duplicated in MsSqlHook and OdbcHook.

[dabla]

As PR40669 has been closed I think this one can also be closed?