Skip to content

Commit

Permalink
Apply suggestions from code review
Browse files Browse the repository at this point in the history
Co-authored-by: Noah Treuhaft <[email protected]>
  • Loading branch information
mattnibs and nwt authored Oct 17, 2023
1 parent c39dd64 commit 60fa042
Show file tree
Hide file tree
Showing 2 changed files with 3 additions and 5 deletions.
2 changes: 1 addition & 1 deletion compiler/kernel/groupby.go
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ func (b *Builder) compileAggAssignment(assignment dag.Assignment) (field.Path, *
}
this, ok := assignment.LHS.(*dag.This)
if !ok {
return nil, nil, errors.New("internal error: aggregator assignment LHS must be a static path")
return nil, nil, errors.New("internal error: aggregator assignment LHS is not a static path: %#v", assignment.LHS)

Check failure on line 48 in compiler/kernel/groupby.go

View workflow job for this annotation

GitHub Actions / output-check

too many arguments in call to errors.New

Check failure on line 48 in compiler/kernel/groupby.go

View workflow job for this annotation

GitHub Actions / test-windows

too many arguments in call to errors.New

Check failure on line 48 in compiler/kernel/groupby.go

View workflow job for this annotation

GitHub Actions / test-windows

too many arguments in call to errors.New

Check failure on line 48 in compiler/kernel/groupby.go

View workflow job for this annotation

GitHub Actions / test

too many arguments in call to errors.New

Check failure on line 48 in compiler/kernel/groupby.go

View workflow job for this annotation

GitHub Actions / test

too many arguments in call to errors.New
}
m, err := b.compileAgg(aggAST)
return this.Path, m, err
Expand Down
6 changes: 2 additions & 4 deletions compiler/semantic/op.go
Original file line number Diff line number Diff line change
Expand Up @@ -416,7 +416,7 @@ func (a *analyzer) semOp(o ast.Op, seq dag.Seq) (dag.Seq, error) {
return nil, err
}
if assignmentHasDynamicLHS(keys) {
return nil, errors.New("summarize: keys must be static field references")
return nil, errors.New("summarize: key output fields must be static")
}
if len(keys) == 0 && len(o.Aggs) == 1 {
if seq := a.singletonAgg(o.Aggs[0], seq); seq != nil {
Expand All @@ -428,7 +428,7 @@ func (a *analyzer) semOp(o ast.Op, seq dag.Seq) (dag.Seq, error) {
return nil, err
}
if assignmentHasDynamicLHS(aggs) {
return nil, errors.New("summarize: aggs must be static field references")
return nil, errors.New("summarize: aggregate output field must be static")
}
// Note: InputSortDir is copied in here but it's not meaningful
// coming from a parser AST, only from a worker using the kernel DSL,
Expand Down Expand Up @@ -967,8 +967,6 @@ func (a *analyzer) semOpAssignment(p *ast.OpAssignment) (dag.Op, error) {
var aggs, puts []dag.Assignment
for _, assign := range p.Assignments {
// Parition assignments into agg vs. puts.
// It's okay to pass false here for the summarize bool because
// semAssignment will check if the RHS is a dag.Agg and override.
a, err := a.semAssignment(assign)
if err != nil {
return nil, err
Expand Down

0 comments on commit 60fa042

Please sign in to comment.