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

remove DISTINCT ON statement #4869

Merged
merged 1 commit into from Apr 24, 2018
Merged

remove DISTINCT ON statement #4869

merged 1 commit into from Apr 24, 2018

Conversation

stillmatic
Copy link
Contributor

this function is used in the 'Enable Filter Select' function. Because it passes column_name to distinct the function is only usable by backends which support the DISTINCT ON statement, i.e. only postgres.

Here is (gist of) the error string I get:

SELECT DISTINCT ON is not supported [SQL: 'SELECT DISTINCT ON (column_name) column_name AS column_name \nFROM public.table_name \n LIMIT 10000 (Background on this error at: http://sqlalche.me/e/tw8g)

DISTINCT ON is only meaningful when selecting values from multiple columns. Because we are selecting rows from only one column, the above processed statement is logically identical to SELECT DISTINCT column_name FROM table_name LIMIT 10.

this passes CI / tox / etc since it's a one line change.

unbreaks for redshift tables and output is identical
@xrmx
Copy link
Contributor

xrmx commented Apr 23, 2018

You are changing the semantics though, and not sure the change in semantics won't introduce issues. What database are you using?

@codecov-io
Copy link

codecov-io commented Apr 23, 2018

Codecov Report

Merging #4869 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #4869   +/-   ##
=======================================
  Coverage   76.96%   76.96%           
=======================================
  Files          44       44           
  Lines        8534     8534           
=======================================
  Hits         6568     6568           
  Misses       1966     1966
Impacted Files Coverage Δ
superset/connectors/sqla/models.py 74.23% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2cd016f...79511e2. Read the comment docs.

@mistercrunch
Copy link
Member

LGTM

@mistercrunch mistercrunch merged commit 7193a47 into apache:master Apr 24, 2018
michellethomas pushed a commit to michellethomas/panoramix that referenced this pull request May 24, 2018
unbreaks for redshift tables and output is identical
timifasubaa pushed a commit to timifasubaa/incubator-superset that referenced this pull request May 31, 2018
unbreaks for redshift tables and output is identical
wenchma pushed a commit to wenchma/incubator-superset that referenced this pull request Nov 16, 2018
unbreaks for redshift tables and output is identical
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.25.0 labels Feb 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.25.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants