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/09/28 15:59:01 UTC
[GitHub] [airflow] flolas opened a new pull request #17329: Split sql statements in DbApi run
flolas opened a new pull request #17329:
URL: https://github.com/apache/airflow/pull/17329
Split sql statements in dbapi hook using sqlparse.
closes: #16979
--
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] flolas commented on a change in pull request #17329: Split sql statements in DbApi run
Posted by GitBox <gi...@apache.org>.
flolas commented on a change in pull request #17329:
URL: https://github.com/apache/airflow/pull/17329#discussion_r681163379
##########
File path: airflow/hooks/dbapi.py
##########
@@ -176,18 +178,32 @@ def run(self, sql, autocommit=False, parameters=None, handler=None):
:param handler: The result handler which is called with the result of each statement.
:type handler: callable
:return: query results if handler was provided.
+ :param split_statements: If true, split sql statements
+ :type split_statements: bool
"""
scalar = isinstance(sql, str)
if scalar:
sql = [sql]
+ sql_statements = sql
+
+ if split_statements:
+ sql_statements = []
+ for q in sql:
+ sql_statements.append(
+ [
+ s[:-1] if s[-1] == ';' else s
Review comment:
@uranusjr Agree, I saw that in drill and presto, I assumed that it's neccesary, but as you said, most sql engine handles ;
--
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] flolas commented on pull request #17329: Split sql statements in DbApi run
Posted by GitBox <gi...@apache.org>.
flolas commented on pull request #17329:
URL: https://github.com/apache/airflow/pull/17329#issuecomment-939052887
@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
[GitHub] [airflow] potiuk closed pull request #17329: Split sql statements in DbApi run
Posted by GitBox <gi...@apache.org>.
potiuk closed pull request #17329:
URL: https://github.com/apache/airflow/pull/17329
--
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] flolas commented on a change in pull request #17329: Split sql statements in DbApi run
Posted by GitBox <gi...@apache.org>.
flolas commented on a change in pull request #17329:
URL: https://github.com/apache/airflow/pull/17329#discussion_r702888607
##########
File path: tests/hooks/test_dbapi.py
##########
@@ -232,3 +233,74 @@ def handler(cur):
assert called == 2
assert self.conn.commit.called
assert result == [obj, obj]
+
+ @patch('airflow.hooks.dbapi.DbApiHook._run_command')
+ def test_run_with_multiple_statements_and_split(self, _run_command):
+ _cases = [
+ (['SQL1; SQL2;', 'SQL3'], 3),
+ (['SQL1; SQL2;', 'SQL3;'], 3),
+ (['SQL1; SQL2; SQL3;'], 3),
+ ('SQL1; SQL2; SQL3;', 3),
+ (['SQL1;', 'SQL2'], 2),
+ (['SQL1;', 'SQL2;'], 2),
+ (['SQL1; SQL2;'], 2),
+ ('SQL1; SQL2;', 2),
+ (['SQL1'], 1),
+ (['SQL1;'], 1),
+ ('SQL1;', 1),
+ ('SQL1', 1),
+ (
+ """
+ CREATE OR REPLACE FUNCTION dfp.extract_vcode(NAME VARCHAR)
+ RETURNS string
+ LANGUAGE javascript
+ STRICT
+ AS '
+ const regex = /[[]v=([0-9]+)/ig;
+ let s = NAME.match(regex);
+ if (s != null) {
+ return s[0].split('=')[1];
+ } else {
+ return null;
+ }
+ ';
+ CREATE OR REPLACE FUNCTION dfp.parse_metadata(DATA varchar)
+ RETURNS OBJECT
+ LANGUAGE javascript
+ STRICT
+ AS '
+ if (!DATA) {
+ return {}
+ }
+
+ let dict = {}
+ const parts = DATA.split("|")
+
+ parts.forEach((p) => {
+ const split = p.split("=")
+ dict[split[0]] = split[1]
+ })
+
+ return dict
+ ';
+ """, # nopep8
+ 2,
+ ),
+ (
+ """
+ SELECT
+ *
+ FROM country
+ LEFT JOIN city ON city.country_id = country.id
+ LEFT JOIN customer ON city.id = customer.city_id
+ LEFT JOIN call ON call.customer_id = customer.id;
+ SELECT country, count(*) FROM country
+ GROUP BY 1;
+ """, # nopep8
Review comment:
1) because the static checks fails without that because of E501 (long multiline string), if there is a better way, im open to change it.
2) I think that is a good improvement for the test. Will do at the end of the day.
--
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] flolas commented on a change in pull request #17329: Split sql statements in DbApi run
Posted by GitBox <gi...@apache.org>.
flolas commented on a change in pull request #17329:
URL: https://github.com/apache/airflow/pull/17329#discussion_r702888607
##########
File path: tests/hooks/test_dbapi.py
##########
@@ -232,3 +233,74 @@ def handler(cur):
assert called == 2
assert self.conn.commit.called
assert result == [obj, obj]
+
+ @patch('airflow.hooks.dbapi.DbApiHook._run_command')
+ def test_run_with_multiple_statements_and_split(self, _run_command):
+ _cases = [
+ (['SQL1; SQL2;', 'SQL3'], 3),
+ (['SQL1; SQL2;', 'SQL3;'], 3),
+ (['SQL1; SQL2; SQL3;'], 3),
+ ('SQL1; SQL2; SQL3;', 3),
+ (['SQL1;', 'SQL2'], 2),
+ (['SQL1;', 'SQL2;'], 2),
+ (['SQL1; SQL2;'], 2),
+ ('SQL1; SQL2;', 2),
+ (['SQL1'], 1),
+ (['SQL1;'], 1),
+ ('SQL1;', 1),
+ ('SQL1', 1),
+ (
+ """
+ CREATE OR REPLACE FUNCTION dfp.extract_vcode(NAME VARCHAR)
+ RETURNS string
+ LANGUAGE javascript
+ STRICT
+ AS '
+ const regex = /[[]v=([0-9]+)/ig;
+ let s = NAME.match(regex);
+ if (s != null) {
+ return s[0].split('=')[1];
+ } else {
+ return null;
+ }
+ ';
+ CREATE OR REPLACE FUNCTION dfp.parse_metadata(DATA varchar)
+ RETURNS OBJECT
+ LANGUAGE javascript
+ STRICT
+ AS '
+ if (!DATA) {
+ return {}
+ }
+
+ let dict = {}
+ const parts = DATA.split("|")
+
+ parts.forEach((p) => {
+ const split = p.split("=")
+ dict[split[0]] = split[1]
+ })
+
+ return dict
+ ';
+ """, # nopep8
+ 2,
+ ),
+ (
+ """
+ SELECT
+ *
+ FROM country
+ LEFT JOIN city ON city.country_id = country.id
+ LEFT JOIN customer ON city.id = customer.city_id
+ LEFT JOIN call ON call.customer_id = customer.id;
+ SELECT country, count(*) FROM country
+ GROUP BY 1;
+ """, # nopep8
Review comment:
1) the static checks fails because of E501 (long multiline string), if there is a better way, im open to change it.
2) I think that is a good improvement for the test. Will do at the end of the day.
--
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 #17329: Split sql statements in DbApi run
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #17329:
URL: https://github.com/apache/airflow/pull/17329#issuecomment-985101679
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] potiuk commented on a change in pull request #17329: Split sql statements in DbApi run
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #17329:
URL: https://github.com/apache/airflow/pull/17329#discussion_r725461128
##########
File path: setup.py
##########
@@ -260,7 +260,7 @@ def write_version(filename: str = os.path.join(*[my_dir, "airflow", "git_version
docker = [
'docker',
]
-drill = ['sqlalchemy-drill>=1.1.0', 'sqlparse>=0.4.1']
Review comment:
This should remain here. Drill should have sqlparse as dependency regardless whether it is also a core dependency, because we can still install new Drill provider on older airflow version that will have no sqlparse as core dependency.
--
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 change in pull request #17329: Split sql statements in DbApi run
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #17329:
URL: https://github.com/apache/airflow/pull/17329#discussion_r725461348
##########
File path: setup.cfg
##########
@@ -150,6 +150,7 @@ install_requires =
setproctitle>=1.1.8, <2
sqlalchemy>=1.3.18
sqlalchemy_jsonfield~=1.0
+ sqlparse>=0.4.1, <1
Review comment:
We should not limit <1.
We are trying to not limit the dependencies with upper bound unless we have very good reason to do so. Our constraint mechanism makes sure we are not going to get incompatible version, and thanks to that we are much more flexible with airflow and if it turns out that newer version still works, users won't be limited.
--
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 #17329: Split sql statements in DbApi run
Posted by GitBox <gi...@apache.org>.
uranusjr commented on pull request #17329:
URL: https://github.com/apache/airflow/pull/17329#issuecomment-988513575
@flolas Do you intend to pick this back up?
--
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] flolas commented on pull request #17329: Split sql statements in DbApi run
Posted by GitBox <gi...@apache.org>.
flolas commented on pull request #17329:
URL: https://github.com/apache/airflow/pull/17329#issuecomment-891368646
> Some static checks are failing
fix'd
--
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] flolas edited a comment on pull request #17329: Split sql statements in DbApi run
Posted by GitBox <gi...@apache.org>.
flolas edited a comment on pull request #17329:
URL: https://github.com/apache/airflow/pull/17329#issuecomment-893071375
> sqlparse is not a core dependency -- it needs adding.
Hi @ashb, what do you mean with "it needs adding"? I added sqlparse to setup.cfg. We need to do anything else?
note: I'm not familiar with all "core" things of Airflow
thx
--
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 closed pull request #17329: Split sql statements in DbApi run
Posted by GitBox <gi...@apache.org>.
potiuk closed pull request #17329:
URL: https://github.com/apache/airflow/pull/17329
--
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] flolas commented on a change in pull request #17329: Split sql statements in DbApi run
Posted by GitBox <gi...@apache.org>.
flolas commented on a change in pull request #17329:
URL: https://github.com/apache/airflow/pull/17329#discussion_r716203117
##########
File path: airflow/hooks/dbapi.py
##########
@@ -259,6 +266,27 @@ def get_cursor(self):
"""Returns a cursor"""
return self.get_conn().cursor()
+ @staticmethod
+ def _split_sql_statements(sql, strip_comments=True, strip_semicolon=True, **kwargs):
+ """
+ Split multiple sql statements in string
+
+ :param sql: sql statements.
+ :type sql: str or list.
+ :return: splitted sql statements.
+ :rtype: list.
+ """
+ if isinstance(sql, str):
+ sql = [sql]
+
+ return [
+ s.rstrip(';') if strip_semicolon else s
Review comment:
Yes, its because what @potiuk said (this is valid for Apache Drill too)
--
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] flolas edited a comment on pull request #17329: Split sql statements in DbApi run
Posted by GitBox <gi...@apache.org>.
flolas edited a comment on pull request #17329:
URL: https://github.com/apache/airflow/pull/17329#issuecomment-928259116
@potiuk Tests fail :( whats wrong?
--
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 #17329: Split sql statements in DbApi run
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #17329:
URL: https://github.com/apache/airflow/pull/17329#issuecomment-891305805
Some static checks are failing
--
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 closed pull request #17329: Split sql statements in DbApi run
Posted by GitBox <gi...@apache.org>.
uranusjr closed pull request #17329:
URL: https://github.com/apache/airflow/pull/17329
--
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] flolas commented on pull request #17329: Split sql statements in DbApi run
Posted by GitBox <gi...@apache.org>.
flolas commented on pull request #17329:
URL: https://github.com/apache/airflow/pull/17329#issuecomment-893071375
> sqlparse is not a core dependency -- it needs adding.
Hi @ashb, what do you mean with "it needs adding"? I added sqlparse to setup.cfg. We need to do anything else?
thx
--
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 #17329: Split sql statements in DbApi run
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #17329:
URL: https://github.com/apache/airflow/pull/17329#discussion_r703180280
##########
File path: tests/hooks/test_dbapi.py
##########
@@ -232,3 +233,74 @@ def handler(cur):
assert called == 2
assert self.conn.commit.called
assert result == [obj, obj]
+
+ @patch('airflow.hooks.dbapi.DbApiHook._run_command')
+ def test_run_with_multiple_statements_and_split(self, _run_command):
+ _cases = [
+ (['SQL1; SQL2;', 'SQL3'], 3),
+ (['SQL1; SQL2;', 'SQL3;'], 3),
+ (['SQL1; SQL2; SQL3;'], 3),
+ ('SQL1; SQL2; SQL3;', 3),
+ (['SQL1;', 'SQL2'], 2),
+ (['SQL1;', 'SQL2;'], 2),
+ (['SQL1; SQL2;'], 2),
+ ('SQL1; SQL2;', 2),
+ (['SQL1'], 1),
+ (['SQL1;'], 1),
+ ('SQL1;', 1),
+ ('SQL1', 1),
+ (
+ """
+ CREATE OR REPLACE FUNCTION dfp.extract_vcode(NAME VARCHAR)
+ RETURNS string
+ LANGUAGE javascript
+ STRICT
+ AS '
+ const regex = /[[]v=([0-9]+)/ig;
+ let s = NAME.match(regex);
+ if (s != null) {
+ return s[0].split('=')[1];
+ } else {
+ return null;
+ }
+ ';
+ CREATE OR REPLACE FUNCTION dfp.parse_metadata(DATA varchar)
+ RETURNS OBJECT
+ LANGUAGE javascript
+ STRICT
+ AS '
+ if (!DATA) {
+ return {}
+ }
+
+ let dict = {}
+ const parts = DATA.split("|")
+
+ parts.forEach((p) => {
+ const split = p.split("=")
+ dict[split[0]] = split[1]
+ })
+
+ return dict
+ ';
+ """, # nopep8
+ 2,
+ ),
+ (
+ """
+ SELECT
+ *
+ FROM country
+ LEFT JOIN city ON city.country_id = country.id
+ LEFT JOIN customer ON city.id = customer.city_id
+ LEFT JOIN call ON call.customer_id = customer.id;
+ SELECT country, count(*) FROM country
+ GROUP BY 1;
+ """, # nopep8
Review comment:
This sounds wrong. Airflow sets the line length to 110 and this is nowhere near that.
https://github.com/apache/airflow/blob/main/.flake8
Is this reported by pre-commit? Or maybe your editor is not picking up the configuration correctly?
--
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] flolas commented on pull request #17329: Split sql statements in DbApi run
Posted by GitBox <gi...@apache.org>.
flolas commented on pull request #17329:
URL: https://github.com/apache/airflow/pull/17329#issuecomment-931708300
@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
[GitHub] [airflow] flolas commented on pull request #17329: Split sql statements in DbApi run
Posted by GitBox <gi...@apache.org>.
flolas commented on pull request #17329:
URL: https://github.com/apache/airflow/pull/17329#issuecomment-927196352
@uranusjr @potiuk Hi, i did some changes in the code for addressing last review.
1) Made a static method for the splitting part (better testing)
2) Default strip ; after query (drill does not supports ; after sentence)
3) added new test cases
--
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 change in pull request #17329: Split sql statements in DbApi run
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #17329:
URL: https://github.com/apache/airflow/pull/17329#discussion_r716130509
##########
File path: airflow/hooks/dbapi.py
##########
@@ -259,6 +266,27 @@ def get_cursor(self):
"""Returns a cursor"""
return self.get_conn().cursor()
+ @staticmethod
+ def _split_sql_statements(sql, strip_comments=True, strip_semicolon=True, **kwargs):
+ """
+ Split multiple sql statements in string
+
+ :param sql: sql statements.
+ :type sql: str or list.
+ :return: splitted sql statements.
+ :rtype: list.
+ """
+ if isinstance(sql, str):
+ sql = [sql]
+
+ return [
+ s.rstrip(';') if strip_semicolon else s
Review comment:
I believe there are certain engines, that actually will complain if `;` is passed. MsSQL particularly I believe.
I just learned couple of days that when you use CLI (via ODBC) to MsSQL, `;` at the end of SQL query does not make it execute - it will complain. Instead wheat you need to do is to type `GO` in the next line š±
--
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 #17329: Split sql statements in DbApi run
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #17329:
URL: https://github.com/apache/airflow/pull/17329#issuecomment-893215329
> > sqlparse is not a core dependency -- it needs adding.
>
> Hi @ashb, what do you mean with "it needs adding"? I added sqlparse to setup.cfg. We need to do anything else?
> note: I'm not familiar with all "core" things of Airflow
Yeah. It looks good for me as it is. I don't think we need anything more than setup.cfg change.
--
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 #17329: Split sql statements in DbApi run
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #17329:
URL: https://github.com/apache/airflow/pull/17329#discussion_r680757995
##########
File path: airflow/hooks/dbapi.py
##########
@@ -176,18 +178,32 @@ def run(self, sql, autocommit=False, parameters=None, handler=None):
:param handler: The result handler which is called with the result of each statement.
:type handler: callable
:return: query results if handler was provided.
+ :param split_statements: If true, split sql statements
+ :type split_statements: bool
"""
scalar = isinstance(sql, str)
if scalar:
sql = [sql]
+ sql_statements = sql
+
+ if split_statements:
+ sql_statements = []
+ for q in sql:
+ sql_statements.append(
+ [
+ s[:-1] if s[-1] == ';' else s
Review comment:
Do we need to strip `;`? I know most SQL engines donāt really care about them.
If we do, this is probably better
```suggestion
s.rstrip(";")
```
--
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] flolas commented on pull request #17329: Split sql statements in DbApi run
Posted by GitBox <gi...@apache.org>.
flolas commented on pull request #17329:
URL: https://github.com/apache/airflow/pull/17329#issuecomment-893071375
> sqlparse is not a core dependency -- it needs adding.
Hi @ashb, what do you mean with "it needs adding"? I added sqlparse to setup.cfg. We need to do anything else?
thx
--
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 #17329: Split sql statements in DbApi run
Posted by GitBox <gi...@apache.org>.
uranusjr commented on pull request #17329:
URL: https://github.com/apache/airflow/pull/17329#issuecomment-890819018
We should also remove `sqlparse` from `drill` in setup.py I think.
--
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 #17329: Split sql statements in DbApi run
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #17329:
URL: https://github.com/apache/airflow/pull/17329#discussion_r682680957
##########
File path: airflow/hooks/dbapi.py
##########
@@ -176,18 +178,29 @@ def run(self, sql, autocommit=False, parameters=None, handler=None):
:param handler: The result handler which is called with the result of each statement.
:type handler: callable
:return: query results if handler was provided.
+ :param split_statements: If true, split sql statements
+ :type split_statements: bool
"""
scalar = isinstance(sql, str)
if scalar:
sql = [sql]
+ sql_statements = sql
+
+ if split_statements:
+ sql_statements = []
+ for q in sql:
+ sql_statements.append(
+ [s.rstrip(";") for s in sqlparse.split(sqlparse.format(q, strip_comments=True))]
+ )
Review comment:
Should this be `extend` instead?
--
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 #17329: Split sql statements in DbApi run
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #17329:
URL: https://github.com/apache/airflow/pull/17329#issuecomment-892032420
My thoughts exactly @uranusjr. I was a bit on a side of backwards compatibility and our SemVer approach (which would make such change wait for 3.* Release).
But we might argue if someone put more statements there, not executing all of them was actually a bug that this PR fixes so I am also for split_statement =true by default, or even better REMOVING it altogether. It should be equally easy for a user to fix any problem resulting from that change by changing the parameter to false as cleaning up the statement and removing extra sql statements. There are some edge cases where this would not be easy I imagine, but i think it is really, really edge 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 #17329: Split sql statements in DbApi run
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #17329:
URL: https://github.com/apache/airflow/pull/17329#issuecomment-931040611
Hmm. Can you please rebase that on e @flolas ?
--
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 #17329: Split sql statements in DbApi run
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #17329:
URL: https://github.com/apache/airflow/pull/17329#issuecomment-913117409
One other comment here: It would be great to add unit tests for more complex queries like: https://stackoverflow.com/questions/69049321/javascript-udf-fails-in-airflow-but-works-in-snowflake/69049529#69049529
There are some DB's (like snowflake) that allow to embed other languages in SQL (like javascript in this case). The split used in Snowflake was not resilient to that when `;` were part of the Javascript. I wonder if sqlparse will handle it properly.
--
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 #17329: Split sql statements in DbApi run
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #17329:
URL: https://github.com/apache/airflow/pull/17329#discussion_r702394866
##########
File path: setup.cfg
##########
@@ -149,6 +149,7 @@ install_requires =
setproctitle>=1.1.8, <2
sqlalchemy>=1.3.18
sqlalchemy_jsonfield~=1.0
+ sqlparse~=0.4.1
Review comment:
Any explicit reason to exclude `sqlparse >= 0.5`?
--
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 #17329: Split sql statements in DbApi run
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #17329:
URL: https://github.com/apache/airflow/pull/17329#discussion_r702625190
##########
File path: tests/hooks/test_dbapi.py
##########
@@ -232,3 +233,74 @@ def handler(cur):
assert called == 2
assert self.conn.commit.called
assert result == [obj, obj]
+
+ @patch('airflow.hooks.dbapi.DbApiHook._run_command')
+ def test_run_with_multiple_statements_and_split(self, _run_command):
+ _cases = [
+ (['SQL1; SQL2;', 'SQL3'], 3),
+ (['SQL1; SQL2;', 'SQL3;'], 3),
+ (['SQL1; SQL2; SQL3;'], 3),
+ ('SQL1; SQL2; SQL3;', 3),
+ (['SQL1;', 'SQL2'], 2),
+ (['SQL1;', 'SQL2;'], 2),
+ (['SQL1; SQL2;'], 2),
+ ('SQL1; SQL2;', 2),
+ (['SQL1'], 1),
+ (['SQL1;'], 1),
+ ('SQL1;', 1),
+ ('SQL1', 1),
+ (
+ """
+ CREATE OR REPLACE FUNCTION dfp.extract_vcode(NAME VARCHAR)
+ RETURNS string
+ LANGUAGE javascript
+ STRICT
+ AS '
+ const regex = /[[]v=([0-9]+)/ig;
+ let s = NAME.match(regex);
+ if (s != null) {
+ return s[0].split('=')[1];
+ } else {
+ return null;
+ }
+ ';
+ CREATE OR REPLACE FUNCTION dfp.parse_metadata(DATA varchar)
+ RETURNS OBJECT
+ LANGUAGE javascript
+ STRICT
+ AS '
+ if (!DATA) {
+ return {}
+ }
+
+ let dict = {}
+ const parts = DATA.split("|")
+
+ parts.forEach((p) => {
+ const split = p.split("=")
+ dict[split[0]] = split[1]
+ })
+
+ return dict
+ ';
+ """, # nopep8
+ 2,
+ ),
+ (
+ """
+ SELECT
+ *
+ FROM country
+ LEFT JOIN city ON city.country_id = country.id
+ LEFT JOIN customer ON city.id = customer.city_id
+ LEFT JOIN call ON call.customer_id = customer.id;
+ SELECT country, count(*) FROM country
+ GROUP BY 1;
+ """, # nopep8
Review comment:
Why is this `nopep8` comment needed?
##########
File path: tests/hooks/test_dbapi.py
##########
@@ -232,3 +233,74 @@ def handler(cur):
assert called == 2
assert self.conn.commit.called
assert result == [obj, obj]
+
+ @patch('airflow.hooks.dbapi.DbApiHook._run_command')
+ def test_run_with_multiple_statements_and_split(self, _run_command):
+ _cases = [
+ (['SQL1; SQL2;', 'SQL3'], 3),
+ (['SQL1; SQL2;', 'SQL3;'], 3),
+ (['SQL1; SQL2; SQL3;'], 3),
+ ('SQL1; SQL2; SQL3;', 3),
+ (['SQL1;', 'SQL2'], 2),
+ (['SQL1;', 'SQL2;'], 2),
+ (['SQL1; SQL2;'], 2),
+ ('SQL1; SQL2;', 2),
+ (['SQL1'], 1),
+ (['SQL1;'], 1),
+ ('SQL1;', 1),
+ ('SQL1', 1),
+ (
+ """
+ CREATE OR REPLACE FUNCTION dfp.extract_vcode(NAME VARCHAR)
+ RETURNS string
+ LANGUAGE javascript
+ STRICT
+ AS '
+ const regex = /[[]v=([0-9]+)/ig;
+ let s = NAME.match(regex);
+ if (s != null) {
+ return s[0].split('=')[1];
+ } else {
+ return null;
+ }
+ ';
+ CREATE OR REPLACE FUNCTION dfp.parse_metadata(DATA varchar)
+ RETURNS OBJECT
+ LANGUAGE javascript
+ STRICT
+ AS '
+ if (!DATA) {
+ return {}
+ }
+
+ let dict = {}
+ const parts = DATA.split("|")
+
+ parts.forEach((p) => {
+ const split = p.split("=")
+ dict[split[0]] = split[1]
+ })
+
+ return dict
+ ';
+ """, # nopep8
+ 2,
+ ),
+ (
+ """
+ SELECT
+ *
+ FROM country
+ LEFT JOIN city ON city.country_id = country.id
+ LEFT JOIN customer ON city.id = customer.city_id
+ LEFT JOIN call ON call.customer_id = customer.id;
+ SELECT country, count(*) FROM country
+ GROUP BY 1;
+ """, # nopep8
Review comment:
Also this entire `for case in cases` thing should be done with `parameterized` instead. And I feel we should check the actual parsed SQL, not just the count.
--
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 #17329: Split sql statements in DbApi run
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #17329:
URL: https://github.com/apache/airflow/pull/17329#issuecomment-930563879
We had some fix merged for helm chart timeouts - let's see if it helps.
--
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 #17329: Split sql statements in DbApi run
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #17329:
URL: https://github.com/apache/airflow/pull/17329#issuecomment-929363293
Looks like some temporary problems at the time it was run. Just in case close/reopen to check
--
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 #17329: Split sql statements in DbApi run
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #17329:
URL: https://github.com/apache/airflow/pull/17329#discussion_r702394317
##########
File path: airflow/hooks/dbapi.py
##########
@@ -190,18 +191,28 @@ def run(self, sql, autocommit=False, parameters=None, handler=None):
:param handler: The result handler which is called with the result of each statement.
:type handler: callable
:return: query results if handler was provided.
+ :param split_statements: If true, split sql statements
+ :type split_statements: bool
"""
scalar = isinstance(sql, str)
if scalar:
sql = [sql]
+ sql_statements = sql
+
+ if split_statements:
+ sql_statements = []
+ for q in sql:
+ sql_statements.extend(
+ [s.rstrip(";") for s in sqlparse.split(sqlparse.format(q, strip_comments=True))]
+ )
Review comment:
```suggestion
if split_statements:
sql_statements = [
s
for q in sql
for s in sqlparse.split(sqlparse.format(q, strip_comments=True))
]
else:
sql_statements = sql
```
--
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 edited a comment on pull request #17329: Split sql statements in DbApi run
Posted by GitBox <gi...@apache.org>.
potiuk edited a comment on pull request #17329:
URL: https://github.com/apache/airflow/pull/17329#issuecomment-913117409
One other comment here: It would be great to add unit tests for more complex queries like: https://stackoverflow.com/questions/69049321/javascript-udf-fails-in-airflow-but-works-in-snowflake/69049529#69049529
There are some DB's (like snowflake) that allow to embed other languages in SQL (like javascript in this case). The split used in Snowflake Operator is not resilient to that when `;` were part of the Javascript. I wonder if sqlparse will handle it properly.
--
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 #17329: Split sql statements in DbApi run
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #17329:
URL: https://github.com/apache/airflow/pull/17329#discussion_r702394237
##########
File path: airflow/hooks/dbapi.py
##########
@@ -190,18 +191,28 @@ def run(self, sql, autocommit=False, parameters=None, handler=None):
:param handler: The result handler which is called with the result of each statement.
:type handler: callable
:return: query results if handler was provided.
+ :param split_statements: If true, split sql statements
+ :type split_statements: bool
"""
scalar = isinstance(sql, str)
if scalar:
sql = [sql]
+ sql_statements = sql
+
+ if split_statements:
+ sql_statements = []
+ for q in sql:
+ sql_statements.extend(
+ [s.rstrip(";") for s in sqlparse.split(sqlparse.format(q, strip_comments=True))]
+ )
Review comment:
```suggestion
sql_statements = [
s
for q in sql
for s in sqlparse.split(sqlparse.format(q, strip_comments=True))
]
```
--
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 #17329: Split sql statements in DbApi run
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #17329:
URL: https://github.com/apache/airflow/pull/17329#discussion_r716578805
##########
File path: airflow/hooks/dbapi.py
##########
@@ -259,6 +266,27 @@ def get_cursor(self):
"""Returns a cursor"""
return self.get_conn().cursor()
+ @staticmethod
+ def _split_sql_statements(sql, strip_comments=True, strip_semicolon=True, **kwargs):
+ """
+ Split multiple sql statements in string
+
+ :param sql: sql statements.
+ :type sql: str or list.
+ :return: splitted sql statements.
+ :rtype: list.
+ """
+ if isinstance(sql, str):
+ sql = [sql]
+
+ return [
+ s.rstrip(';') if strip_semicolon else s
Review comment:
SQL engines are the third worst things in the world (behind browser CSS and JavaScript implementations)
##########
File path: airflow/hooks/dbapi.py
##########
@@ -259,6 +266,27 @@ def get_cursor(self):
"""Returns a cursor"""
return self.get_conn().cursor()
+ @staticmethod
+ def _split_sql_statements(sql, strip_comments=True, strip_semicolon=True, **kwargs):
+ """
+ Split multiple sql statements in string
+
+ :param sql: sql statements.
+ :type sql: str or list.
+ :return: splitted sql statements.
+ :rtype: list.
+ """
+ if isinstance(sql, str):
+ sql = [sql]
+
+ return [
+ s.rstrip(';') if strip_semicolon else s
Review comment:
SQL engines are the third worst thing in the world (behind browser CSS and JavaScript implementations)
--
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 #17329: Split sql statements in DbApi run
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #17329:
URL: https://github.com/apache/airflow/pull/17329#discussion_r682680957
##########
File path: airflow/hooks/dbapi.py
##########
@@ -176,18 +178,29 @@ def run(self, sql, autocommit=False, parameters=None, handler=None):
:param handler: The result handler which is called with the result of each statement.
:type handler: callable
:return: query results if handler was provided.
+ :param split_statements: If true, split sql statements
+ :type split_statements: bool
"""
scalar = isinstance(sql, str)
if scalar:
sql = [sql]
+ sql_statements = sql
+
+ if split_statements:
+ sql_statements = []
+ for q in sql:
+ sql_statements.append(
+ [s.rstrip(";") for s in sqlparse.split(sqlparse.format(q, strip_comments=True))]
+ )
Review comment:
Should this be `extend` instead?
--
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] flolas commented on a change in pull request #17329: Split sql statements in DbApi run
Posted by GitBox <gi...@apache.org>.
flolas commented on a change in pull request #17329:
URL: https://github.com/apache/airflow/pull/17329#discussion_r683045221
##########
File path: airflow/hooks/dbapi.py
##########
@@ -176,18 +178,29 @@ def run(self, sql, autocommit=False, parameters=None, handler=None):
:param handler: The result handler which is called with the result of each statement.
:type handler: callable
:return: query results if handler was provided.
+ :param split_statements: If true, split sql statements
+ :type split_statements: bool
"""
scalar = isinstance(sql, str)
if scalar:
sql = [sql]
+ sql_statements = sql
+
+ if split_statements:
+ sql_statements = []
+ for q in sql:
+ sql_statements.append(
+ [s.rstrip(";") for s in sqlparse.split(sqlparse.format(q, strip_comments=True))]
+ )
Review comment:
Yes! @uranusjr good catch!
--
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] flolas edited a comment on pull request #17329: Split sql statements in DbApi run
Posted by GitBox <gi...@apache.org>.
flolas edited a comment on pull request #17329:
URL: https://github.com/apache/airflow/pull/17329#issuecomment-893071375
> sqlparse is not a core dependency -- it needs adding.
Hi @ashb, what do you mean with "it needs adding"? I added sqlparse to setup.cfg. We need to do anything else?
note: I'm not familiar with all "core" things of Airflow
thx
--
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 #17329: Split sql statements in DbApi run
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #17329:
URL: https://github.com/apache/airflow/pull/17329#issuecomment-930563879
We had some fix merged for helm chart timeouts - let's see if it helps.
--
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 closed pull request #17329: Split sql statements in DbApi run
Posted by GitBox <gi...@apache.org>.
potiuk closed pull request #17329:
URL: https://github.com/apache/airflow/pull/17329
--
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 #17329: Split sql statements in DbApi run
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #17329:
URL: https://github.com/apache/airflow/pull/17329#issuecomment-893215329
> > sqlparse is not a core dependency -- it needs adding.
>
> Hi @ashb, what do you mean with "it needs adding"? I added sqlparse to setup.cfg. We need to do anything else?
> note: I'm not familiar with all "core" things of Airflow
Yeah. It looks good for me as it is. I don't think we need anything more than setup.cfg change.
--
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 #17329: Split sql statements in DbApi run
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #17329:
URL: https://github.com/apache/airflow/pull/17329#discussion_r685059467
##########
File path: airflow/hooks/dbapi.py
##########
@@ -176,18 +177,28 @@ def run(self, sql, autocommit=False, parameters=None, handler=None):
:param handler: The result handler which is called with the result of each statement.
:type handler: callable
:return: query results if handler was provided.
+ :param split_statements: If true, split sql statements
+ :type split_statements: bool
"""
scalar = isinstance(sql, str)
if scalar:
sql = [sql]
+ sql_statements = sql
+
+ if split_statements:
+ sql_statements = []
+ for q in sql:
+ sql_statements.extend(
+ [s.rstrip(";") for s in sqlparse.split(sqlparse.format(q, strip_comments=True))]
+ )
Review comment:
I think we need to also set `scalar = False` here? Otherwise only the last statementās result can be returned, which seems unintuitive to me.
--
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] flolas commented on pull request #17329: Split sql statements in DbApi run
Posted by GitBox <gi...@apache.org>.
flolas commented on pull request #17329:
URL: https://github.com/apache/airflow/pull/17329#issuecomment-892050301
@uranusjr @potiuk Okay, I think that defaulting True for split_statement sounds good, bc if someone has some edge-cases (sqlparse is not perfect) can be easily fixed changing to False as you both said. Maybe in JdbcOperator it is not necessary to expose split_statement too. If we are okay, i can make the changes to the 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] uranusjr commented on a change in pull request #17329: Split sql statements in DbApi run
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #17329:
URL: https://github.com/apache/airflow/pull/17329#discussion_r730570544
##########
File path: airflow/hooks/dbapi.py
##########
@@ -190,18 +191,24 @@ def run(self, sql, autocommit=False, parameters=None, handler=None):
:param handler: The result handler which is called with the result of each statement.
:type handler: callable
:return: query results if handler was provided.
+ :param split_statements: If true, split sql statements
Review comment:
This probably needs to be expanded a bit. What does "split" means? Maybe something like *execute `sql` as multiple SQL statements, separated by `;`*?
--
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 #17329: Split sql statements in DbApi run
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #17329:
URL: https://github.com/apache/airflow/pull/17329#discussion_r716129135
##########
File path: airflow/hooks/dbapi.py
##########
@@ -259,6 +266,27 @@ def get_cursor(self):
"""Returns a cursor"""
return self.get_conn().cursor()
+ @staticmethod
+ def _split_sql_statements(sql, strip_comments=True, strip_semicolon=True, **kwargs):
+ """
+ Split multiple sql statements in string
+
+ :param sql: sql statements.
+ :type sql: str or list.
+ :return: splitted sql statements.
+ :rtype: list.
+ """
+ if isinstance(sql, str):
+ sql = [sql]
+
+ return [
+ s.rstrip(';') if strip_semicolon else s
Review comment:
When would this semicolon stripping be useful?
--
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] flolas commented on pull request #17329: Split sql statements in DbApi run
Posted by GitBox <gi...@apache.org>.
flolas commented on pull request #17329:
URL: https://github.com/apache/airflow/pull/17329#issuecomment-928259116
@potiuk Tests fails :( whats wrong?
--
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 #17329: Split sql statements in DbApi run
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #17329:
URL: https://github.com/apache/airflow/pull/17329#issuecomment-929363293
Looks like some temporary problems at the time it was run. Just in case close/reopen to check
--
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 #17329: Split sql statements in DbApi run
Posted by GitBox <gi...@apache.org>.
uranusjr commented on pull request #17329:
URL: https://github.com/apache/airflow/pull/17329#issuecomment-891495665
Implementation LGTM! Iām wondering about the interface though. Right now the default `split_statements` is False, but the argument is persuambly meaningless when `sql` contains only one statement (barring sqlparse bugs), so the only scenario youād want `split_statements=False` *and* be meaningful is when `sql` contains multiple statements but you donāt want them to be splitted (or when `sqlparse` incorrectly splits a statment into multiple ones), which is quite rare. So I feel maybe `split_statements` should default to True instead.
--
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 #17329: Split sql statements in DbApi run
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #17329:
URL: https://github.com/apache/airflow/pull/17329#issuecomment-894177284
Looks like all those errors are intermittent (And the rest LGTM)
--
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 #17329: Split sql statements in DbApi run
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #17329:
URL: https://github.com/apache/airflow/pull/17329#discussion_r702394784
##########
File path: airflow/hooks/dbapi.py
##########
@@ -215,7 +226,7 @@ def run(self, sql, autocommit=False, parameters=None, handler=None):
if handler is None:
return None
- if scalar:
+ if len(results) == 1:
Review comment:
I feel instead of checking the length of `results`, we should instead check the length of `sql_statements`.
Also, is `scalar` no longer needed?
--
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] flolas commented on pull request #17329: Split sql statements in DbApi run
Posted by GitBox <gi...@apache.org>.
flolas commented on pull request #17329:
URL: https://github.com/apache/airflow/pull/17329#issuecomment-928259116
@potiuk Tests fails :( whats wrong?
--
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] flolas commented on a change in pull request #17329: Split sql statements in DbApi run
Posted by GitBox <gi...@apache.org>.
flolas commented on a change in pull request #17329:
URL: https://github.com/apache/airflow/pull/17329#discussion_r702201377
##########
File path: airflow/hooks/dbapi.py
##########
@@ -176,18 +177,28 @@ def run(self, sql, autocommit=False, parameters=None, handler=None):
:param handler: The result handler which is called with the result of each statement.
:type handler: callable
:return: query results if handler was provided.
+ :param split_statements: If true, split sql statements
+ :type split_statements: bool
"""
scalar = isinstance(sql, str)
if scalar:
sql = [sql]
+ sql_statements = sql
+
+ if split_statements:
+ sql_statements = []
+ for q in sql:
+ sql_statements.extend(
+ [s.rstrip(";") for s in sqlparse.split(sqlparse.format(q, strip_comments=True))]
+ )
Review comment:
I was on vacation! You are right.
--
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 closed pull request #17329: Split sql statements in DbApi run
Posted by GitBox <gi...@apache.org>.
potiuk closed pull request #17329:
URL: https://github.com/apache/airflow/pull/17329
--
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] flolas commented on a change in pull request #17329: Split sql statements in DbApi run
Posted by GitBox <gi...@apache.org>.
flolas commented on a change in pull request #17329:
URL: https://github.com/apache/airflow/pull/17329#discussion_r683045221
##########
File path: airflow/hooks/dbapi.py
##########
@@ -176,18 +178,29 @@ def run(self, sql, autocommit=False, parameters=None, handler=None):
:param handler: The result handler which is called with the result of each statement.
:type handler: callable
:return: query results if handler was provided.
+ :param split_statements: If true, split sql statements
+ :type split_statements: bool
"""
scalar = isinstance(sql, str)
if scalar:
sql = [sql]
+ sql_statements = sql
+
+ if split_statements:
+ sql_statements = []
+ for q in sql:
+ sql_statements.append(
+ [s.rstrip(";") for s in sqlparse.split(sqlparse.format(q, strip_comments=True))]
+ )
Review comment:
Yes! @uranusjr good catch!
--
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 #17329: Split sql statements in DbApi run
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #17329:
URL: https://github.com/apache/airflow/pull/17329#issuecomment-891306016
The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.
--
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 pull request #17329: Split sql statements in DbApi run
Posted by GitBox <gi...@apache.org>.
ashb commented on pull request #17329:
URL: https://github.com/apache/airflow/pull/17329#issuecomment-893415749
> > sqlparse is not a core dependency -- it needs adding.
>
> Hi @ashb, what do you mean with "it needs adding"? I added sqlparse to setup.cfg. We need to do anything else?
> note: I'm not familiar with all "core" things of Airflow
>
> thx
š¤¦š»āāļø I missed that somehow.
--
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 #17329: Split sql statements in DbApi run
Posted by GitBox <gi...@apache.org>.
github-actions[bot] closed pull request #17329:
URL: https://github.com/apache/airflow/pull/17329
--
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 change in pull request #17329: Split sql statements in DbApi run
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #17329:
URL: https://github.com/apache/airflow/pull/17329#discussion_r703330922
##########
File path: tests/hooks/test_dbapi.py
##########
@@ -232,3 +233,74 @@ def handler(cur):
assert called == 2
assert self.conn.commit.called
assert result == [obj, obj]
+
+ @patch('airflow.hooks.dbapi.DbApiHook._run_command')
+ def test_run_with_multiple_statements_and_split(self, _run_command):
+ _cases = [
+ (['SQL1; SQL2;', 'SQL3'], 3),
+ (['SQL1; SQL2;', 'SQL3;'], 3),
+ (['SQL1; SQL2; SQL3;'], 3),
+ ('SQL1; SQL2; SQL3;', 3),
+ (['SQL1;', 'SQL2'], 2),
+ (['SQL1;', 'SQL2;'], 2),
+ (['SQL1; SQL2;'], 2),
+ ('SQL1; SQL2;', 2),
+ (['SQL1'], 1),
+ (['SQL1;'], 1),
+ ('SQL1;', 1),
+ ('SQL1', 1),
+ (
+ """
+ CREATE OR REPLACE FUNCTION dfp.extract_vcode(NAME VARCHAR)
+ RETURNS string
+ LANGUAGE javascript
+ STRICT
+ AS '
+ const regex = /[[]v=([0-9]+)/ig;
+ let s = NAME.match(regex);
+ if (s != null) {
+ return s[0].split('=')[1];
+ } else {
+ return null;
+ }
+ ';
+ CREATE OR REPLACE FUNCTION dfp.parse_metadata(DATA varchar)
+ RETURNS OBJECT
+ LANGUAGE javascript
+ STRICT
+ AS '
+ if (!DATA) {
+ return {}
+ }
+
+ let dict = {}
+ const parts = DATA.split("|")
+
+ parts.forEach((p) => {
+ const split = p.split("=")
+ dict[split[0]] = split[1]
+ })
+
+ return dict
+ ';
+ """, # nopep8
+ 2,
+ ),
+ (
+ """
+ SELECT
+ *
+ FROM country
+ LEFT JOIN city ON city.country_id = country.id
+ LEFT JOIN customer ON city.id = customer.city_id
+ LEFT JOIN call ON call.customer_id = customer.id;
+ SELECT country, count(*) FROM country
+ GROUP BY 1;
+ """, # nopep8
Review comment:
I think in many editors you have to set it manually (Intellij https://intellij-support.jetbrains.com/hc/en-us/community/posts/115000617684-Configuring-PEP8-not-ignoring-rules-really-changing-it-#:~:text=The%20PEP8%20limit%20in%20PyCharm,(and%2072%20for%20comments).)
PEP8 is flexible around line length and indeed we "strongly prefer a bit longer line"
--
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] flolas commented on a change in pull request #17329: Split sql statements in DbApi run
Posted by GitBox <gi...@apache.org>.
flolas commented on a change in pull request #17329:
URL: https://github.com/apache/airflow/pull/17329#discussion_r702888607
##########
File path: tests/hooks/test_dbapi.py
##########
@@ -232,3 +233,74 @@ def handler(cur):
assert called == 2
assert self.conn.commit.called
assert result == [obj, obj]
+
+ @patch('airflow.hooks.dbapi.DbApiHook._run_command')
+ def test_run_with_multiple_statements_and_split(self, _run_command):
+ _cases = [
+ (['SQL1; SQL2;', 'SQL3'], 3),
+ (['SQL1; SQL2;', 'SQL3;'], 3),
+ (['SQL1; SQL2; SQL3;'], 3),
+ ('SQL1; SQL2; SQL3;', 3),
+ (['SQL1;', 'SQL2'], 2),
+ (['SQL1;', 'SQL2;'], 2),
+ (['SQL1; SQL2;'], 2),
+ ('SQL1; SQL2;', 2),
+ (['SQL1'], 1),
+ (['SQL1;'], 1),
+ ('SQL1;', 1),
+ ('SQL1', 1),
+ (
+ """
+ CREATE OR REPLACE FUNCTION dfp.extract_vcode(NAME VARCHAR)
+ RETURNS string
+ LANGUAGE javascript
+ STRICT
+ AS '
+ const regex = /[[]v=([0-9]+)/ig;
+ let s = NAME.match(regex);
+ if (s != null) {
+ return s[0].split('=')[1];
+ } else {
+ return null;
+ }
+ ';
+ CREATE OR REPLACE FUNCTION dfp.parse_metadata(DATA varchar)
+ RETURNS OBJECT
+ LANGUAGE javascript
+ STRICT
+ AS '
+ if (!DATA) {
+ return {}
+ }
+
+ let dict = {}
+ const parts = DATA.split("|")
+
+ parts.forEach((p) => {
+ const split = p.split("=")
+ dict[split[0]] = split[1]
+ })
+
+ return dict
+ ';
+ """, # nopep8
+ 2,
+ ),
+ (
+ """
+ SELECT
+ *
+ FROM country
+ LEFT JOIN city ON city.country_id = country.id
+ LEFT JOIN customer ON city.id = customer.city_id
+ LEFT JOIN call ON call.customer_id = customer.id;
+ SELECT country, count(*) FROM country
+ GROUP BY 1;
+ """, # nopep8
Review comment:
1) because the static checks fails because of E501 (long multiline string), if there is a better way, im open to change it.
2) I think that is a good improvement for the test. Will do at the end of the day.
--
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 closed pull request #17329: Split sql statements in DbApi run
Posted by GitBox <gi...@apache.org>.
potiuk closed pull request #17329:
URL: https://github.com/apache/airflow/pull/17329
--
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] flolas edited a comment on pull request #17329: Split sql statements in DbApi run
Posted by GitBox <gi...@apache.org>.
flolas edited a comment on pull request #17329:
URL: https://github.com/apache/airflow/pull/17329#issuecomment-928259116
@potiuk Tests fail :( whats wrong?
--
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 #17329: Split sql statements in DbApi run
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #17329:
URL: https://github.com/apache/airflow/pull/17329#issuecomment-938359161
Hey @flolas - could you please rebase? We've implemented quite a number of stability fixes for the CI tests and we should be getting much more into the "green"
--
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 #17329: Split sql statements in DbApi run
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #17329:
URL: https://github.com/apache/airflow/pull/17329#discussion_r716129167
##########
File path: airflow/hooks/dbapi.py
##########
@@ -259,6 +266,27 @@ def get_cursor(self):
"""Returns a cursor"""
return self.get_conn().cursor()
+ @staticmethod
+ def _split_sql_statements(sql, strip_comments=True, strip_semicolon=True, **kwargs):
+ """
+ Split multiple sql statements in string
+
+ :param sql: sql statements.
+ :type sql: str or list.
+ :return: splitted sql statements.
+ :rtype: list.
Review comment:
Need to describe the two `strip_*` flags 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] potiuk commented on pull request #17329: Split sql statements in DbApi run
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #17329:
URL: https://github.com/apache/airflow/pull/17329#issuecomment-927220483
Besides the few nit comments LGTM too :). Looks like the `;` in string works well too so the Snowflake case will be handled !.
--
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 #17329: Split sql statements in DbApi run
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #17329:
URL: https://github.com/apache/airflow/pull/17329#discussion_r716129072
##########
File path: airflow/hooks/dbapi.py
##########
@@ -259,6 +266,27 @@ def get_cursor(self):
"""Returns a cursor"""
return self.get_conn().cursor()
+ @staticmethod
+ def _split_sql_statements(sql, strip_comments=True, strip_semicolon=True, **kwargs):
+ """
+ Split multiple sql statements in string
+
+ :param sql: sql statements.
+ :type sql: str or list.
+ :return: splitted sql statements.
+ :rtype: list.
+ """
+ if isinstance(sql, str):
+ sql = [sql]
Review comment:
I donāt think this is needed? Because you always wrap a str in a list on the call site (in `run`). Since this is a private method, always requiring a `List[str]` would make things cleaner. A type annotation on the function argument would help 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] potiuk commented on a change in pull request #17329: Split sql statements in DbApi run
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #17329:
URL: https://github.com/apache/airflow/pull/17329#discussion_r716130509
##########
File path: airflow/hooks/dbapi.py
##########
@@ -259,6 +266,27 @@ def get_cursor(self):
"""Returns a cursor"""
return self.get_conn().cursor()
+ @staticmethod
+ def _split_sql_statements(sql, strip_comments=True, strip_semicolon=True, **kwargs):
+ """
+ Split multiple sql statements in string
+
+ :param sql: sql statements.
+ :type sql: str or list.
+ :return: splitted sql statements.
+ :rtype: list.
+ """
+ if isinstance(sql, str):
+ sql = [sql]
+
+ return [
+ s.rstrip(';') if strip_semicolon else s
Review comment:
I believe there are certain engines, that actually will complain if `;` is passed. MsSQL particularly I believe.
I just learn couple of days that when you use CLI (via ODBC) to MsSQL, `;` at the end of SQL query does not make it execute - it will complain. Instead wheat you need to do is to type `GO` in the next line š±
--
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