You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "LMnet (via GitHub)" <gi...@apache.org> on 2023/02/09 01:31:19 UTC
[GitHub] [airflow] LMnet opened a new pull request, #29434: Impovements for RedshiftDataOperator
LMnet opened a new pull request, #29434:
URL: https://github.com/apache/airflow/pull/29434
I added two improvements to the `RedshiftDataOperator`:
1. Better error reporting. Before my changes, all you get in case of an error is just a status (`FAILURE`). It's not very helpful. I added the whole response in the exception, so it will be printed in the logs and it will give a possibility to troubleshoot problems easily.
2. I added a `return_sql_result` optional parameter. By default it's `False`, so it's a backward-compatible change. If it's `True` operator will return a result of a SQL query from the `execute` method. It means that this result will be available through xcom variables. It may be handy if you need to get some small portions of data (or metadata) from Redshift. For example, I'm running `show table` query to get a DDL of some table and create a temporary copy of 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] LMnet commented on a diff in pull request #29434: Impovements for RedshiftDataOperator
Posted by "LMnet (via GitHub)" <gi...@apache.org>.
LMnet commented on code in PR #29434:
URL: https://github.com/apache/airflow/pull/29434#discussion_r1116478006
##########
airflow/providers/amazon/aws/operators/redshift_data.py:
##########
@@ -25,6 +26,8 @@
from airflow.providers.amazon.aws.hooks.redshift_data import RedshiftDataHook
from airflow.providers.amazon.aws.utils import trim_none_values
+from mypy_boto3_redshift_data.type_defs import GetStatementResultResponseTypeDef
+
Review Comment:
Fixed
--
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] boring-cyborg[bot] commented on pull request #29434: Impovements for RedshiftDataOperator
Posted by "boring-cyborg[bot] (via GitHub)" <gi...@apache.org>.
boring-cyborg[bot] commented on PR #29434:
URL: https://github.com/apache/airflow/pull/29434#issuecomment-1451587155
Awesome work, congrats on your first merged pull request!
--
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] o-nikolas commented on pull request #29434: Impovements for RedshiftDataOperator
Posted by "o-nikolas (via GitHub)" <gi...@apache.org>.
o-nikolas commented on PR #29434:
URL: https://github.com/apache/airflow/pull/29434#issuecomment-1449249247
Looks like tests are passing and changes have been made. @Taragolis your requested changes are blocking the merge. Does everything look acceptable from your end?
--
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] LMnet commented on pull request #29434: Impovements for RedshiftDataOperator
Posted by "LMnet (via GitHub)" <gi...@apache.org>.
LMnet commented on PR #29434:
URL: https://github.com/apache/airflow/pull/29434#issuecomment-1442768784
I rebased and squashed my changes.
--
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] vincbeck commented on a diff in pull request #29434: Impovements for RedshiftDataOperator
Posted by "vincbeck (via GitHub)" <gi...@apache.org>.
vincbeck commented on code in PR #29434:
URL: https://github.com/apache/airflow/pull/29434#discussion_r1101915933
##########
airflow/providers/amazon/aws/operators/redshift_data.py:
##########
@@ -47,6 +48,8 @@ class RedshiftDataOperator(BaseOperator):
:param with_event: indicates whether to send an event to EventBridge
:param await_result: indicates whether to wait for a result, if True wait, if False don't wait
:param poll_interval: how often in seconds to check the query status
+ :param return_sql_result: if True will return the result of an SQL statement,
+ if False will return statement ID
Review Comment:
+1
--
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] Taragolis commented on a diff in pull request #29434: Impovements for RedshiftDataOperator
Posted by "Taragolis (via GitHub)" <gi...@apache.org>.
Taragolis commented on code in PR #29434:
URL: https://github.com/apache/airflow/pull/29434#discussion_r1111063142
##########
airflow/providers/amazon/aws/operators/redshift_data.py:
##########
@@ -25,6 +26,8 @@
from airflow.providers.amazon.aws.hooks.redshift_data import RedshiftDataHook
from airflow.providers.amazon.aws.utils import trim_none_values
+from mypy_boto3_redshift_data.type_defs import GetStatementResultResponseTypeDef
+
Review Comment:
Please move it under the `if TYPE_CHECKING:` block
##########
tests/providers/amazon/aws/operators/test_redshift_data.py:
##########
@@ -140,3 +140,39 @@ def test_batch_execute(self, mock_conn):
StatementName=statement_name,
WithEvent=False,
)
+
+ @mock.patch("airflow.providers.amazon.aws.hooks.redshift_data.RedshiftDataHook.conn")
+ def test_return_sql_result(self, mock_conn):
+ expected_result = {"Result": True}
+ mock_conn.execute_statement.return_value = {"Id": STATEMENT_ID}
Review Comment:
```suggestion
mock_conn.batch_execute_statement.return_value = {"Id": STATEMENT_ID}
```
You need to mock `batch_execute_statement` here because you call
https://github.com/apache/airflow/blob/2a34dc9e8470285b0ed2db71109ef4265e29688b/airflow/providers/amazon/aws/operators/redshift_data.py#L124-L136
You could check it locally in Breeze or venv, for more detail see: [Airflow Unit Tests](https://github.com/apache/airflow/blob/main/TESTING.rst#id3)
--
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] boring-cyborg[bot] commented on pull request #29434: Impovements for RedshiftDataOperator
Posted by "boring-cyborg[bot] (via GitHub)" <gi...@apache.org>.
boring-cyborg[bot] commented on PR #29434:
URL: https://github.com/apache/airflow/pull/29434#issuecomment-1423481719
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst)
Here are some useful points:
- Pay attention to the quality of your code (ruff, mypy and type annotations). Our [pre-commits]( https://github.com/apache/airflow/blob/main/STATIC_CODE_CHECKS.rst#prerequisites-for-pre-commit-hooks) will help you with that.
- In case of a new feature add useful documentation (in docstrings or in `docs/` directory). Adding a new operator? Check this short [guide](https://github.com/apache/airflow/blob/main/docs/apache-airflow/howto/custom-operator.rst) Consider adding an example DAG that shows how users should use it.
- Consider using [Breeze environment](https://github.com/apache/airflow/blob/main/BREEZE.rst) for testing locally, it's a heavy docker but it ships with a working Airflow and a lot of integrations.
- Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
- Please follow [ASF Code of Conduct](https://www.apache.org/foundation/policies/conduct) for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
- Be sure to read the [Airflow Coding style]( https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#coding-style-and-best-practices).
Apache Airflow is a community-driven project and together we are making it better 🚀.
In case of doubts contact the developers at:
Mailing List: dev@airflow.apache.org
Slack: https://s.apache.org/airflow-slack
--
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] LMnet commented on a diff in pull request #29434: Impovements for RedshiftDataOperator
Posted by "LMnet (via GitHub)" <gi...@apache.org>.
LMnet commented on code in PR #29434:
URL: https://github.com/apache/airflow/pull/29434#discussion_r1101995475
##########
airflow/providers/amazon/aws/operators/redshift_data.py:
##########
@@ -47,6 +48,8 @@ class RedshiftDataOperator(BaseOperator):
:param with_event: indicates whether to send an event to EventBridge
:param await_result: indicates whether to wait for a result, if True wait, if False don't wait
:param poll_interval: how often in seconds to check the query status
+ :param return_sql_result: if True will return the result of an SQL statement,
+ if False will return statement ID
Review Comment:
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
[GitHub] [airflow] ephraimbuddy commented on a diff in pull request #29434: Impovements for RedshiftDataOperator
Posted by "ephraimbuddy (via GitHub)" <gi...@apache.org>.
ephraimbuddy commented on code in PR #29434:
URL: https://github.com/apache/airflow/pull/29434#discussion_r1101159734
##########
airflow/providers/amazon/aws/operators/redshift_data.py:
##########
@@ -47,6 +48,8 @@ class RedshiftDataOperator(BaseOperator):
:param with_event: indicates whether to send an event to EventBridge
:param await_result: indicates whether to wait for a result, if True wait, if False don't wait
:param poll_interval: how often in seconds to check the query status
+ :param return_sql_result: if True will return the result of an SQL statement,
+ if False will return statement ID
Review Comment:
Should we add that the default is False?
--
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] Taragolis merged pull request #29434: Impovements for RedshiftDataOperator
Posted by "Taragolis (via GitHub)" <gi...@apache.org>.
Taragolis merged PR #29434:
URL: https://github.com/apache/airflow/pull/29434
--
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] LMnet commented on a diff in pull request #29434: Impovements for RedshiftDataOperator
Posted by "LMnet (via GitHub)" <gi...@apache.org>.
LMnet commented on code in PR #29434:
URL: https://github.com/apache/airflow/pull/29434#discussion_r1116478361
##########
tests/providers/amazon/aws/operators/test_redshift_data.py:
##########
@@ -140,3 +140,39 @@ def test_batch_execute(self, mock_conn):
StatementName=statement_name,
WithEvent=False,
)
+
+ @mock.patch("airflow.providers.amazon.aws.hooks.redshift_data.RedshiftDataHook.conn")
+ def test_return_sql_result(self, mock_conn):
+ expected_result = {"Result": True}
+ mock_conn.execute_statement.return_value = {"Id": STATEMENT_ID}
Review Comment:
Fixed. Actually, I needed `execute_statement`, not `batch_execute_statement`. Looks like it was a copy-paste problem.
--
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