feat: support for flux cardinality query #22441
Conversation
cmd/influxd/launcher/storage_test.go
Outdated
|
|
||
| buf, err := http.SimpleQuery(l.URL(), query, l.Org.Name, l.Auth.Token) | ||
| if err != nil { | ||
| t.Fatalf("unexpected error querying server: %v", err) |
There was a problem hiding this comment.
Could we use require.NoError instead of t.Fatal in these spots? The test is mixing both styles now
There was a problem hiding this comment.
Updated! Was some cruft from copy/paste of previous tests. Using require makes the error messages more readable as well IMO, so that's a nice bonus.
cmd/influxd/launcher/storage_test.go
Outdated
| buf, err := http.SimpleQuery(l.URL(), query, l.Org.Name, l.Auth.Token) | ||
| if err != nil { | ||
| t.Fatalf("unexpected error querying server: %v", err) | ||
| } else if diff := cmp.Diff(string(buf), exp); diff != "" { |
There was a problem hiding this comment.
Preference would be for require.Equal instead of cmp.Diff + t.Fatal
There was a problem hiding this comment.
Updated; see above.
cmd/influxd/launcher/storage_test.go
Outdated
| if err != nil { | ||
| t.Fatalf("unexpected error querying server: %v", err) | ||
| } else if diff := cmp.Diff(string(buf), tt.exp); diff != "" { | ||
| t.Fatal(diff) |
There was a problem hiding this comment.
Preference for require here as well
There was a problem hiding this comment.
Updated; see above.
v1/services/storage/store.go
Outdated
| } | ||
|
|
||
| if found := reads.HasFieldValueKey(expr); found { | ||
| return nil, errors.New("field values unsupported") |
There was a problem hiding this comment.
Will this error get wrapped up with more prefixes before being shown to the user? If not, I think it'd be good to add a little more detail (i.e. filtering on field values is not supported in cardinality predicates)
There was a problem hiding this comment.
It doesn't really get wrapped in anything helpful so I updated the error message as you suggest.
No matter what, it currently gives the rather unhelpful Error: failed to execute query: 500 Internal Server Error: An internal error has occurred message, but this should be improved by #22448
There was a problem hiding this comment.
Not anymore - now it's just a pointer to the logs again.
Ideally at some point this error would translate to something user-visible. (i.e. would be translated to an influxdb.Error)
There was a problem hiding this comment.
Ah I see, #22448 will at least log the error message though, so that's something.
v1/services/storage/store.go
Outdated
| } | ||
|
|
||
| // shard is entirely within the specified time range | ||
| if sg.StartTime.After(time.Unix(0, start)) && sg.EndTime.Before(time.Unix(0, end)) { |
There was a problem hiding this comment.
Are After and Before inclusive? Or exclusive?
There was a problem hiding this comment.
Exclusive! Nice catch. I updated the code to check for shards being within the range inclusive and added some test cases for this method.
I think that's a good idea - at least for the cases where the two can perform an equivalent query. Flux can do time ranges and predicates, which InfluxQL cannot, and those are obviously going to be slower queries. But for the fast path, a benchmark would be good. I've created an issue for that here: #22455 |
Closes #21513
This PR adds support for the flux cardinality query.