-
Notifications
You must be signed in to change notification settings - Fork 13.5k
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(mssql): support top syntax for limiting queries #18746
Conversation
MSSQL is not supporting LIMIT syntax in SQLs. For limiting the rows, MSSQL having a different keyword TOP. Added fixes for handling the TOP and LIMIT clauses based on the database engines.
Codecov Report
@@ Coverage Diff @@
## master #18746 +/- ##
==========================================
+ Coverage 66.31% 66.43% +0.11%
==========================================
Files 1620 1617 -3
Lines 63088 62847 -241
Branches 6372 6320 -52
==========================================
- Hits 41839 41750 -89
+ Misses 19592 19445 -147
+ Partials 1657 1652 -5
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
Teradata code for top clause handling removed from teradata.py file, since we added generic section in base engine for the same.
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.
First pass comments. Let's also ping the original authors of the Teradata TOP functionality when this is ready 👍
Added changes to handle TOP command in CTEs, for DB Engines which are not supporting inline CTEs.
Added multiple unit test cases for MSSQL top command handling and also along with CTEs
Corrected the select_keywords name key in basengine
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.
Second pass comments
made the required corrections based on code review to keep good code readability and code cleanliness.
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.
Some last nit comments, but I think this is pretty much ready for a final test pass + a few added unit tests to the MSSQL test suite. Also, I suggest checking failing workflows on CI, as there appears to be simple linting issues that need to be addressed.
Made the changes according to the review comments.
@dmcnulla this PR generalizes the work you did in #18240 to make it work for MSSQL, too. Would you be able to review this PR, and better yet test it on Teradata to ensure it still works as intended? Also notice the updated Teradata unit tests (I pushed a few new ones to this PR as I noticed the |
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.
Two minor nits
Moved the top/limit flag check from sql_lab to core.
Changes for keeping code cleanliness
Corrected lint issue.
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.
Last nit, after this I believe this is good to go
Code cleanliness
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! Thanks for all the hard work here; this is a major step forward in ensuring Superset works properly on MSSQL, Synapse and other Azure db products! ❤️
I am noodling on #21027 (comment) and found this PR via blame view on the relevant code. @sujiplr do you have thoughts on that issue - modifying the codebase to support use of |
MSSQL is not supporting LIMIT syntax in SQLs. For limiting the rows, MSSQL having a different keyword TOP. Added fixes for handling the TOP and LIMIT clauses based on the database engines. Made this fix as generalised one for handling different database engines.
SUMMARY
This fix will help to start supporting TOP clause in SQLs. And also along with CTE queries.
BEFORE SCREENSHOTS
AFTER SCREENSHOTS
TESTING INSTRUCTIONS
You can test the SQLs as per the above screenshots.
ADDITIONAL INFORMATION