simonw / datasette

An open source multi-tool for exploring and publishing data
https://datasette.io
Apache License 2.0
9.59k stars 691 forks source link

`facet_time_limit_ms` and `sql_time_limit_ms` overlap? #1780

Open davepeck opened 2 years ago

davepeck commented 2 years ago

I needed more than the default 200ms to facet a specific column in a database I was working with, so I ran datasette with --setting facet_time_limit_ms 30000 — definitely overkill!

But it still didn't work; it took a moment to realize I also needed to up my sql_time_limit_ms to something larger too.

I'm happy to submit a PR that documents this behavior if it's helpful. Or, if there's a code change we'd like to make (like making sure sql_time_limit_ms is always set to the larger of itself and facet_time_limit_ms), happy to do that too.

Apologies if I missed this somewhere in the docs. And: thanks. I'm really enjoying the simple, effective tooling datasette gives me out of the box for exploring my databases!

simonw commented 2 years ago

This is deliberate, but maybe it's a bad design decision?

Right now Datasette assumes you made a mistake if you set facet_time_limit_ms higher than sql_time_limit_ms and ignores you.

Maybe it would be better if it refused to start the server at all and showed you an error message?

% datasette . --setting facet_time_limit_ms 30000
Error: facet_time_limit_ms greater than sql_time_limit_ms. Try this instead:

    datasette . --setting facet_time_limit_ms 30000 --setting sql_time_limit_ms 30000

Or perhaps running this should set both of the time limits to 30s:

datasette . --setting facet_time_limit_ms 30000

I'm nervous about doing that though as it feels like it may surprise people who only wanted to increase one of the limits.

Option 3: make it so it's possible for facets to have a greater time limit than SQL queries generally.

Maybe that's the best option? It's not surprising to people, and I think it's reasonable to implement.