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

Preserve tags fields simple selector #425

Merged
merged 1 commit into from
Apr 14, 2016
Merged

Preserve tags fields simple selector #425

merged 1 commit into from
Apr 14, 2016

Conversation

yosiat
Copy link
Contributor

@yosiat yosiat commented Apr 7, 2016

Fixes #371

@yosiat yosiat force-pushed the preserve-tags-fields-simple-selector branch from 484554f to 4d57f99 Compare April 7, 2016 09:55
@yosiat yosiat mentioned this pull request Apr 7, 2016
Closed
@yosiat
Copy link
Contributor Author

yosiat commented Apr 7, 2016

This pull request and #422 are failing because of:

Downloading vet tool...
[DEBUG] go get golang.org/x/tools/cmd/vet


Executed command failed!
-- Command run was: go get golang.org/x/tools/cmd/vet
-- Failure was: package golang.org/x/tools/cmd/vet: cannot find package "golang.org/x/tools/cmd/vet" in any of:
    /usr/local/go/src/golang.org/x/tools/cmd/vet (from $GOROOT)
    /root/go/src/golang.org/x/tools/cmd/vet (from $GOPATH)

If I am not mistaken the "go tool vet" is baked in go and there is no need to "go get" it.

@yosiat yosiat mentioned this pull request Apr 7, 2016
Merged
@yosiat yosiat force-pushed the preserve-tags-fields-simple-selector branch from 4d57f99 to 68d34b8 Compare April 7, 2016 18:27
nathanielc
nathanielc reviewed Apr 8, 2016
View reviewed changes
influxql.gen.go.tmpl
@@ -125,7 +143,7 @@ func (e *{{.name}}PointEmitter) EmitPoint() (models.Point, error) {
Time: t,
Group: e.group,
Dimensions: e.dimensions,
Tags: e.tags,
Tags: ap.Aux[0].(models.Tags),
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like the cause of the CI failure. ap.Aux[0] will only have the tags if isSimpleSelector is true.

@nathanielc
Copy link
Contributor

@yosiat Other than those two small issues this looks good to go. Thanks!

@yosiat yosiat force-pushed the preserve-tags-fields-simple-selector branch from 68d34b8 to 6871378 Compare April 10, 2016 09:15
@yosiat
Copy link
Contributor Author

yosiat commented Apr 12, 2016

@nathanielc , I added a simple condition to fetch from the tags from the right place (aux/emitter's tags).
But I found out that Aux is empty for this test: TestStream_InfluxQL the Percentile test case specifically.

I see that in floatPointBulkAggregator.AggregateBatch, I do put the tags in the aux fields, but on floatPointEmitter.EmitPoint the Aux is empty.

Do you know why?

@nathanielc
Copy link
Contributor

@yosiat I took a look at the percentile case. Looks like a bug on the InfluxDB side https://github.com/influxdata/influxdb/blob/master/influxql/call_iterator.go#L991

The returned point does not preserve Aux fields or Tags. I'll ping them and see what they say. It should be an easy fix on their end of things.

@nathanielc
Copy link
Contributor

This is the PR for fixing percentile in InfluxDB influxdata/influxdb#6293

@yosiat yosiat force-pushed the preserve-tags-fields-simple-selector branch from 8947464 to b3641be Compare April 13, 2016 12:54
@yosiat
Copy link
Contributor Author

yosiat commented Apr 13, 2016

@nathanielc I tried to run the tests after doing "git pull" in $GOPATH/github.com/influxdata/influxdb but I am getting those errors:

pipeline/influxql.go:89: undefined: "github.com/influxdata/influxdb/influxql".FloatDistinctReduceSlice
pipeline/influxql.go:93: undefined: "github.com/influxdata/influxdb/influxql".IntegerDistinctReduceSlice

I did something wrong, or should I fix those errors?

@nathanielc
Copy link
Contributor

I did something wrong, or should I fix those errors?

Nope, InfluxDB core changed again and broke things. I am working on setting up vendoring so that things like this don't keep breaking Kapacitor builds.

@nathanielc
Copy link
Contributor

@yosiat Vendoring complete, a rebase should fix the build.

@yosiat yosiat force-pushed the preserve-tags-fields-simple-selector branch from b3641be to cc3cc3e Compare April 13, 2016 21:27
@yosiat
Copy link
Contributor Author

yosiat commented Apr 13, 2016

@nathanielc for some reason I fail to run the tests:

λ ~/code/go/src/github.com/influxdata/kapacitor/ preserve-tags-fields-simple-selector go test -v -run='TestStream_InfluxQL' ./integrations/streamer_test.go ./integrations/helpers_test.go
# command-line-arguments
integrations/streamer_test.go:20:2: cannot find package "command-l/vendor/github.com/influxdata/influxdb/client" in any of:
    /Users/yosi/.gvm/gos/go1.6/src/command-l/vendor/github.com/influxdata/influxdb/client (from $GOROOT)
    /Users/yosi/code/go/src/command-l/vendor/github.com/influxdata/influxdb/client (from $GOPATH)
FAIL    command-line-arguments [setup failed]

do you have any idea why?

@nathanielc
Copy link
Contributor

@yosiat Not sure, what is command-l?

@yosiat
Copy link
Contributor Author

yosiat commented Apr 13, 2016

@nathanielc I have no idea as well.. I will try to check why TopSelectorTests are not passing and will fix the TestStream_InfluxQL tests to contain the tags in their expectations tomorrow.

@yosiat
Copy link
Contributor Author

yosiat commented Apr 14, 2016

@nathanielc Does those are simple selectors as well?

  • sum
  • count
  • distinct
  • mean
  • median

By the way, I found another way to run the tests that runs -

go test -v -run='TestStream_InfluxQL' github.com/influxdata/kapacitor/integrations

@yosiat
Copy link
Contributor Author

yosiat commented Apr 14, 2016

@nathanielc are we good to go? If so I will squash

nathanielc
nathanielc reviewed Apr 14, 2016
View reviewed changes
influxql.gen.go
@@ -81,6 +91,10 @@ func (a *floatPointBulkAggregator) AggregateBatch(b *models.Batch) {
// We need to populate the Aux fields
floatPopulateAuxFieldsAndTags(&slice[i], a.topBottomInfo.FieldsAndTags, p.Fields, p.Tags)
}

if a.isSimpleSelector {
(&slice[i]).Aux = []interface{}{p.Tags}
Copy link
Contributor

Choose a reason for hiding this comment

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

There shouldn't be any need here to take the reference of the slice element.

@nathanielc
Copy link
Contributor

@yosiat Just those minor changes and a squash and this should be good to go. Thanks!

@nathanielc
Copy link
Contributor

@yosiat Also can you add a CHANGELOG entry? And mark it as BREAKING? Since is possible that someone was relying on the previous incorrect behavior.

@yosiat yosiat force-pushed the preserve-tags-fields-simple-selector branch from 5d7efb7 to 02f4e89 Compare April 14, 2016 17:01
@yosiat
Copy link
Contributor Author

yosiat commented Apr 14, 2016

@nathanielc Done, can you make sure the changelog/indentation formatting is ok?

nathanielc
nathanielc reviewed Apr 14, 2016
View reviewed changes
pipeline/influxql.gen.go.tmpl
@@ -12,6 +12,7 @@ type ReduceCreater struct {
{{end}}{{end}}{{end}}

TopBottomCallInfo *TopBottomCallInfo
IsSimpleSelector bool
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you fix these spaces too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@yosiat yosiat force-pushed the preserve-tags-fields-simple-selector branch from 02f4e89 to 4cd6a37 Compare April 14, 2016 18:33
@yosiat
Copy link
Contributor Author

yosiat commented Apr 14, 2016

@nathanielc the tests are passing and the formatting is ok 👍

@nathanielc nathanielc merged commit 354b8a8 into influxdata:master Apr 14, 2016
@yosiat yosiat deleted the preserve-tags-fields-simple-selector branch April 14, 2016 20:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@yosiat @nathanielc