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 2020/07/23 19:45:29 UTC
[GitHub] [airflow] JCoder01 opened a new pull request #9960: Implement passing a list into get_records
JCoder01 opened a new pull request #9960:
URL: https://github.com/apache/airflow/pull/9960
closes: 9957
Implements ability to pass a list of queries to the `get_records` dbapi_hook. This is functionality that is listed in the docstring but was not implemented
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+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 [UPDATING.md](https://github.com/apache/airflow/blob/master/UPDATING.md).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] closed pull request #9960: Implement passing a list into get_records
Posted by GitBox <gi...@apache.org>.
github-actions[bot] closed pull request #9960:
URL: https://github.com/apache/airflow/pull/9960
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] JCoder01 commented on pull request #9960: Implement passing a list into get_records
Posted by GitBox <gi...@apache.org>.
JCoder01 commented on pull request #9960:
URL: https://github.com/apache/airflow/pull/9960#issuecomment-708004249
@HaloKo4 If you want to take this over, feel free, I've been slammed with "real" work and haven't had an opportunity to look at this lately.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] turbaszek commented on a change in pull request #9960: Implement passing a list into get_records
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #9960:
URL: https://github.com/apache/airflow/pull/9960#discussion_r487567921
##########
File path: airflow/hooks/dbapi_hook.py
##########
@@ -123,7 +123,8 @@ def get_pandas_df(self, sql, parameters=None, **kwargs):
with closing(self.get_conn()) as conn:
return psql.read_sql(sql, con=conn, params=parameters, **kwargs)
- def get_records(self, sql, parameters=None):
+
+ def _get_sql(self, sql, parameters, first_only):
"""
Executes the sql and returns a set of records.
Review comment:
Can you update the description with the behavior of passing a list?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] JCoder01 commented on pull request #9960: Implement passing a list into get_records
Posted by GitBox <gi...@apache.org>.
JCoder01 commented on pull request #9960:
URL: https://github.com/apache/airflow/pull/9960#issuecomment-679956821
@HaloKo4 this is a good point, I was trying to save adding another function, but I guess the cleanest way would be to add a function that takes a bool and based on that bool returns either `fetchall()` or `fetchone()`, then `get_records` and `get_first` just call this function with the appropriate flag
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] turbaszek commented on a change in pull request #9960: Implement passing a list into get_records
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #9960:
URL: https://github.com/apache/airflow/pull/9960#discussion_r487567645
##########
File path: airflow/hooks/dbapi_hook.py
##########
@@ -132,14 +133,35 @@ def get_records(self, sql, parameters=None):
:type sql: str or list
:param parameters: The parameters to render the SQL query with.
:type parameters: dict or iterable
+ :param first_only: Bool indicating whether to return full set or first record
+ :type first_only: bool
"""
+ if isinstance(sql, str):
+ sql = [sql]
with closing(self.get_conn()) as conn:
with closing(conn.cursor()) as cur:
- if parameters is not None:
- cur.execute(sql, parameters)
+ for sql_statement in sql:
+ if parameters is not None:
+ cur.execute(sql_statement, parameters)
+ else:
+ cur.execute(sql_statement)
+ if first_only:
+ return cur.fetchone()
else:
- cur.execute(sql)
- return cur.fetchall()
+ return cur.fetchall()
+
+
+ def get_records(self, sql, parameters=None):
Review comment:
```suggestion
def get_records(self, sql: Union[str, List[str]], parameters: Optional[Dict] = None):
```
##########
File path: airflow/hooks/dbapi_hook.py
##########
@@ -132,14 +133,35 @@ def get_records(self, sql, parameters=None):
:type sql: str or list
:param parameters: The parameters to render the SQL query with.
:type parameters: dict or iterable
+ :param first_only: Bool indicating whether to return full set or first record
+ :type first_only: bool
"""
+ if isinstance(sql, str):
+ sql = [sql]
with closing(self.get_conn()) as conn:
with closing(conn.cursor()) as cur:
- if parameters is not None:
- cur.execute(sql, parameters)
+ for sql_statement in sql:
+ if parameters is not None:
+ cur.execute(sql_statement, parameters)
+ else:
+ cur.execute(sql_statement)
+ if first_only:
+ return cur.fetchone()
else:
- cur.execute(sql)
- return cur.fetchall()
+ return cur.fetchall()
+
+
+ def get_records(self, sql, parameters=None):
+ """
+ Executes the sql and returns a set of records.
+
+ :param sql: the sql statement to be executed (str) or a list of
+ sql statements to execute
+ :type sql: str or list
Review comment:
```suggestion
:type sql: Union[str, List[str]]
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] turbaszek commented on a change in pull request #9960: Implement passing a list into get_records
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #9960:
URL: https://github.com/apache/airflow/pull/9960#discussion_r487567778
##########
File path: airflow/hooks/dbapi_hook.py
##########
@@ -132,14 +133,35 @@ def get_records(self, sql, parameters=None):
:type sql: str or list
:param parameters: The parameters to render the SQL query with.
:type parameters: dict or iterable
+ :param first_only: Bool indicating whether to return full set or first record
+ :type first_only: bool
"""
+ if isinstance(sql, str):
+ sql = [sql]
with closing(self.get_conn()) as conn:
with closing(conn.cursor()) as cur:
- if parameters is not None:
- cur.execute(sql, parameters)
+ for sql_statement in sql:
+ if parameters is not None:
+ cur.execute(sql_statement, parameters)
+ else:
+ cur.execute(sql_statement)
Review comment:
Do we need this if? I would expect that if we can do `cur.execute(sql_statement)` then default value for parameters is `None` so we can remove this if clause
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] stale[bot] commented on pull request #9960: Implement passing a list into get_records
Posted by GitBox <gi...@apache.org>.
stale[bot] commented on pull request #9960:
URL: https://github.com/apache/airflow/pull/9960#issuecomment-751240013
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] HaloKo4 commented on pull request #9960: Implement passing a list into get_records
Posted by GitBox <gi...@apache.org>.
HaloKo4 commented on pull request #9960:
URL: https://github.com/apache/airflow/pull/9960#issuecomment-707988879
@JCoder01 do u need help?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org