New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: downgrade sqlparse and add unit test #10165
Conversation
e4d8a5b
to
882598f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
setup.py
needs to also be updated to reflect this. I'm assuming for now setup.py
will pin to sqlparse==0.3.0
. Next to the pinned version I would also add a comment why this is pinned, and a link to the issue to make sure we can safely unpin it once it is resolved.
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
I’m sure there were a number of fixes in |
0702b8f
to
0ac1478
Compare
@villebro addressed the comments, thanks! |
Looking at the changelog I suspect this commit (related to comments) is probably the biggest contender for a regression in terms of table extraction, though it's likely to be low. |
@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. |
0ac1478
to
305444e
Compare
It is my understanding that the bug you uncovered in |
setup.py
Outdated
@@ -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", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but I suggest adding the comment raised by @mistercrunch as to why it's currently pinned
305444e
to
a83b50d
Compare
* Downgrade sqlparse and add unit test * Explain why sqlparse is pinned Co-authored-by: bogdan kyryliuk <bogdankyryliuk@dropbox.com>
SUMMARY
Downgrade sqlparse library to 0.3.0
Reported bug in sqlparse as well: andialbrecht/sqlparse#562
0.3.1 has a bug and reindentation converts [removes the space]
into
Partially reverts: https://github.com/apache/incubator-superset/pull/9786/files (sqlparse bump and the test)
TEST PLAN
unit tests and local deployment
ADDITIONAL INFORMATION
[x] Bug Fix