feat(storage): enable window agg mean pushdown #19807

fchikwekwe · 2020-10-22T21:37:04Z

Final issue to port window agg mean and bare agg mean pushdown to OSS.

As discussed below, this PR will also remove capabilities and consolidate the Store and StoreReader interfaces as well as any mock interfaces for testing.

fchikwekwe · 2020-10-23T17:19:48Z

storage/flux/reader.go

@@ -54,6 +57,31 @@ type storeReader struct {
 	s storage.Store
 }

+type qWAC struct{}


This was just added to return appropriate capabilities.

Why do we need this duplicated capability struct? It is exactly the same as readsWAC.

To take one step back, I think we do not need to have the capability system in OSS at all since it does not have separate queryd and storaged services and operate everything in single node.

fchikwekwe · 2020-10-23T17:20:35Z

storage/flux/reader.go

+		}
+	}
+
+	req.Aggregate = make([]*datatypes.Aggregate, len(wai.spec.Aggregates))


datatypes was substituted for anywhere that storageproto was previously used.

fchikwekwe · 2020-10-23T17:22:25Z

v1/services/storage/store.go

@@ -54,6 +54,56 @@ type Store struct {
 	Logger     *zap.Logger
 }

+type readsWAC struct{}


added to return enabled capabilities

fchikwekwe · 2020-10-23T17:23:02Z

v1/services/storage/store.go

+	database, rp, start, end, err := s.validateArgs(source.OrganizationID, source.BucketID, req.Range.Start, req.Range.End)
+	if err != nil {
+		return nil, err
+	}
+
+	shardIDs, err := s.findShardIDs(database, rp, false, start, end)
+	if err != nil {
+		return nil, err
+	}
+	if len(shardIDs) == 0 { // TODO(jeff): this was a typed nil
+		return nil, nil
+	}
+
+	var cur reads.SeriesCursor
+	if ic, err := newIndexSeriesCursor(ctx, req.Predicate, s.TSDBStore.Shards(shardIDs)); err != nil {
+		return nil, err
+	} else if ic == nil { // TODO(jeff): this was a typed nil
+		return nil, nil
+	} else {
+		cur = ic
+	}
+
+	return reads.NewWindowAggregateResultSet(ctx, req, cur)


changed to follow OSS convention for similar pushdown types

fchikwekwe · 2020-10-23T17:23:56Z

