You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2020/06/25 17:16:21 UTC
[GitHub] [incubator-superset] bkyryliuk opened a new pull request #10165: fix: downgrade sqlparse and add unit test
bkyryliuk opened a new pull request #10165:
URL: https://github.com/apache/incubator-superset/pull/10165
### SUMMARY
Downgrade sqlparse library to 0.3.0
0.3.1 has a bug and reindentation converts
```
extract(HOUR from from_unixtime(hour_ts) AT TIME ZONE 'America/Los_Angeles')
```
into
```
extract(HOUR **fromfrom_unixtime(hour_ts)** AT TIME ZONE 'America/Los_Angeles')
```
### TEST PLAN
unit tests and local deployment
### ADDITIONAL INFORMATION
[x] Bug Fix
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] john-bodley commented on pull request #10165: fix: downgrade sqlparse and add unit test
Posted by GitBox <gi...@apache.org>.
john-bodley commented on pull request #10165:
URL: https://github.com/apache/incubator-superset/pull/10165#issuecomment-650403013
Looking at the changelog I suspect this [commit](https://github.com/andialbrecht/sqlparse/commit/523a010faaca4934a18f4975a3eceb823ade3526) (related to comments) is probably the biggest contender for a regression in terms of table extraction, though it's likely to be low.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] bkyryliuk commented on a change in pull request #10165: fix: downgrade sqlparse and add unit test
Posted by GitBox <gi...@apache.org>.
bkyryliuk commented on a change in pull request #10165:
URL: https://github.com/apache/incubator-superset/pull/10165#discussion_r446290160
##########
File path: tests/sql_parse_tests.py
##########
@@ -187,7 +189,8 @@ def test_select_if(self):
# SHOW TABLES ((FROM | IN) qualifiedName)? (LIKE pattern=STRING)?
def test_show_tables(self):
query = "SHOW TABLES FROM s1 like '%order%'"
- self.assertEqual(set(), self.extract_tables(query))
+ # TODO: figure out what should code do here
Review comment:
done
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] villebro commented on pull request #10165: fix: downgrade sqlparse and add unit test
Posted by GitBox <gi...@apache.org>.
villebro commented on pull request #10165:
URL: https://github.com/apache/incubator-superset/pull/10165#issuecomment-653358417
It is my understanding that the bug you uncovered in `0.3.1` is more serious than the one that I know of in `0.3.0`, from the perspective of Superset anyway. Therefore I believe the best course of action is to merge 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] villebro commented on a change in pull request #10165: fix: downgrade sqlparse and add unit test
Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #10165:
URL: https://github.com/apache/incubator-superset/pull/10165#discussion_r445970108
##########
File path: tests/sql_parse_tests.py
##########
@@ -187,7 +189,8 @@ def test_select_if(self):
# SHOW TABLES ((FROM | IN) qualifiedName)? (LIKE pattern=STRING)?
def test_show_tables(self):
query = "SHOW TABLES FROM s1 like '%order%'"
- self.assertEqual(set(), self.extract_tables(query))
+ # TODO: figure out what should code do here
Review comment:
When I bumped from `0.3.0` to `0.3.1`, the comments and the test case weren't super clear to me. It would be more transparent to state that this test should return an empty set, but on version `0.3.0` it incorrectly returns the database/schema name. Also it would be good to include the expected true test case here commented out, so that the next person knows that the test is expected to fail now, but commenting out the true case should work when bumping to `>=0.3.2`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] bkyryliuk commented on a change in pull request #10165: fix: downgrade sqlparse and add unit test
Posted by GitBox <gi...@apache.org>.
bkyryliuk commented on a change in pull request #10165:
URL: https://github.com/apache/incubator-superset/pull/10165#discussion_r450396176
##########
File path: setup.py
##########
@@ -105,7 +105,7 @@ def get_git_sha():
"slackclient>=2.6.2",
"sqlalchemy>=1.3.16, <2.0",
"sqlalchemy-utils>=0.36.6,<0.37",
- "sqlparse>=0.3.0, <0.4",
+ "sqlparse==0.3.0",
Review comment:
done
##########
File path: setup.py
##########
@@ -105,7 +105,7 @@ def get_git_sha():
"slackclient>=2.6.2",
"sqlalchemy>=1.3.16, <2.0",
"sqlalchemy-utils>=0.36.6,<0.37",
- "sqlparse>=0.3.0, <0.4",
+ "sqlparse==0.3.0",
Review comment:
done
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] bkyryliuk commented on pull request #10165: fix: downgrade sqlparse and add unit test
Posted by GitBox <gi...@apache.org>.
bkyryliuk commented on pull request #10165:
URL: https://github.com/apache/incubator-superset/pull/10165#issuecomment-651306492
@villebro , @john-bodley how would you prefer to proceed here? We've downgraded to 0.30 @ dropbox, I would suggest to do the same for the upcoming 0.37 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] bkyryliuk merged pull request #10165: fix: downgrade sqlparse and add unit test
Posted by GitBox <gi...@apache.org>.
bkyryliuk merged pull request #10165:
URL: https://github.com/apache/incubator-superset/pull/10165
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] mistercrunch commented on a change in pull request #10165: fix: downgrade sqlparse and add unit test
Posted by GitBox <gi...@apache.org>.
mistercrunch commented on a change in pull request #10165:
URL: https://github.com/apache/incubator-superset/pull/10165#discussion_r449387375
##########
File path: setup.py
##########
@@ -105,7 +105,7 @@ def get_git_sha():
"slackclient>=2.6.2",
"sqlalchemy>=1.3.16, <2.0",
"sqlalchemy-utils>=0.36.6,<0.37",
- "sqlparse>=0.3.0, <0.4",
+ "sqlparse==0.3.0",
Review comment:
Can we add a comment right here as to why it's pinned? Maybe a link to an issue on the lib's Github (if any) or to this PR if that's the best option
```
"sqlparse==0.3.0", # PINNED! see PR #10165
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] john-bodley commented on pull request #10165: fix: downgrade sqlparse and add unit test
Posted by GitBox <gi...@apache.org>.
john-bodley commented on pull request #10165:
URL: https://github.com/apache/incubator-superset/pull/10165#issuecomment-649975565
I’m sure there were a number of fixes in `0.3.1`. I wonder if anyone has any sense on what regressions would be introduction by downgrading. I guess we could look at the `sqlparse` changelog.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] bkyryliuk commented on pull request #10165: fix: downgrade sqlparse and add unit test
Posted by GitBox <gi...@apache.org>.
bkyryliuk commented on pull request #10165:
URL: https://github.com/apache/incubator-superset/pull/10165#issuecomment-650278210
@villebro addressed the comments, thanks!
@john-bodley 0.36 version used sqlparse 0.3.0 as I remember,
https://sqlparse.readthedocs.io/en/latest/changes/ - shows some bugfixes and improvements, however I am not familiar on how they related to superset bugs. https://github.com/apache/incubator-superset/pull/9786/files doesn't seem to be mentioned in any superset issues.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org