-
Notifications
You must be signed in to change notification settings - Fork 56
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.
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
feat: Add support for top level aggregates #594
Changes from all commits
17572fa
447a96f
b68dfeb
35035c0
1bc8fd0
4bc39dc
4d75932
7ed7bc1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -125,6 +125,14 @@ func (p *Planner) newPlan(stmt interface{}) (planNode, error) { | |
if err != nil { | ||
return nil, err | ||
} | ||
|
||
if _, isAgg := parserTypes.Aggregates[n.Name]; isAgg { | ||
// If this Select is an aggregate, then it must be a top-level | ||
// aggregate and we need to resolve it within the context of a | ||
// top-level node. | ||
return p.Top(m) | ||
} | ||
|
||
return p.Select(m) | ||
case *mapper.Select: | ||
return p.Select(n) | ||
|
@@ -223,6 +231,23 @@ func (p *Planner) expandPlan(plan planNode, parentPlan *selectTopNode) error { | |
case *deleteNode: | ||
return p.expandPlan(n.source, parentPlan) | ||
|
||
case *topLevelNode: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. question: now we will have a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The select top node is essentially a join (on all), similar to how aggregates behave in all other cases - most of that code hasn't changed. Pre-render structure is roughly:
|
||
for _, child := range n.children { | ||
switch c := child.(type) { | ||
case *selectTopNode: | ||
// We only care about expanding the child source here, it is assumed that the parent source | ||
// is expanded elsewhere/already | ||
err := p.expandPlan(child, parentPlan) | ||
if err != nil { | ||
return err | ||
} | ||
case aggregateNode: | ||
// top-level aggregates use the top-level node as a source | ||
c.SetPlan(n) | ||
} | ||
} | ||
return nil | ||
|
||
default: | ||
return nil | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -33,13 +33,12 @@ type sumNode struct { | |
} | ||
|
||
func (p *Planner) Sum( | ||
sourceInfo *sourceInfo, | ||
field *mapper.Aggregate, | ||
parent *mapper.Select, | ||
) (*sumNode, error) { | ||
isFloat := false | ||
for _, target := range field.AggregateTargets { | ||
isTargetFloat, err := p.isValueFloat(&sourceInfo.collectionDescription, parent, &target) | ||
isTargetFloat, err := p.isValueFloat(parent, &target) | ||
if err != nil { | ||
return nil, err | ||
} | ||
|
@@ -61,7 +60,6 @@ func (p *Planner) Sum( | |
|
||
// Returns true if the value to be summed is a float, otherwise false. | ||
func (p *Planner) isValueFloat( | ||
parentDescription *client.CollectionDescription, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. thought: Outside of this PR, it would be great if we look into the generalization of that descriptions repo cache you implemented for the mapper, make it more generic to be used by any part of the codebase, then can be embedded within the planner or mapper systems respectively. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Glad you support this, as I had that in mind when writing that |
||
parent *mapper.Select, | ||
source *mapper.AggregateTarget, | ||
) (bool, error) { | ||
|
@@ -72,7 +70,11 @@ func (p *Planner) isValueFloat( | |
} | ||
|
||
if !source.ChildTarget.HasValue { | ||
// If path length is one - we are summing an inline array | ||
parentDescription, err := p.getCollectionDesc(parent.CollectionName) | ||
if err != nil { | ||
return false, err | ||
} | ||
|
||
fieldDescription, fieldDescriptionFound := parentDescription.GetField(source.Name) | ||
if !fieldDescriptionFound { | ||
return false, fmt.Errorf( | ||
|
@@ -95,11 +97,6 @@ func (p *Planner) isValueFloat( | |
return false, fmt.Errorf("Expected child select but none was found") | ||
} | ||
|
||
childCollectionDescription, err := p.getCollectionDesc(child.CollectionName) | ||
if err != nil { | ||
return false, err | ||
} | ||
|
||
if _, isAggregate := parserTypes.Aggregates[source.ChildTarget.Name]; isAggregate { | ||
// If we are aggregating an aggregate, we need to traverse the aggregation chain down to | ||
// the root field in order to determine the value type. This is recursive to allow handling | ||
|
@@ -108,7 +105,6 @@ func (p *Planner) isValueFloat( | |
|
||
for _, aggregateTarget := range sourceField.AggregateTargets { | ||
isFloat, err := p.isValueFloat( | ||
&childCollectionDescription, | ||
child, | ||
&aggregateTarget, | ||
) | ||
|
@@ -124,6 +120,11 @@ func (p *Planner) isValueFloat( | |
return false, nil | ||
} | ||
|
||
childCollectionDescription, err := p.getCollectionDesc(child.CollectionName) | ||
if err != nil { | ||
return false, err | ||
} | ||
|
||
fieldDescription, fieldDescriptionFound := childCollectionDescription.GetField(source.ChildTarget.Name) | ||
if !fieldDescriptionFound { | ||
return false, | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: A line of comment here would be great.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure how much new info I could add that isn't in the func signature, but will have a look
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
leaving as is