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 2021/11/21 08:40:24 UTC

[GitHub] [airflow] aakashnand opened a new pull request #19733: Allow session properties for trino connection

aakashnand opened a new pull request #19733:
URL: https://github.com/apache/airflow/pull/19733


   Hi, I was using the Trino provider package of airflow. While using I found out that I am not able to set `session_properties` for the Trino connector.  So I am creating this PR to fix this. This way if the user needs to add session_properties from Airflow UI they can add it like below
   
   ```
   {"catalog": "hive", "protocol": "https", "session_properties": "{'hive.insert_existing_partitions_behavior':'OVERWRITE', 'scale_writers':'true','task_writer_count':'1','writer_min_size':'32MB'}"}
   ```
   
   ![image](https://user-images.githubusercontent.com/14219201/142755452-28c0a44b-ac7c-4059-adff-453d35b4d5e9.png)
   


-- 
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 #19733: Allow session properties for trino connection

Posted by GitBox <gi...@apache.org>.

boring-cyborg[bot] commented on pull request #19733:
URL: https://github.com/apache/airflow/pull/19733#issuecomment-974776527

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 (flake8, 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] aakashnand edited a comment on pull request #19733: Allow session properties for trino connection

Posted by GitBox <gi...@apache.org>.

aakashnand edited a comment on pull request #19733:
URL: https://github.com/apache/airflow/pull/19733#issuecomment-975039473


   Got it. Let me try to work on the tests, I will need some time to understand the existing test. Will update the PR soon.


-- 
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] aakashnand commented on pull request #19733: Allow session properties for trino connection

Posted by GitBox <gi...@apache.org>.

aakashnand commented on pull request #19733:
URL: https://github.com/apache/airflow/pull/19733#issuecomment-975039473


   Got it. Let me try to work test, I will need some time to understand the existing test. Will update the PR soon.


-- 
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] uranusjr commented on pull request #19733: Allow session properties for trino connection

Posted by GitBox <gi...@apache.org>.

uranusjr commented on pull request #19733:
URL: https://github.com/apache/airflow/pull/19733#issuecomment-975141520


   > is it okay to arbitrarily add one or two session_properties and test it?
   
   Yes, just testing a couple of those is good enough (but maybe have some variants e.g. check if numbers and strings etc. all work? I’m not that familiar with Trino)
   
   > Also `assert_connection_called_wtih` needs to be modified like this right?
   
   I think so, it’s best to actually run the tests to make sure.


-- 
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] aakashnand commented on pull request #19733: Allow session properties for trino connection

Posted by GitBox <gi...@apache.org>.

aakashnand commented on pull request #19733:
URL: https://github.com/apache/airflow/pull/19733#issuecomment-975143407


   Understood, I am familiar with Trino but I am not so familiar with the tests written in that file, so I found it difficult to understand the flow of the tests and how they are getting called etc. I will work on it. Thanks for the 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.

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] github-actions[bot] commented on pull request #19733: Allow session properties for trino connection

Posted by GitBox <gi...@apache.org>.

github-actions[bot] commented on pull request #19733:
URL: https://github.com/apache/airflow/pull/19733#issuecomment-1007031303


   This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days 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.

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] aakashnand edited a comment on pull request #19733: Allow session properties for trino connection

Posted by GitBox <gi...@apache.org>.

aakashnand edited a comment on pull request #19733:
URL: https://github.com/apache/airflow/pull/19733#issuecomment-975110639


   @uranusjr Got it. I will try this way. One more question is that `session_properties` can contain many values which can be found in the official trino documentation so for the test is it okay to arbitrarily add one or two session_properties and test it?
   
   Also `assert_connection_called_wtih` needs to be modified like this right?
   
   ```diff
   def assert_connection_called_with(mock_connect, auth=None, verify=True, session_properties=None):
           mock_connect.assert_called_once_with(
               catalog='hive',
               host='host',
               port=None,
               http_scheme='http',
               schema='hive',
               source='airflow',
               user='login',
               isolation_level=0,
               auth=None if not auth else auth.return_value,
               verify=verify,
   +           session_properties= None if not session_properties else session_properties
           )
   ```


