Skip to content
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

Forced LIMIT clause not being applied correctly when SQL has a trailing comment #7483

Closed
dflionis opened this issue May 10, 2019 · 1 comment

Comments

Projects
None yet
1 participant
@dflionis
Copy link

commented May 10, 2019

When a user submits a query that ends in a comment, Superset incorrectly appends its built-in LIMIT clause to that same last line which makes the LIMIT clause commented out as well.

Expected results

Ideally the limit would be added on a new line so that SQL comments cannot have any effect on it.

Actual results

Superset appends the LIMIT clause to the same line which is functionally equivalent to not adding a limit at all.

Screenshots

User submits the following query (with a default LIMIT of 500 in the UI set):

SELECT y, m, d, COUNT(*)
FROM some_table
GROUP BY y, m, d -- uh oh a trailing comment!

Superset submits to the database engine:

SELECT y, m, d, COUNT(*)
FROM some_table
GROUP BY y, m, d -- uh oh a trailing comment! LIMIT 500

How to reproduce the bug

Run a query that ends in a trailing comment, such as the example user submission in the Screenshots section above.

Environment

  • superset version: 0.32.0rc2
  • python version: 3.6
  • node.js version: ???
  • npm version: ???

Checklist

Make sure these boxes are checked before submitting your issue - thank you!

  • I have checked the superset logs for python stacktraces and included it here as text if there are any.
  • I have reproduced the issue with at least the latest released version of superset.
  • I have checked the issue tracker for the same issue and I haven't found one similar.

Additional context

I'm not sure if its as simple as inserting a newline here but I'm having trouble getting the tests to pass locally so I didn't want to submit a PR that I'm not confident about.

@issue-label-bot

This comment has been minimized.

Copy link

commented May 10, 2019

Issue-Label Bot is automatically applying the label #bug to this issue, with a confidence of 0.84. Please mark this comment with 👍 or 👎 to give our bot feedback!

Links: app homepage, dashboard and code for this bot.

@issue-label-bot issue-label-bot bot added the #bug label May 10, 2019

mistercrunch added a commit to mistercrunch/superset that referenced this issue May 10, 2019

Break line before LIMIT statement to prevent trailing comment issue
This may not be a perfect solution but it addresses the issue in 7483

closes apache#7483

mistercrunch added a commit to mistercrunch/superset that referenced this issue May 10, 2019

Break line before LIMIT statement to prevent trailing comment issue
This may not be a perfect solution but it addresses the issue in 7483

closes apache#7483

mistercrunch added a commit that referenced this issue May 13, 2019

Break line before LIMIT statement to prevent trailing comment issue (#…
…7485)

* Break line before LIMIT statement to prevent trailing comment issue

This may not be a perfect solution but it addresses the issue in 7483

closes #7483

* fix tests

craig-rueda added a commit to craig-rueda/incubator-superset that referenced this issue May 16, 2019

Break line before LIMIT statement to prevent trailing comment issue (a…
…pache#7485)

* Break line before LIMIT statement to prevent trailing comment issue

This may not be a perfect solution but it addresses the issue in 7483

closes apache#7483

* fix tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.