refactor reducers #1481
Merged
refactor reducers #1481
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This commit refactors the reducer package to be more go-like and ergonomic. These changes will make it easier to add the "where" clause to reducers and add new reducer functions. The reducer compilation logic moved to groupby, as a temporary stop on the way to zq/compiler (see issue #1162). reducer/compile is now gone and reducers don't know about rows. The multi-type math got moved to zq/anymath, replacing streamfn, and expr/functions.go now uses this too.
nwt
approved these changes
Oct 13, 2020
reducer/reducer.go
Outdated
Comment on lines
50
to
51
| // Currently,tThe only reducer that supports operator without | ||
| // a field is count(). |
There was a problem hiding this comment.
Suggested change
| // Currently,tThe only reducer that supports operator without | |
| // a field is count(). | |
| // Count is the only reducer that doesn't require an argument. |
anymath/math.go
Outdated
Comment on lines
5
to
20
| type Init struct { | ||
| Float64 float64 | ||
| Int64 int64 | ||
| Uint64 uint64 | ||
| } | ||
|
|
||
| type Int64 func(int64, int64) int64 | ||
| type Uint64 func(uint64, uint64) uint64 | ||
| type Float64 func(float64, float64) float64 | ||
|
|
||
| type Function struct { | ||
| Init | ||
| Int64 | ||
| Uint64 | ||
| Float64 | ||
| } |
There was a problem hiding this comment.
NIts:
- You don't really need the Float64, Int64, and Uint64 types.
- Function is more interesting than Init, so lead with it.
- Use the same field order (Float64, Int64, Uint64) in Function and Init.
Suggested change
| type Init struct { | |
| Float64 float64 | |
| Int64 int64 | |
| Uint64 uint64 | |
| } | |
| type Int64 func(int64, int64) int64 | |
| type Uint64 func(uint64, uint64) uint64 | |
| type Float64 func(float64, float64) float64 | |
| type Function struct { | |
| Init | |
| Int64 | |
| Uint64 | |
| Float64 | |
| } | |
| type Function struct { | |
| Init | |
| Float64 func(float64, float64) float64 | |
| Int64 func(int64, int64) int64 | |
| Uint64 func(uint64, uint64) uint64 | |
| } | |
| type Init struct { | |
| Float64 float64 | |
| Int64 int64 | |
| Uint64 uint64 | |
| } |
There was a problem hiding this comment.
Did you see their use in the math reducers? I prefer the types but if people find it confusing, it's always easy to change.
| }, nil | ||
| } | ||
|
|
||
| func CompileReducer(assignment ast.Assignment) (field.Static, reducer.Maker, error) { |
There was a problem hiding this comment.
Nit: No need to export this so call it compileReducer.
There was a problem hiding this comment.
It's currently used by reducer_test.go. We can un-import this when we refactor the compiler components as this is all going to move anyway.
proc/groupby/compile.go
Outdated
Comment on lines
15
to
18
| type reducerMaker struct { | ||
| name field.Static | ||
| create reducer.Maker | ||
| } |
There was a problem hiding this comment.
Nit: This seems a little out of place here. I'd declare it immediately after type Params in groupby.go.
proc/groupby/groupby.go
Outdated
Comment on lines
172
to
173
| instance := r.create() | ||
| if _, ok := instance.(reducer.Decomposable); !ok { |
There was a problem hiding this comment.
Suggested change
| instance := r.create() | |
| if _, ok := instance.(reducer.Decomposable); !ok { | |
| if _, ok := r.create().(reducer.Decomposable); !ok { |
proc/groupby/row.go
Outdated
Comment on lines
43
to
44
| resolver := col.nameExpr | ||
| v, err := resolver.Eval(rec) |
There was a problem hiding this comment.
Suggested change
| resolver := col.nameExpr | |
| v, err := resolver.Eval(rec) | |
| v, err := col.nameExpr.Eval(rec) |
brim-bot
pushed a commit
to brimdata/zui
that referenced
this pull request
Oct 15, 2020
This is an auto-generated commit with a zq dependency update. The zq PR brimdata/super#1481, authored by @mccanne, has been merged. refactor reducers This commit refactors the reducer package to be more go-like and ergonomic. These changes will make it easier to add the "where" clause to reducers and add new reducer functions. The reducer compilation logic moved to groupby, as a temporary stop on the way to zq/compiler (see issue brimdata/super#1162). reducer/compile is now gone and reducers don't know about rows. The multi-type math got moved to zq/anymath, replacing streamfn, and expr/functions.go now uses this too.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This commit refactors the reducer package to be more go-like and
ergonomic. These changes will make it easier to add the "where"
clause to reducers and add new reducer functions.
The reducer compilation logic moved to groupby, as a temporary stop
on the way to zq/compiler (see issue #1162). reducer/compile is
now gone and reducers don't know about rows.
The multi-type math got moved to zq/anymath, replacing streamfn,
and expr/functions.go now uses this too.