-- 
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] aakashnand commented on pull request #19733: Allow session properties for trino connection

Posted by GitBox <gi...@apache.org>.

aakashnand commented on pull request #19733:
URL: https://github.com/apache/airflow/pull/19733#issuecomment-975110639


   @uranusjr Got it. I will try this way. One more question is that `session_properties` can contain many values which can be found in the official trino documentation so for the test is it okay to arbitrarily add one or two session_properties and test 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] uranusjr commented on a change in pull request #19733: Allow session properties for trino connection

Posted by GitBox <gi...@apache.org>.

uranusjr commented on a change in pull request #19733:
URL: https://github.com/apache/airflow/pull/19733#discussion_r753900055



##########
File path: airflow/providers/trino/hooks/trino.py
##########
@@ -15,7 +15,7 @@
 # KIND, either express or implied.  See the License for the
 # specific language governing permissions and limitations
 # under the License.
-import os
+import os, ast

Review comment:
       ```suggestion
   import ast
   import os
   ```
   
   Please consider [setting up pre-commit](https://github.com/apache/airflow/blob/main/STATIC_CODE_CHECKS.rst#pre-commit-hooks) locally to catch issues like this.




-- 
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] aakashnand edited a comment on pull request #19733: Allow session properties for trino connection

Posted by GitBox <gi...@apache.org>.

aakashnand edited a comment on pull request #19733:
URL: https://github.com/apache/airflow/pull/19733#issuecomment-975110639






-- 
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] uranusjr commented on pull request #19733: Allow session properties for trino connection

Posted by GitBox <gi...@apache.org>.

uranusjr commented on pull request #19733:
URL: https://github.com/apache/airflow/pull/19733#issuecomment-975025586


   LGTM. Could you add a test for this? (Or maybe just modify the existing Trino tests to also check this argument.)


-- 
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] aakashnand commented on pull request #19733: Allow session properties for trino connection

Posted by GitBox <gi...@apache.org>.

aakashnand commented on pull request #19733:
URL: https://github.com/apache/airflow/pull/19733#issuecomment-975071765


   @uranusjr I am actually confused about how to test this argument because this argument is part of the extras argument. Which is kind of covered in this test. Can you help me?
   
   https://github.com/apache/airflow/blob/1b89e682a1a04adf2cf5ebb37a453e245a6e7191/tests/providers/trino/hooks/test_trino.py#L106
   
   cc @danarwix 


-- 
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] github-actions[bot] closed pull request #19733: Allow session properties for trino connection

Posted by GitBox <gi...@apache.org>.

github-actions[bot] closed pull request #19733:
URL: https://github.com/apache/airflow/pull/19733


   


-- 
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 #19733: Allow session properties for trino connection

Posted by GitBox <gi...@apache.org>.

boring-cyborg[bot] commented on pull request #19733:
URL: https://github.com/apache/airflow/pull/19733#issuecomment-974776527

--
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] aakashnand commented on a change in pull request #19733: Allow session properties for trino connection

Posted by GitBox <gi...@apache.org>.

aakashnand commented on a change in pull request #19733:
URL: https://github.com/apache/airflow/pull/19733#discussion_r753910231



##########
File path: airflow/providers/trino/hooks/trino.py
##########
@@ -15,7 +15,7 @@
 # KIND, either express or implied.  See the License for the
 # specific language governing permissions and limitations
 # under the License.
-import os
+import os, ast

Review comment:
       @uranusjr Understood. Sorry, my bad I did not set up pre-commit, I will make changes and it set it up and push again.




-- 
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] uranusjr commented on pull request #19733: Allow session properties for trino connection

Posted by GitBox <gi...@apache.org>.

uranusjr commented on pull request #19733:
URL: https://github.com/apache/airflow/pull/19733#issuecomment-975105455


   What you want to test here is if `trino.dbapi.connect` is called with the correct `session_properties` value. So I’d add a test that’s similar to `test_get_conn_kerberos_auth` but have different `extras` values, and check if `session_properties` is correctly eval-ed and passed to `mock_connect`.


-- 
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