evidence-dev / evidence

Business intelligence as code: build fast, interactive data visualizations in pure SQL and markdown
https://evidence.dev
MIT License
3.44k stars 167 forks source link

HTML select is out of order in Dropdown #1478

Open archiewood opened 4 months ago

archiewood commented 4 months ago

Steps To Reproduce

Nav to https://mdsinabox.com/nba/historical_matchups/

Open the picker for Year

NB edited screenshot

CleanShot 2024-01-10 at 14 21 25@2x

Expected Behavior

years should be in order because they are sorted in the query

Actual Behaviour

years are not in order

csjh commented 4 months ago
image

Seems intentional based on the source markdown

We should support default=1986 though, which seems like the goal of this, might be worth adding on to #1477

archiewood commented 4 months ago

This screenshot was suboptimal for showing the issue

Here is where it gets really weird

CleanShot 2024-01-10 at 14 21 25@2x

csjh commented 4 months ago

Looks like filtering with SELECT DISTINCT throws ordering out the window, not sure if there's a good way for us to preserve (repro w/ SELECT DISTINCT range AS blah FROM (SELECT * FROM (SELECT * FROM range(1, 100) ORDER BY range DESC));)

Adding order=season on the Dropdown should fix

archiewood commented 4 months ago

@matsonj

archiewood commented 4 months ago

agree with having a default.

I think we should also have this default for the ButtonGroup

archiewood commented 4 months ago

SELECT DISTINCT range AS blah FROM (SELECT FROM (SELECT FROM range(1, 100) ORDER BY range DESC));

I guess an option is for us to not do any DISTINCT-ing

csjh commented 4 months ago

Yeah that's also an option, just not sure which is more important between distinct vs. preserving order

archiewood commented 4 months ago

Not 100% sure either but I would lean towards preserving order over distinct-ing:

Any SQL building that we do on top of the queries can lead to inexplicable results. I was on a call with a user who couldn't work out why their sort was discarded.

However if you write a query with no group by / distinct and your dropdown has multiple entries for each option, your steps to rectify are clear.

An option would be to have a distinct=true prop that you add to the Dropdown componenent

matsonj commented 4 months ago

I think the key bit from a DX perspective is that SQL should be transparently passed to components. It should be consistent for me to reason about when passing SQL to evidence components. So I vote for removing the DISTINCT and adding an option, although unclear why we need distinct in the dropdown component when we can add it in SQL.

csjh commented 4 months ago

Makes sense, I think the DISTINCT default made more sense when the query was built fully in-component (with table name passed in to the data field instead of a query result)

archiewood commented 4 months ago

although unclear why we need distinct in the dropdown component when we can add it in SQL.

Yeah I think I agree. It would only be an alternative, but SQL > New syntax a user has to learn

maartenbosteels commented 2 months ago

I am probably misunderstanding something, but can't the "order by" be done after the distinct ?

Or is the issue that the "order by" comes from the client query while the "distinct" is added by evidence? In that case I would also be in favor of removing the distinct added by evidence (the user can still do it) and keeping the order by.

csjh commented 2 months ago

Yes, the issue is that the order comes from the client query and the distinct is evidence

archiewood commented 2 months ago

My suggestion is we remove the distinct for a future release.

It shouldn't really be a breaking change, some users passing in non-distinct tables may end up with dropdowns with repeated entries, but the dropdowns would still work, and they could filter them in the SQL to rectify when they notice.