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/08/24 14:51:16 UTC
[GitHub] [airflow] alexandermalyga opened a new pull request, #25939: Fix insert placeholders in TrinoHook
alexandermalyga opened a new pull request, #25939:
URL: https://github.com/apache/airflow/pull/25939
closes: #25937
---
Updated the TrinoHook insert SQL generation to use "?" as the placeholder for parameters.
--
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] eladkal commented on a diff in pull request #25939: Fix insert placeholders in TrinoHook
Posted by GitBox <gi...@apache.org>.
eladkal commented on code in PR #25939:
URL: https://github.com/apache/airflow/pull/25939#discussion_r953930263
##########
airflow/providers/trino/hooks/trino.py:
##########
@@ -251,3 +251,33 @@ def test_connection(self):
message = str(e)
return status, message
+
+ @staticmethod
+ def _generate_insert_sql(table, values, target_fields, replace, **kwargs):
+ """
+ Static helper method that generates the INSERT SQL statement.
+ The REPLACE variant is specific to MySQL syntax.
+
+ :param table: Name of the target table
+ :param values: The row to insert into the table
+ :param target_fields: The names of the columns to fill in the table
+ :param replace: Whether to replace instead of insert
+ :return: The generated INSERT or REPLACE SQL statement
+ :rtype: str
+ """
+ placeholders = [
+ "?",
Review Comment:
This is a lot of code duplication for changing just one char.
Sqlite provider also duplicate this function just to change this one char...
Maybe we can think of a better way to handle 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] eladkal commented on a diff in pull request #25939: Add parametrized placeholder value. Fix placeholders in TrinoHook and PrestoHook.
Posted by GitBox <gi...@apache.org>.
eladkal commented on code in PR #25939:
URL: https://github.com/apache/airflow/pull/25939#discussion_r954663355
##########
airflow/providers/trino/hooks/trino.py:
##########
@@ -251,3 +251,33 @@ def test_connection(self):
message = str(e)
return status, message
+
+ @staticmethod
+ def _generate_insert_sql(table, values, target_fields, replace, **kwargs):
+ """
+ Static helper method that generates the INSERT SQL statement.
+ The REPLACE variant is specific to MySQL syntax.
+
+ :param table: Name of the target table
+ :param values: The row to insert into the table
+ :param target_fields: The names of the columns to fill in the table
+ :param replace: Whether to replace instead of insert
+ :return: The generated INSERT or REPLACE SQL statement
+ :rtype: str
+ """
+ placeholders = [
+ "?",
Review Comment:
Can you also take a look at `_generate_insert_sql` in `PostgresHook`?
I'm not sure there is action there but just to confirm
--
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] alexandermalyga commented on a diff in pull request #25939: Fix insert placeholders in TrinoHook
Posted by GitBox <gi...@apache.org>.
alexandermalyga commented on code in PR #25939:
URL: https://github.com/apache/airflow/pull/25939#discussion_r953933817
##########
airflow/providers/trino/hooks/trino.py:
##########
@@ -251,3 +251,33 @@ def test_connection(self):
message = str(e)
return status, message
+
+ @staticmethod
+ def _generate_insert_sql(table, values, target_fields, replace, **kwargs):
+ """
+ Static helper method that generates the INSERT SQL statement.
+ The REPLACE variant is specific to MySQL syntax.
+
+ :param table: Name of the target table
+ :param values: The row to insert into the table
+ :param target_fields: The names of the columns to fill in the table
+ :param replace: Whether to replace instead of insert
+ :return: The generated INSERT or REPLACE SQL statement
+ :rtype: str
+ """
+ placeholders = [
+ "?",
Review Comment:
We could add a `placeholder` parameter to `DbApiHook._generate_insert_sql()` function, which would be passed from `**kwargs` in `DbApiHook.insert_rows()`. The parameter could have a default value of `"%s"`, and other `insert_rows()` implementations could pass another string, `"?"` in this case.
--
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 #25939: Fix placeholders in `TrinoHook`, `PrestoHook`, `SqliteHook`
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #25939:
URL: https://github.com/apache/airflow/pull/25939#issuecomment-1227170878
This is cool. I believe you will also have to bump common-sql provider to 1.2.0 for it in order to make CI pass.
--
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 #25939: Fix insert placeholders in TrinoHook
Posted by GitBox <gi...@apache.org>.
boring-cyborg[bot] commented on PR #25939:
URL: https://github.com/apache/airflow/pull/25939#issuecomment-1225834404
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] potiuk commented on a diff in pull request #25939: Fix insert placeholders in TrinoHook
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #25939:
URL: https://github.com/apache/airflow/pull/25939#discussion_r953958410
##########
airflow/providers/trino/hooks/trino.py:
##########
@@ -251,3 +251,33 @@ def test_connection(self):
message = str(e)
return status, message
+
+ @staticmethod
+ def _generate_insert_sql(table, values, target_fields, replace, **kwargs):
+ """
+ Static helper method that generates the INSERT SQL statement.
+ The REPLACE variant is specific to MySQL syntax.
+
+ :param table: Name of the target table
+ :param values: The row to insert into the table
+ :param target_fields: The names of the columns to fill in the table
+ :param replace: Whether to replace instead of insert
+ :return: The generated INSERT or REPLACE SQL statement
+ :rtype: str
+ """
+ placeholders = [
+ "?",
Review Comment:
Yep. This is better. Especially that now we have common.sql provider and such change can be released together (and we can add `common-sql>=1.2.0` as limit :D.
One of the benefits of having the `common.sql` package which is independent from airlfow.
--
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 diff in pull request #25939: Add parametrized placeholder value. Fix placeholders in TrinoHook and PrestoHook.
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #25939:
URL: https://github.com/apache/airflow/pull/25939#discussion_r954666011
##########
airflow/providers/common/sql/hooks/sql.py:
##########
@@ -348,7 +348,7 @@ def get_cursor(self):
return self.get_conn().cursor()
@staticmethod
- def _generate_insert_sql(table, values, target_fields, replace, **kwargs):
+ def _generate_insert_sql(table, values, target_fields, replace, placeholder="%s", **kwargs):
Review Comment:
Perhaps this should be a class attribute instead? It’s hard to imagine a SQL backend would need to support multiple placeholder types, and we will probably need to do the same for e.g. `UPDATE` as well.
--
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] eladkal commented on a diff in pull request #25939: Add parametrized placeholder value. Fix placeholders in TrinoHook and PrestoHook.
Posted by GitBox <gi...@apache.org>.
eladkal commented on code in PR #25939:
URL: https://github.com/apache/airflow/pull/25939#discussion_r954738188
##########
airflow/providers/trino/hooks/trino.py:
##########
@@ -251,3 +251,33 @@ def test_connection(self):
message = str(e)
return status, message
+
+ @staticmethod
+ def _generate_insert_sql(table, values, target_fields, replace, **kwargs):
+ """
+ Static helper method that generates the INSERT SQL statement.
+ The REPLACE variant is specific to MySQL syntax.
+
+ :param table: Name of the target table
+ :param values: The row to insert into the table
+ :param target_fields: The names of the columns to fill in the table
+ :param replace: Whether to replace instead of insert
+ :return: The generated INSERT or REPLACE SQL statement
+ :rtype: str
+ """
+ placeholders = [
+ "?",
Review Comment:
thanks
Note to the comment in https://github.com/apache/airflow/pull/25939#discussion_r953958410
By adding this change the effected providers are depended on a new release of common-sql
so we should edit the provider.yml :
https://github.com/apache/airflow/blob/e996a88c7b19a1d30c529f5dd126d0a8871f5ce0/airflow/providers/trino/provider.yaml#L40
and set the dependency as `>=1.2.0`
(1.2.0 will be the next release of common.sql that include this PR)
Otherwise we are risking users installing the new version of Trino provider with older version of common.sql provider which will result in code not working
(Same goes for the other effected providers)
--
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] alexandermalyga commented on pull request #25939: Fix placeholders in `TrinoHook`, `PrestoHook`, `SqliteHook`
Posted by GitBox <gi...@apache.org>.
alexandermalyga commented on PR #25939:
URL: https://github.com/apache/airflow/pull/25939#issuecomment-1227393802
Any idea on why that one CI check has failed?
--
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 a diff in pull request #25939: Fix insert placeholders in TrinoHook
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #25939:
URL: https://github.com/apache/airflow/pull/25939#discussion_r953958410
##########
airflow/providers/trino/hooks/trino.py:
##########
@@ -251,3 +251,33 @@ def test_connection(self):
message = str(e)
return status, message
+
+ @staticmethod
+ def _generate_insert_sql(table, values, target_fields, replace, **kwargs):
+ """
+ Static helper method that generates the INSERT SQL statement.
+ The REPLACE variant is specific to MySQL syntax.
+
+ :param table: Name of the target table
+ :param values: The row to insert into the table
+ :param target_fields: The names of the columns to fill in the table
+ :param replace: Whether to replace instead of insert
+ :return: The generated INSERT or REPLACE SQL statement
+ :rtype: str
+ """
+ placeholders = [
+ "?",
Review Comment:
Yep. This is better. Especially that now we have common.sql provider and such change can be released together (and we can add `common-sql-provider>1.2.0` as limit :D.
One of the benefits of having the `common.sql` package which is independent from airlfow.
--
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] eladkal commented on a diff in pull request #25939: Add parametrized placeholder value. Fix placeholders in TrinoHook and PrestoHook.
Posted by GitBox <gi...@apache.org>.
eladkal commented on code in PR #25939:
URL: https://github.com/apache/airflow/pull/25939#discussion_r954738188
##########
airflow/providers/trino/hooks/trino.py:
##########
@@ -251,3 +251,33 @@ def test_connection(self):
message = str(e)
return status, message
+
+ @staticmethod
+ def _generate_insert_sql(table, values, target_fields, replace, **kwargs):
+ """
+ Static helper method that generates the INSERT SQL statement.
+ The REPLACE variant is specific to MySQL syntax.
+
+ :param table: Name of the target table
+ :param values: The row to insert into the table
+ :param target_fields: The names of the columns to fill in the table
+ :param replace: Whether to replace instead of insert
+ :return: The generated INSERT or REPLACE SQL statement
+ :rtype: str
+ """
+ placeholders = [
+ "?",
Review Comment:
thanks
Note to the comment in https://github.com/apache/airflow/pull/25939#discussion_r953958410
By adding this change the effected providers are depended on a new release of common-sql
so we should edit the provider.yml :
https://github.com/apache/airflow/blob/e996a88c7b19a1d30c529f5dd126d0a8871f5ce0/airflow/providers/trino/provider.yaml#L40
and set the dependency as `>=1.2.0`
Otherwise we are risking users installing the new version of Trino provider with older version of common.sql provider which will result in code not working
(Same goes for the other effected providers)
--
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] eladkal commented on a diff in pull request #25939: Fix insert placeholders in TrinoHook
Posted by GitBox <gi...@apache.org>.
eladkal commented on code in PR #25939:
URL: https://github.com/apache/airflow/pull/25939#discussion_r953994791
##########
airflow/providers/trino/hooks/trino.py:
##########
@@ -251,3 +251,33 @@ def test_connection(self):
message = str(e)
return status, message
+
+ @staticmethod
+ def _generate_insert_sql(table, values, target_fields, replace, **kwargs):
+ """
+ Static helper method that generates the INSERT SQL statement.
+ The REPLACE variant is specific to MySQL syntax.
+
+ :param table: Name of the target table
+ :param values: The row to insert into the table
+ :param target_fields: The names of the columns to fill in the table
+ :param replace: Whether to replace instead of insert
+ :return: The generated INSERT or REPLACE SQL statement
+ :rtype: str
+ """
+ placeholders = [
+ "?",
Review Comment:
Sure you can (just make sure commit message and PR title/description) reflect this.
Unit tests would be nice too (if not covered already) as it will help to avoid regression
BTW can you check also with PrestoHook?
Usualy issues with Trino apply also to presto
--
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 #25939: Fix placeholders in `TrinoHook`, `PrestoHook`, `SqliteHook`
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #25939:
URL: https://github.com/apache/airflow/pull/25939#issuecomment-1227416590
Random GA glitch. retarted 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] potiuk commented on a diff in pull request #25939: Fix placeholders in `TrinoHook`, `PrestoHook`, `SqliteHook`
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #25939:
URL: https://github.com/apache/airflow/pull/25939#discussion_r961093775
##########
airflow/providers/common/sql/provider.yaml:
##########
@@ -22,6 +22,7 @@ description: |
`Common SQL Provider <https://en.wikipedia.org/wiki/SQL>`__
versions:
+ - 1.2.0
Review Comment:
No. It's fine. There is new feature (placeholder) added to common.sql . The bugfix bumps should be in trino, presto, sqlite, but common-sql has a new feature added and those bugfix versions of trino/presto/sqlite should depend on this new feature. All looks fine here.
--
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 #25939: Fix placeholders in `TrinoHook`, `PrestoHook`, `SqliteHook`
Posted by GitBox <gi...@apache.org>.
Taragolis commented on code in PR #25939:
URL: https://github.com/apache/airflow/pull/25939#discussion_r956027518
##########
airflow/providers/common/sql/hooks/sql.py:
##########
@@ -109,6 +109,8 @@ class DbApiHook(BaseForDbApiHook):
connector = None # type: Optional[ConnectorProtocol]
# Override with db-specific query to check connection
_test_connection_sql = "select 1"
+ # Override with the db-specific value used for placeholders
+ placeholder: str = "%s"
Review Comment:
Is it reference to [mandatory DB-API 2 paramstyle property](https://peps.python.org/pep-0249/#paramstyle)?
```python
Python 3.9.10 (main, Feb 25 2022, 16:54:01)
Type 'copyright', 'credits' or 'license' for more information
IPython 8.4.0 -- An enhanced Interactive Python. Type '?' for help.
PyDev console: using IPython 8.4.0
Python 3.9.10 (main, Feb 25 2022, 16:54:01)
[Clang 13.0.0 (clang-1300.0.29.30)] on darwin
import sqlite3, trino, prestodb, psycopg2
sqlite3.paramstyle
Out[3]: 'qmark'
trino.dbapi.paramstyle
Out[4]: 'qmark'
psycopg2.paramstyle
Out[5]: 'pyformat'
prestodb.dbapi.paramstyle
Traceback (most recent call last):
File "/Users/taragolis/.pyenv/versions/airflow-dev-env-39/lib/python3.9/site-packages/IPython/core/interactiveshell.py", line 3398, in run_code
exec(code_obj, self.user_global_ns, self.user_ns)
File "<ipython-input-6-cb48c8aec499>", line 1, in <cell line: 1>
prestodb.dbapi.paramstyle
AttributeError: module 'prestodb.dbapi' has no attribute 'paramstyle'
```
--
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] alexandermalyga commented on a diff in pull request #25939: Add parametrized placeholder value. Fix placeholders in TrinoHook and PrestoHook.
Posted by GitBox <gi...@apache.org>.
alexandermalyga commented on code in PR #25939:
URL: https://github.com/apache/airflow/pull/25939#discussion_r954693897
##########
airflow/providers/common/sql/hooks/sql.py:
##########
@@ -348,7 +348,7 @@ def get_cursor(self):
return self.get_conn().cursor()
@staticmethod
- def _generate_insert_sql(table, values, target_fields, replace, **kwargs):
+ def _generate_insert_sql(table, values, target_fields, replace, placeholder="%s", **kwargs):
Review Comment:
Makes sense, and it would be the same behaviour as with other backend-specific parameters.
--
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] alexandermalyga commented on a diff in pull request #25939: Fix insert placeholders in TrinoHook
Posted by GitBox <gi...@apache.org>.
alexandermalyga commented on code in PR #25939:
URL: https://github.com/apache/airflow/pull/25939#discussion_r953988097
##########
airflow/providers/trino/hooks/trino.py:
##########
@@ -251,3 +251,33 @@ def test_connection(self):
message = str(e)
return status, message
+
+ @staticmethod
+ def _generate_insert_sql(table, values, target_fields, replace, **kwargs):
+ """
+ Static helper method that generates the INSERT SQL statement.
+ The REPLACE variant is specific to MySQL syntax.
+
+ :param table: Name of the target table
+ :param values: The row to insert into the table
+ :param target_fields: The names of the columns to fill in the table
+ :param replace: Whether to replace instead of insert
+ :return: The generated INSERT or REPLACE SQL statement
+ :rtype: str
+ """
+ placeholders = [
+ "?",
Review Comment:
Just committed the discussed functionality. Should I also modify the SQLite implementation in this PR?
--
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] eladkal commented on a diff in pull request #25939: Fix insert placeholders in TrinoHook
Posted by GitBox <gi...@apache.org>.
eladkal commented on code in PR #25939:
URL: https://github.com/apache/airflow/pull/25939#discussion_r953994791
##########
airflow/providers/trino/hooks/trino.py:
##########
@@ -251,3 +251,33 @@ def test_connection(self):
message = str(e)
return status, message
+
+ @staticmethod
+ def _generate_insert_sql(table, values, target_fields, replace, **kwargs):
+ """
+ Static helper method that generates the INSERT SQL statement.
+ The REPLACE variant is specific to MySQL syntax.
+
+ :param table: Name of the target table
+ :param values: The row to insert into the table
+ :param target_fields: The names of the columns to fill in the table
+ :param replace: Whether to replace instead of insert
+ :return: The generated INSERT or REPLACE SQL statement
+ :rtype: str
+ """
+ placeholders = [
+ "?",
Review Comment:
Sure you can (just make sure commit message and PR title/description) reflect this.
BTW can you check also with PrestoHook?
Usualy issues with Trino apply also to presto
--
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] eladkal commented on a diff in pull request #25939: Add parametrized placeholder value. Fix placeholders in TrinoHook and PrestoHook.
Posted by GitBox <gi...@apache.org>.
eladkal commented on code in PR #25939:
URL: https://github.com/apache/airflow/pull/25939#discussion_r954777518
##########
airflow/providers/trino/hooks/trino.py:
##########
@@ -251,3 +251,33 @@ def test_connection(self):
message = str(e)
return status, message
+
+ @staticmethod
+ def _generate_insert_sql(table, values, target_fields, replace, **kwargs):
+ """
+ Static helper method that generates the INSERT SQL statement.
+ The REPLACE variant is specific to MySQL syntax.
+
+ :param table: Name of the target table
+ :param values: The row to insert into the table
+ :param target_fields: The names of the columns to fill in the table
+ :param replace: Whether to replace instead of insert
+ :return: The generated INSERT or REPLACE SQL statement
+ :rtype: str
+ """
+ placeholders = [
+ "?",
Review Comment:
it will be the next release. providers are released together so it's fine
--
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 diff in pull request #25939: Fix placeholders in `TrinoHook`, `PrestoHook`, `SqliteHook`
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #25939:
URL: https://github.com/apache/airflow/pull/25939#discussion_r954850614
##########
airflow/providers/common/sql/hooks/sql.py:
##########
@@ -109,6 +109,8 @@ class DbApiHook(BaseForDbApiHook):
connector = None # type: Optional[ConnectorProtocol]
# Override with db-specific query to check connection
_test_connection_sql = "select 1"
+ # Override with the db-specific value used for placeholders
+ placeholder = "%s" # type: str
Review Comment:
```suggestion
placeholder: str = "%s"
```
--
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] alexandermalyga commented on a diff in pull request #25939: Fix insert placeholders in TrinoHook
Posted by GitBox <gi...@apache.org>.
alexandermalyga commented on code in PR #25939:
URL: https://github.com/apache/airflow/pull/25939#discussion_r953933817
##########
airflow/providers/trino/hooks/trino.py:
##########
@@ -251,3 +251,33 @@ def test_connection(self):
message = str(e)
return status, message
+
+ @staticmethod
+ def _generate_insert_sql(table, values, target_fields, replace, **kwargs):
+ """
+ Static helper method that generates the INSERT SQL statement.
+ The REPLACE variant is specific to MySQL syntax.
+
+ :param table: Name of the target table
+ :param values: The row to insert into the table
+ :param target_fields: The names of the columns to fill in the table
+ :param replace: Whether to replace instead of insert
+ :return: The generated INSERT or REPLACE SQL statement
+ :rtype: str
+ """
+ placeholders = [
+ "?",
Review Comment:
We could add a `placeholder` parameter to `DbApiHook._generate_insert_sql()` function, which would be passed from `**kwargs` in `DbApiHook.insert_rows()`. The parameter could have a default value of `"%s"`, and other `insert_rows()` could pass another string, `"?"` in this case.
--
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 #25939: Fix placeholders in `TrinoHook`, `PrestoHook`, `SqliteHook`
Posted by GitBox <gi...@apache.org>.
potiuk merged PR #25939:
URL: https://github.com/apache/airflow/pull/25939
--
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 a diff in pull request #25939: Fix placeholders in `TrinoHook`, `PrestoHook`, `SqliteHook`
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #25939:
URL: https://github.com/apache/airflow/pull/25939#discussion_r961093775
##########
airflow/providers/common/sql/provider.yaml:
##########
@@ -22,6 +22,7 @@ description: |
`Common SQL Provider <https://en.wikipedia.org/wiki/SQL>`__
versions:
+ - 1.2.0
Review Comment:
No. It's fine. There is new feature (placeholder) added to common.sql . The bugfix bumps should be in trino, presto, sqlite, but common-sql has a new feature added (and those bugfix versions of trino/presto/sqlite should depend on this new feature). All looks fine here.
--
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 #25939: Fix placeholders in `TrinoHook`, `PrestoHook`, `SqliteHook`
Posted by GitBox <gi...@apache.org>.
boring-cyborg[bot] commented on PR #25939:
URL: https://github.com/apache/airflow/pull/25939#issuecomment-1229086616
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] alexandermalyga commented on a diff in pull request #25939: Add parametrized placeholder value. Fix placeholders in TrinoHook and PrestoHook.
Posted by GitBox <gi...@apache.org>.
alexandermalyga commented on code in PR #25939:
URL: https://github.com/apache/airflow/pull/25939#discussion_r954670368
##########
airflow/providers/trino/hooks/trino.py:
##########
@@ -251,3 +251,33 @@ def test_connection(self):
message = str(e)
return status, message
+
+ @staticmethod
+ def _generate_insert_sql(table, values, target_fields, replace, **kwargs):
+ """
+ Static helper method that generates the INSERT SQL statement.
+ The REPLACE variant is specific to MySQL syntax.
+
+ :param table: Name of the target table
+ :param values: The row to insert into the table
+ :param target_fields: The names of the columns to fill in the table
+ :param replace: Whether to replace instead of insert
+ :return: The generated INSERT or REPLACE SQL statement
+ :rtype: str
+ """
+ placeholders = [
+ "?",
Review Comment:
Yep, just had a look and it has Postgres-specific SQL generation, and the placeholder used is `"%s"` which is [correct for psycopg2](https://www.psycopg.org/docs/usage.html#passing-parameters-to-sql-queries)
--
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] alexandermalyga commented on a diff in pull request #25939: Fix insert placeholders in TrinoHook
Posted by GitBox <gi...@apache.org>.
alexandermalyga commented on code in PR #25939:
URL: https://github.com/apache/airflow/pull/25939#discussion_r953933817
##########
airflow/providers/trino/hooks/trino.py:
##########
@@ -251,3 +251,33 @@ def test_connection(self):
message = str(e)
return status, message
+
+ @staticmethod
+ def _generate_insert_sql(table, values, target_fields, replace, **kwargs):
+ """
+ Static helper method that generates the INSERT SQL statement.
+ The REPLACE variant is specific to MySQL syntax.
+
+ :param table: Name of the target table
+ :param values: The row to insert into the table
+ :param target_fields: The names of the columns to fill in the table
+ :param replace: Whether to replace instead of insert
+ :return: The generated INSERT or REPLACE SQL statement
+ :rtype: str
+ """
+ placeholders = [
+ "?",
Review Comment:
We could add a `placeholder` parameter to `DbApiHook._generate_insert_sql()` function, which would be passed from `**kwargs` in `DbApiHook.insert_rows()`. The parameter could have a default value of "%s".
--
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] alexandermalyga commented on a diff in pull request #25939: Add parametrized placeholder value. Fix placeholders in TrinoHook and PrestoHook.
Posted by GitBox <gi...@apache.org>.
alexandermalyga commented on code in PR #25939:
URL: https://github.com/apache/airflow/pull/25939#discussion_r954768716
##########
airflow/providers/trino/hooks/trino.py:
##########
@@ -251,3 +251,33 @@ def test_connection(self):
message = str(e)
return status, message
+
+ @staticmethod
+ def _generate_insert_sql(table, values, target_fields, replace, **kwargs):
+ """
+ Static helper method that generates the INSERT SQL statement.
+ The REPLACE variant is specific to MySQL syntax.
+
+ :param table: Name of the target table
+ :param values: The row to insert into the table
+ :param target_fields: The names of the columns to fill in the table
+ :param replace: Whether to replace instead of insert
+ :return: The generated INSERT or REPLACE SQL statement
+ :rtype: str
+ """
+ placeholders = [
+ "?",
Review Comment:
When should we expect 1.2.0 to be released?
--
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 #25939: Fix placeholders in `TrinoHook`, `PrestoHook`, `SqliteHook`
Posted by GitBox <gi...@apache.org>.
ashb commented on code in PR #25939:
URL: https://github.com/apache/airflow/pull/25939#discussion_r961064767
##########
airflow/providers/common/sql/provider.yaml:
##########
@@ -22,6 +22,7 @@ description: |
`Common SQL Provider <https://en.wikipedia.org/wiki/SQL>`__
versions:
+ - 1.2.0
Review Comment:
@potiuk This feels like it should be a 1.1.1 -- i.e. a bug fix release?
--
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