cmd/influxd/launcher/query_test.go

 	testcases := []struct {
 		name  string
 		data  []string
 		query string
 		op    string
 		want  string
+		skip  string


Unskipped mean tests and left all other skipped for now. There are not tests for monthly durations yet as that improvement has not yet made it to OSS.

jpacik

This looks good. Were you planning on adding the pushdown rule too or is that coming in a different PR?

fchikwekwe · 2020-10-23T20:28:26Z

@jlapacik they already existed in oss here, plus the launcher tests indicate that the pushdown is occurring. Unless you're saying that i'm missing something else?

jpacik

Awesome!

ethanyzhang · 2020-10-27T15:27:34Z

query/storage.go

@@ -96,6 +96,7 @@ type ReadWindowAggregateSpec struct {
 	Aggregates  []plan.ProcedureKind
 	CreateEmpty bool
 	TimeColumn  string
+	Window      execute.Window


Why do we need this? Does it duplicate WindowEvery and Offset in this struct? I don't see the value of this gets assigned anywhere in the code?

Oh, yeah that's a remnant from the duration support update. We will need it in the future for when we add monthly durations, but with some revision I can remove it for now and we can add it back when we make that change.

And for the record, it was used in the added code. it was used to create durations like here:

influxdb/storage/flux/reader.go

Lines 278 to 300 in f34cee7

if wai.spec.WindowEvery != 0 || wai.spec.Offset != 0 {

req.Window = &datatypes.Window{

Every: &datatypes.Duration{

Nsecs: wai.spec.WindowEvery,

},

Offset: &datatypes.Duration{

Nsecs: wai.spec.Offset,

},

}

} else {

req.Window = &datatypes.Window{

Every: &datatypes.Duration{

Nsecs: wai.spec.Window.Every.Nanoseconds(),

Months: wai.spec.Window.Every.Months(),

Negative: wai.spec.Window.Every.IsNegative(),

},

Offset: &datatypes.Duration{

Nsecs: wai.spec.Window.Offset.Nanoseconds(),

Months: wai.spec.Window.Offset.Months(),

Negative: wai.spec.Window.Offset.IsNegative(),

},

}

}

But I'll be removing that since the duration support update was not an intentional part of this PR.

I'm fine if we keep this since it's just going to be added anyway when we enable the rest of the pushdowns.

We don't actually have a current issue for adding duration support right now I don't think.

Or are you saying that as we port over all the currently planned pushdowns, we'll get this revision for free as long as we don't remove these bits?

Yes that's correct.

I'm fine with leaving it then. @yzhang1991 ?

@jlapacik in that case, I think that leaving duration support in place and enabling those tests should be added to the DOD for finalizing the other pushdowns.

ethanyzhang · 2020-10-27T15:34:49Z

storage/flux/reader.go

@@ -54,6 +57,31 @@ type storeReader struct {
 	s storage.Store
 }

+type qWAC struct{}


Why do we need this duplicated capability struct? It is exactly the same as readsWAC.

To take one step back, I think we do not need to have the capability system in OSS at all since it does not have separate queryd and storaged services and operate everything in single node.

storage/reads/store.go

fchikwekwe · 2020-10-27T16:12:49Z

@yzhang1991 yeah, the team discussed we decided to backtrack on leaving the capabilities in for now. We also decided that it makes sense to consolidate the Store interface with the WindowAggregateStore interface. I'll go ahead and make that change.

fchikwekwe · 2020-10-27T18:01:24Z

query/stdlib/influxdata/influxdb/rules.go

@@ -680,69 +680,19 @@ func (rule PushDownWindowAggregateRule) Pattern() plan.Pattern {
 }

 func canPushWindowedAggregate(ctx context.Context, fnNode plan.Node) bool {


We're only pushing down mean as a part of this PR. Cases can be added back for each agg as it is ported.

fchikwekwe · 2020-10-27T20:00:55Z

query/stdlib/influxdata/influxdb/rules_test.go

 func fluxTime(t int64) flux.Time {
 	return flux.Time{
 		Absolute: time.Unix(0, t).UTC(),
 	}
 }

+var skipTests = map[string]string{


@jlapacik I had to skip these tests once I changed the planner rules. You can remove them from the map as they are enabled.

fchikwekwe force-pushed the feat/enable-mean-pushdown branch 3 times, most recently from 2273591 to f34cee7 Compare October 22, 2020 21:56

fchikwekwe requested review from a team and ethanyzhang and removed request for a team October 22, 2020 22:16

nathanielc requested a review from jpacik October 23, 2020 17:13

fchikwekwe commented Oct 23, 2020

View reviewed changes

jpacik reviewed Oct 23, 2020

View reviewed changes

jpacik approved these changes Oct 26, 2020

View reviewed changes

ethanyzhang reviewed Oct 27, 2020

View reviewed changes

fchikwekwe force-pushed the feat/enable-mean-pushdown branch 3 times, most recently from dc186b6 to fa6d59c Compare October 27, 2020 18:00

fchikwekwe commented Oct 27, 2020

View reviewed changes

fchikwekwe force-pushed the feat/enable-mean-pushdown branch from fa6d59c to c735e54 Compare October 27, 2020 19:09

feat(storage): enable window agg mean pushdown

5e1cdda

fchikwekwe force-pushed the feat/enable-mean-pushdown branch from c735e54 to 5e1cdda Compare October 27, 2020 19:54

fchikwekwe commented Oct 27, 2020

View reviewed changes

fchikwekwe requested a review from ethanyzhang October 27, 2020 20:09

jpacik approved these changes Oct 27, 2020

View reviewed changes

fchikwekwe merged commit bd38374 into master Oct 27, 2020

fchikwekwe deleted the feat/enable-mean-pushdown branch October 27, 2020 20:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(storage): enable window agg mean pushdown #19807

feat(storage): enable window agg mean pushdown #19807

fchikwekwe commented Oct 22, 2020 •

edited

Loading

fchikwekwe Oct 23, 2020

ethanyzhang Oct 27, 2020

fchikwekwe Oct 23, 2020

fchikwekwe Oct 23, 2020

fchikwekwe Oct 23, 2020

fchikwekwe Oct 23, 2020

jpacik left a comment

fchikwekwe commented Oct 23, 2020

jpacik left a comment

ethanyzhang Oct 27, 2020

fchikwekwe Oct 27, 2020

fchikwekwe Oct 27, 2020

jpacik Oct 27, 2020

fchikwekwe Oct 27, 2020

fchikwekwe Oct 27, 2020 •

edited

Loading

jpacik Oct 27, 2020

fchikwekwe Oct 27, 2020

fchikwekwe Oct 27, 2020

ethanyzhang Oct 27, 2020

fchikwekwe commented Oct 27, 2020

fchikwekwe Oct 27, 2020

fchikwekwe Oct 27, 2020

	if wai.spec.WindowEvery != 0 \|\| wai.spec.Offset != 0 {
	req.Window = &datatypes.Window{
	Every: &datatypes.Duration{
	Nsecs: wai.spec.WindowEvery,
	},
	Offset: &datatypes.Duration{
	Nsecs: wai.spec.Offset,
	},
	}
	} else {
	req.Window = &datatypes.Window{
	Every: &datatypes.Duration{
	Nsecs: wai.spec.Window.Every.Nanoseconds(),
	Months: wai.spec.Window.Every.Months(),
	Negative: wai.spec.Window.Every.IsNegative(),
	},
	Offset: &datatypes.Duration{
	Nsecs: wai.spec.Window.Offset.Nanoseconds(),
	Months: wai.spec.Window.Offset.Months(),
	Negative: wai.spec.Window.Offset.IsNegative(),
	},
	}
	}

		@@ -680,69 +680,19 @@ func (rule PushDownWindowAggregateRule) Pattern() plan.Pattern {
		}

		func canPushWindowedAggregate(ctx context.Context, fnNode plan.Node) bool {

feat(storage): enable window agg mean pushdown #19807

feat(storage): enable window agg mean pushdown #19807

Conversation

fchikwekwe commented Oct 22, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpacik left a comment

Choose a reason for hiding this comment

fchikwekwe commented Oct 23, 2020

jpacik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fchikwekwe Oct 27, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fchikwekwe commented Oct 27, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fchikwekwe commented Oct 22, 2020 •

edited

Loading

fchikwekwe Oct 27, 2020 •

edited

Loading