fix: questioning a few lines that force an ORDER clause #11153
Conversation
mistercrunch
changed the title
Codecov Report
@@ Coverage Diff @@
## master #11153 +/- ##
==========================================
- Coverage 65.84% 61.55% -4.30%
==========================================
Files 827 827
Lines 39051 39046 -5
Branches 3673 3673
==========================================
- Hits 25715 24036 -1679
- Misses 13229 14829 +1600
- Partials 107 181 +74
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
I agree with removing this, as the current logic causes a funny from-top-to-bottom sweeping effect (I noticed this once when playing around with low limits). A few alternatives could be ordering by groupby, then __timestamp in descending order so as to get 1) as many complete series and 2) have them start from most recent datapoint like you mentioned. However, I feel we can get back to this later once when we start implementing server side pagination to chart data requests.
|
Can you think of other things that may break from this? Why is it there in the first place? Maybe the table viz? |
|
@villebro ^^^ ? |
|
I checked and this changes the default behavior of the "Table" visualization where currently, by default is sorts descending on the first metric. I feel like the right thing to do is to identify the visualization that do rely on this behavior and to explicitly do it for those. |
mistercrunch
force-pushed
the
query_lines
branch
from
cad8a1d
to
22aa5c9
Compare
|
I added the default behavior logic of sorting by main metric DESC in Table when not specified as part of that visualization logic. |
|
@mistercrunch sorry, I've been having trouble with notifications on GH. I'll review this asap. |
Codecov Report
@@ Coverage Diff @@
## master #11153 +/- ##
=======================================
Coverage 65.56% 65.56%
=======================================
Files 832 832
Lines 39393 39394 +1
Branches 3592 3592
=======================================
+ Hits 25828 25829 +1
Misses 13456 13456
Partials 109 109
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
* draft: questioning a few lines that force an ORDER clause * sorting by main metric DESC by default
mistercrunch
added
🏷️ bot
SUMMARY
I'm questioning the need for these 2 lines here, and would like to understand what they are about. It seems like this should be handled by the caller if/when needed.
There's nothing really wrong about doing the unnecessary sort apart for the extra cost on the database backend. In the case of the line chart, it may make more sense to sort on time DESC [if anything] to make the forced row LIMIT to show something more consistent.
before
after