Limit group by to MaxGroupByPoints (currently 100,000) #2004
Conversation
|
One possible enhancement we could make to this is to either use the limit or the end time of the query. |
|
|
||
| // Write series with one point to the database. | ||
| s.MustWriteSeries("foo", "raw", []influxdb.Point{{Name: "cpu", Tags: map[string]string{"region": "us-east"}, Timestamp: mustParseTime("2000-01-01T00:00:00Z"), Fields: map[string]interface{}{"value": float64(10)}}}) | ||
| s.MustWriteSeries("foo", "raw", []influxdb.Point{{Name: "cpu", Tags: map[string]string{"region": "us-east"}, Timestamp: mustParseTime("2000-01-01T00:00:10Z"), Fields: map[string]interface{}{"value": 20}}}) |
There was a problem hiding this comment.
Doesn't this case only require 1 write? If so, the code could be shorter.
corylanou
force-pushed
the
limit-group-by
branch
from
87037ec
to
12e1537
Compare
corylanou
force-pushed
the
limit-group-by
branch
from
12e1537
to
5f29565
Compare
| @@ -1506,6 +1506,37 @@ func TestServer_LimitAndOffset(t *testing.T) { | |||
| } | |||
| } | |||
|
|
|||
| // Ensure the server can execute a group by with a fill(0) and very large limit | |||
| // that results in a grouping of over 100,000 points returns an error | |||
| func TestServer_ExecuteGroupByFillLimit(t *testing.T) { | |||
There was a problem hiding this comment.
I think server_integration_test.go would be a better place for this. Easier to just add this to the table tests.
There was a problem hiding this comment.
I was headed that direction and realized that those tests aren't set up to check for error conditions. I do agree it should be there. I'll have to do some refactoring of that test suite to make this one possible.
| } | ||
|
|
||
| // If we start getting out of our max time range, then truncate values and return | ||
| if t > m.TMax && !isRaw { |
There was a problem hiding this comment.
Would this ever happen? Shouldn't the size of resultTimes be fixed to the exact number of buckets we want?
| @@ -952,25 +961,23 @@ func runTestsData(t *testing.T, testName string, nodes Cluster, database, retent | |||
| }, | |||
| } | |||
|
|
|||
| // See if we should run a subset of this test | |||
| testPrefix := os.Getenv("TEST_PREFIX") | |||
There was a problem hiding this comment.
Can you give me an example of TEST_PREFIX?
There was a problem hiding this comment.
I like this idea. What if we made it a regex, kinda like the -run flag?
There was a problem hiding this comment.
I'm all for it. I likely won't do it on this PR due to needing to get it shipped. But if I don't get to it, feel free to add it as well. Makes a lot of sense to work just like the run flag.
|
Made the test @corylanou requested, but now the tests are failing. Looking into it. |
|
I see the original implementation of |
|
@otoolep yeah, |
|
D'oh @pauldix of course, reverse it. |
|
LGTM 🚢 |
Limit group by to MaxGroupByPoints (currently 100,000)
|
@otoolep Super big awesome thank you for picking this up! |
* Update icon font * Make nav bar icon font size a whole number * Change icons in navbar
Fixes #1992
Also fixes a bug in
results.Error(). It wasn't looking into the slice of series to see if those also had an error. I needed to fix that to make my tests work otherwise I couldn't see the error.