You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2022/09/28 08:58:46 UTC
[GitHub] [airflow] feluelle opened a new pull request, #26744: Rename schema to database in PostgresHook
feluelle opened a new pull request, #26744:
URL: https://github.com/apache/airflow/pull/26744
In PostgresHook the "schema" field is only being called like that to make it compatible with the underlying DbApiHook which uses the schema for the sql alchemy connector. The postgres connector library however does not allow setting a schema in the connection instead a database can be set. To clarify that, we change all references in the PostgresHook code and documentation.
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code changes, an Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvement+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in a newsfragment file, named `{pr_number}.significant.rst` or `{issue_number}.significant.rst`, in [newsfragments](https://github.com/apache/airflow/tree/main/newsfragments).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk merged pull request #26744: Rename schema to database in PostgresHook
Posted by GitBox <gi...@apache.org>.
potiuk merged PR #26744:
URL: https://github.com/apache/airflow/pull/26744
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] feluelle commented on pull request #26744: Rename schema to database in PostgresHook
Posted by GitBox <gi...@apache.org>.
feluelle commented on PR #26744:
URL: https://github.com/apache/airflow/pull/26744#issuecomment-1262359440
Thanks @jedcunningham, I will also fix the other issues.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] feluelle commented on a diff in pull request #26744: Rename schema to database in PostgresHook
Posted by GitBox <gi...@apache.org>.
feluelle commented on code in PR #26744:
URL: https://github.com/apache/airflow/pull/26744#discussion_r984549743
##########
airflow/providers/postgres/hooks/postgres.py:
##########
@@ -67,10 +68,18 @@ class PostgresHook(DbApiHook):
supports_autocommit = True
def __init__(self, *args, **kwargs) -> None:
+ if 'schema' in kwargs:
+ warnings.warn(
+ 'The "schema" arg has been renamed to "database" as it contained the database name.'
+ 'Please use "database" to set the database name.',
+ DeprecationWarning,
+ stacklevel=2,
+ )
+ kwargs['database'] = kwargs['schema']
super().__init__(*args, **kwargs)
self.connection: Connection | None = kwargs.pop("connection", None)
self.conn: connection = None
- self.schema: str | None = kwargs.pop("schema", None)
+ self.database: str | None = kwargs.pop("database", None)
Review Comment:
`db.schema` there refers to the connection schema - not the hook schema attribute.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] feluelle commented on pull request #26744: Rename schema to database in PostgresHook
Posted by GitBox <gi...@apache.org>.
feluelle commented on PR #26744:
URL: https://github.com/apache/airflow/pull/26744#issuecomment-1260604215
Added 1da4ee7
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a diff in pull request #26744: Rename schema to database in PostgresHook
Posted by GitBox <gi...@apache.org>.
ashb commented on code in PR #26744:
URL: https://github.com/apache/airflow/pull/26744#discussion_r984537244
##########
airflow/providers/postgres/hooks/postgres.py:
##########
@@ -67,10 +68,18 @@ class PostgresHook(DbApiHook):
supports_autocommit = True
def __init__(self, *args, **kwargs) -> None:
+ if 'schema' in kwargs:
+ warnings.warn(
+ 'The "schema" arg has been renamed to "database" as it contained the database name.'
+ 'Please use "database" to set the database name.',
+ DeprecationWarning,
+ stacklevel=2,
+ )
+ kwargs['database'] = kwargs['schema']
super().__init__(*args, **kwargs)
self.connection: Connection | None = kwargs.pop("connection", None)
self.conn: connection = None
- self.schema: str | None = kwargs.pop("schema", None)
+ self.database: str | None = kwargs.pop("database", None)
Review Comment:
I don't think we can even make it deprecated really:
This is in DbAPIHook:
``` def get_conn(self):
"""Returns a connection object"""
db = self.get_connection(getattr(self, self.conn_name_attr))
return self.connector.connect(host=db.host, port=db.port, username=db.login, schema=db.schema)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a diff in pull request #26744: Rename schema to database in PostgresHook
Posted by GitBox <gi...@apache.org>.
ashb commented on code in PR #26744:
URL: https://github.com/apache/airflow/pull/26744#discussion_r984576839
##########
airflow/providers/postgres/hooks/postgres.py:
##########
@@ -67,10 +68,18 @@ class PostgresHook(DbApiHook):
supports_autocommit = True
def __init__(self, *args, **kwargs) -> None:
+ if 'schema' in kwargs:
+ warnings.warn(
+ 'The "schema" arg has been renamed to "database" as it contained the database name.'
+ 'Please use "database" to set the database name.',
+ DeprecationWarning,
+ stacklevel=2,
+ )
+ kwargs['database'] = kwargs['schema']
super().__init__(*args, **kwargs)
self.connection: Connection | None = kwargs.pop("connection", None)
self.conn: connection = None
- self.schema: str | None = kwargs.pop("schema", None)
+ self.database: str | None = kwargs.pop("database", None)
Review Comment:
Oh right yes.
In which case I think it's still worth having a deprecation warning property.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a diff in pull request #26744: Rename schema to database in PostgresHook
Posted by GitBox <gi...@apache.org>.
kaxil commented on code in PR #26744:
URL: https://github.com/apache/airflow/pull/26744#discussion_r983958008
##########
airflow/providers/postgres/hooks/postgres.py:
##########
@@ -67,10 +68,18 @@ class PostgresHook(DbApiHook):
supports_autocommit = True
def __init__(self, *args, **kwargs) -> None:
+ if 'schema' in kwargs:
+ warnings.warn(
+ 'The "schema" arg has been renamed to "database" as it contained the database name.'
+ 'Please use "database" to set the database name.',
+ DeprecationWarning,
+ stacklevel=2,
+ )
+ kwargs['database'] = kwargs['schema']
super().__init__(*args, **kwargs)
self.connection: Connection | None = kwargs.pop("connection", None)
self.conn: connection = None
- self.schema: str | None = kwargs.pop("schema", None)
+ self.database: str | None = kwargs.pop("database", None)
Review Comment:
Thought: Should we add `schema` as property and raise deprecation warning for folks relying on `PostgresHook().schema`? Or we rather give them `AttributeError` as it is wrong anyways?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] feluelle commented on a diff in pull request #26744: Rename schema to database in PostgresHook
Posted by GitBox <gi...@apache.org>.
feluelle commented on code in PR #26744:
URL: https://github.com/apache/airflow/pull/26744#discussion_r984584554
##########
airflow/providers/postgres/hooks/postgres.py:
##########
@@ -67,10 +68,18 @@ class PostgresHook(DbApiHook):
supports_autocommit = True
def __init__(self, *args, **kwargs) -> None:
+ if 'schema' in kwargs:
+ warnings.warn(
+ 'The "schema" arg has been renamed to "database" as it contained the database name.'
+ 'Please use "database" to set the database name.',
+ DeprecationWarning,
+ stacklevel=2,
+ )
+ kwargs['database'] = kwargs['schema']
super().__init__(*args, **kwargs)
self.connection: Connection | None = kwargs.pop("connection", None)
self.conn: connection = None
- self.schema: str | None = kwargs.pop("schema", None)
+ self.database: str | None = kwargs.pop("database", None)
Review Comment:
Okay, will add it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] feluelle commented on a diff in pull request #26744: Rename schema to database in PostgresHook
Posted by GitBox <gi...@apache.org>.
feluelle commented on code in PR #26744:
URL: https://github.com/apache/airflow/pull/26744#discussion_r984529796
##########
airflow/providers/postgres/hooks/postgres.py:
##########
@@ -67,10 +68,18 @@ class PostgresHook(DbApiHook):
supports_autocommit = True
def __init__(self, *args, **kwargs) -> None:
+ if 'schema' in kwargs:
+ warnings.warn(
+ 'The "schema" arg has been renamed to "database" as it contained the database name.'
+ 'Please use "database" to set the database name.',
+ DeprecationWarning,
+ stacklevel=2,
+ )
+ kwargs['database'] = kwargs['schema']
super().__init__(*args, **kwargs)
self.connection: Connection | None = kwargs.pop("connection", None)
self.conn: connection = None
- self.schema: str | None = kwargs.pop("schema", None)
+ self.database: str | None = kwargs.pop("database", None)
Review Comment:
I don't have a strong opinion on this. @ashb @potiuk maybe?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a diff in pull request #26744: Rename schema to database in PostgresHook
Posted by GitBox <gi...@apache.org>.
ashb commented on code in PR #26744:
URL: https://github.com/apache/airflow/pull/26744#discussion_r984537244
##########
airflow/providers/postgres/hooks/postgres.py:
##########
@@ -67,10 +68,18 @@ class PostgresHook(DbApiHook):
supports_autocommit = True
def __init__(self, *args, **kwargs) -> None:
+ if 'schema' in kwargs:
+ warnings.warn(
+ 'The "schema" arg has been renamed to "database" as it contained the database name.'
+ 'Please use "database" to set the database name.',
+ DeprecationWarning,
+ stacklevel=2,
+ )
+ kwargs['database'] = kwargs['schema']
super().__init__(*args, **kwargs)
self.connection: Connection | None = kwargs.pop("connection", None)
self.conn: connection = None
- self.schema: str | None = kwargs.pop("schema", None)
+ self.database: str | None = kwargs.pop("database", None)
Review Comment:
I don't think we can even make it deprecated really:
This is in DbAPIHook:
```python
def get_conn(self):
"""Returns a connection object"""
db = self.get_connection(getattr(self, self.conn_name_attr))
return self.connector.connect(host=db.host, port=db.port, username=db.login, schema=db.schema)
```
So we need to add a
```python
@property
def schema(self):
return self.schema
```
I think. I can't see how we can do that without breaking anything which uses dbapi hook, such as `SQLTableCheckOperator`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] feluelle commented on a diff in pull request #26744: Rename schema to database in PostgresHook
Posted by GitBox <gi...@apache.org>.
feluelle commented on code in PR #26744:
URL: https://github.com/apache/airflow/pull/26744#discussion_r984589396
##########
airflow/providers/postgres/hooks/postgres.py:
##########
@@ -67,10 +68,18 @@ class PostgresHook(DbApiHook):
supports_autocommit = True
def __init__(self, *args, **kwargs) -> None:
+ if 'schema' in kwargs:
+ warnings.warn(
+ 'The "schema" arg has been renamed to "database" as it contained the database name.'
+ 'Please use "database" to set the database name.',
+ DeprecationWarning,
+ stacklevel=2,
+ )
+ kwargs['database'] = kwargs['schema']
super().__init__(*args, **kwargs)
self.connection: Connection | None = kwargs.pop("connection", None)
self.conn: connection = None
- self.schema: str | None = kwargs.pop("schema", None)
+ self.database: str | None = kwargs.pop("database", None)
Review Comment:
Added in 4a7ae31
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #26744: Rename schema to database in PostgresHook
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #26744:
URL: https://github.com/apache/airflow/pull/26744#issuecomment-1287947657
Conflicts need fixing :(
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] feluelle commented on pull request #26744: Rename schema to database in PostgresHook
Posted by GitBox <gi...@apache.org>.
feluelle commented on PR #26744:
URL: https://github.com/apache/airflow/pull/26744#issuecomment-1289445158
@potiuk Done. 😊
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org