Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Allow dimension and datasource values to be string #61

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Allow dimension and datasource values to be string #61

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion builder/datasource/data_source.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,15 @@ func Load(data []byte) (builder.DataSource, error) {
Typ builder.ComponentType `json:"type,omitempty"`
}
if err := json.Unmarshal(data, &t); err != nil {
return nil, err
// the datasource may be a string which is supported by Druid
var tstr string
if err := json.Unmarshal(data, &tstr); err != nil {
return nil, err
}
// We will just make it table which is the most common type according to the Druid docs
d = NewTable()
d.(*Table).SetName(tstr)
return d, nil
}
switch t.Typ {
case "globalTable":
Expand Down
14 changes: 13 additions & 1 deletion builder/dimension/dimension.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,8 +48,20 @@ func Load(data []byte) (builder.Dimension, error) {
Typ builder.ComponentType `json:"type,omitempty"`
}
if err := json.Unmarshal(data, &t); err != nil {
return nil, err
// the dimension may be a string which is supported by Druid
var tstr string
if err := json.Unmarshal(data, &tstr); err != nil {
return nil, err
}
// We will just make it a 'default' dimension
d = &Base{}
d.(*Base).SetType("Default")
d.(*Base).SetDimension(tstr)
d.(*Base).SetOutputName(tstr)
d.(*Base).SetOutputType(types.String)
Comment on lines +58 to +61
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The (*Base) shouldn't be required in this context, as it's already a concrete type.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I remove the (*Base) type assertion, the compiler complains.
image

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/facepalm

My bad, I didn't realize d already existed, I read it as d := &Base{}. Keep it as is.

return d, nil
}

switch t.Typ {
case "default":
d = NewDefault()
Expand Down
23 changes: 12 additions & 11 deletions builder/intervals/intervals.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ package intervals

import (
"encoding/json"
"errors"

"github.com/grafadruid/go-druid/builder"
)
Expand All @@ -25,17 +24,19 @@ func Load(data []byte) (builder.Intervals, error) {
if string(data) == "null" {
return i, nil
}
var t struct {
Typ builder.ComponentType `json:"type,omitempty"`
}
if err := json.Unmarshal(data, &t); err != nil {
// "intervals" in the spec is just an string array
var intv []string
if err := json.Unmarshal(data, &intv); err != nil {
return nil, err
}
switch t.Typ {
case "intervals":
i = NewIntervals()
default:
return nil, errors.New("unsupported intervals type")
// Now cast the only array item into an actual "interval"
interval := Interval(intv[0])
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no guarantee that we have an item, it could be []. We need to be more defensive here - remember, it's user supplied data.

Also, does the spec say it's only one?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately the Druid documentation for the query spec is... not very specific (from what I've seen).
The GroupBy documentation has this to say:

intervals | A JSON Object representing ISO-8601 Intervals. This defines the time ranges to run the query over.
The code example is a single object array

"intervals": [ "2012-01-01T00:00:00.000/2012-01-03T00:00:00.000" ],

However some other documentation shows that multiple time ranges are acceptable.

Returns a list of all segments, overlapping with any of given intervals, for a datasource as stored in the metadata store. Request body is array of string IS0 8601 intervals like [interval1, interval2,...] for example ["2012-01-01T00:00:00.000/2012-01-03T00:00:00.000", "2012-01-05T00:00:00.000/2012-01-07T00:00:00.000"]

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But this is a good point, the code should be able to handle multiple intervals.

// create our "intervals" object with "Typ"
i = &Intervals{
Base: Base{
Typ: "intervals",
},
Intervals: []*Interval{&interval},
}
return i, json.Unmarshal(data, &i)
return i, nil
}