feat: Add support for top level aggregates #594

AndrewSisley · 2022-07-06T18:35:29Z

Relevant issue(s)

Resolves #98

Description

Adds support for top-level aggregates, allowing consumers to aggregate across entire collections.

I consider the following desirable, but out of scope:

multiple top-level aggregates in same query (we dont support this for normal queries, although the top-level node goes some way to adding this support)
applying top-level node for everything. I think this would be good, and would largely solve the above issue, and an inconsistency in the return type structure (is flattened for collection-queries). Happy to explain this further over discord/zoom if people want.
nicer gql type names, code was refactored here but the old names largely remain the same - they aren't great, but there is a dedicated ticket for cleaning this up properly
explain for top level aggs

Tasks

I made sure the code is well commented, particularly hard-to-understand areas.
I made sure the repository-held documentation is changed accordingly.
I made sure the pull request title adheres to the conventional commit style (the subset used in the project can be found in tools/configs/chglog/config.yml).
I made sure to discuss its limitations such as threats to validity, vulnerability to mistake and misuse, robustness to invalidation of assumptions, resource requirements, ...

How has this been tested?

Manual type checking in the Altair client, plus int. tests.

Specify the platform(s) on which this was tested:

Debian Linux

shahzadlone · 2022-07-06T18:50:18Z

query/graphql/planner/planner.go

@@ -223,6 +231,25 @@ func (p *Planner) expandPlan(plan planNode, parentPlan *selectTopNode) error {
 	case *deleteNode:
 		return p.expandPlan(n.source, parentPlan)

+	case *topLevelNode:


question: now we will have a topLevelNode for these top-level aggregates which will wrap the selectTopNode?

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:

{ count: 2, users: [{...},{...}], }

shahzadlone · 2022-07-06T18:51:50Z

query/graphql/planner/operations.go

@@ -35,6 +35,7 @@ var (
 	_ planNode = (*typeJoinOne)(nil)
 	_ planNode = (*updateNode)(nil)
 	_ planNode = (*valuesNode)(nil)
+	_ planNode = (*topLevelNode)(nil)


question: Does this new node need to be explainable? I think it should be (can be out of this PR).

very much agree that that is out of scope, will add to description

nitpick: I think the entire list was alphabetically sorted (pre-sort-to-order-node PR) maybe if you can sort this as following, will make my ocd happy.

_ planNode = (*averageNode)(nil) _ planNode = (*commitSelectNode)(nil) _ planNode = (*commitSelectTopNode)(nil) _ planNode = (*countNode)(nil) _ planNode = (*createNode)(nil) _ planNode = (*dagScanNode)(nil) _ planNode = (*deleteNode)(nil) _ planNode = (*groupNode)(nil) _ planNode = (*hardLimitNode)(nil) _ planNode = (*headsetScanNode)(nil) _ planNode = (*multiScanNode)(nil) _ planNode = (*orderNode)(nil) _ planNode = (*parallelNode)(nil) _ planNode = (*pipeNode)(nil) _ planNode = (*renderLimitNode)(nil) _ planNode = (*scanNode)(nil) _ planNode = (*selectNode)(nil) _ planNode = (*selectTopNode)(nil) _ planNode = (*sumNode)(nil) _ planNode = (*topLevelNode)(nil) _ planNode = (*typeIndexJoin)(nil) _ planNode = (*typeJoinMany)(nil) _ planNode = (*typeJoinOne)(nil) _ planNode = (*updateNode)(nil) _ planNode = (*valuesNode)(nil)

lol.... maybe 🤣

shahzadlone · 2022-07-06T18:56:35Z

query/graphql/planner/top.go

+)
+
+// topLevelNode is a special node that represents the very top of the
+// plan graph. It has no source, and will only yield a single item


question: this represents the very top of the plan graph which contains top level aggregates only right? or for every plan we will have this? Also if it is the top level why would it not have source (linking to the rest of the plangraph, aren't the children the source)?

Just aggregates atm, this is noted in the description under stuff I consider out of scope

children are disinct from source. Source is typically what nodes iterate through, which is nil in this case and distinct from children.

Would we want Source() to return children (even if in other PR)?

Maybe, but I dont see that as a question to be answered in this PR. My gut says nil is better as the children are not really the source (as mentioned, they do not yield items in the same sense as other nodes)

shahzadlone

Beauty of a PR + tests are on point! Made some comments and asked some questions. I did a round 1 review of sorts so far, but I feel somewhat unqualified to review generate.go stuff. I will give a more detailed look when I wake up.

shahzadlone · 2022-07-07T13:30:26Z

query/graphql/mapper/mapper.go

@@ -433,13 +439,39 @@ func getRequestables(
 	return
 }

+func getAggregateRequests(index int, aggregate *parser.Select) (aggregateRequest, error) {


suggestion: A line of comment here would be great.

Not sure how much new info I could add that isn't in the func signature, but will have a look

doc getAggregateRequests?

leaving as is

shahzadlone · 2022-07-07T13:39:13Z

query/graphql/planner/operations.go

@@ -35,6 +35,7 @@ var (
 	_ planNode = (*typeJoinOne)(nil)
 	_ planNode = (*updateNode)(nil)
 	_ planNode = (*valuesNode)(nil)
+	_ planNode = (*topLevelNode)(nil)


nitpick: I think the entire list was alphabetically sorted (pre-sort-to-order-node PR) maybe if you can sort this as following, will make my ocd happy.

_ planNode = (*averageNode)(nil) _ planNode = (*commitSelectNode)(nil) _ planNode = (*commitSelectTopNode)(nil) _ planNode = (*countNode)(nil) _ planNode = (*createNode)(nil) _ planNode = (*dagScanNode)(nil) _ planNode = (*deleteNode)(nil) _ planNode = (*groupNode)(nil) _ planNode = (*hardLimitNode)(nil) _ planNode = (*headsetScanNode)(nil) _ planNode = (*multiScanNode)(nil) _ planNode = (*orderNode)(nil) _ planNode = (*parallelNode)(nil) _ planNode = (*pipeNode)(nil) _ planNode = (*renderLimitNode)(nil) _ planNode = (*scanNode)(nil) _ planNode = (*selectNode)(nil) _ planNode = (*selectTopNode)(nil) _ planNode = (*sumNode)(nil) _ planNode = (*topLevelNode)(nil) _ planNode = (*typeIndexJoin)(nil) _ planNode = (*typeJoinMany)(nil) _ planNode = (*typeJoinOne)(nil) _ planNode = (*updateNode)(nil) _ planNode = (*valuesNode)(nil)

shahzadlone · 2022-07-07T13:46:53Z

query/graphql/planner/planner.go

+			if _, isSelect := child.(*selectTopNode); isSelect {
+				// 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
+				}
+			} else {
+				switch c := child.(type) {
+				case aggregateNode:
+					// top-level aggregates use the top-level node as a source
+					c.SetPlan(n)
+				}
+			}


question: Would the following not do the same exact thing ?

Suggested change

if _, isSelect := child.(*selectTopNode); isSelect {

// 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

}

} else {

switch c := child.(type) {

case aggregateNode:

// top-level aggregates use the top-level node as a source

c.SetPlan(n)

}

}

switch c := child.(type) {

case aggregateNode:

// top-level aggregates use the top-level node as a source

c.SetPlan(n)

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

}

}

lol, my bad - thanks, will change :D

cleanup idiot code

shahzadlone · 2022-07-07T13:52:28Z

query/graphql/schema/generate.go

+					expandedField := &gql.InputObjectFieldConfig{
+						Type: g.manager.schema.TypeMap()[name+"FilterArg"],
+					}
+					aggregateTarget.Type.(*gql.InputObject).AddFieldConfig("filter", expandedField)


Suggested change

aggregateTarget.Type.(*gql.InputObject).AddFieldConfig("filter", expandedField)

aggregateTarget.Type.(*gql.InputObject).AddFieldConfig(parserTypes.FilterClause, expandedField)

Cheers, will do

const

shahzadlone · 2022-07-07T13:55:38Z

query/graphql/schema/generate.go

+			return err
+		}
+
+		objs = g.genCountInlineArrayInputs(t)


question: why do we need mutation here?

Sorry - I can't tell what you mean here, could you expand?

Sorry my bad, I was wondering why we need to re-assign to objs?

Ah, I didnt spot that cheers. will renaming it quickly

rename

shahzadlone · 2022-07-07T13:58:00Z

query/graphql/schema/generate.go

+		Fields: gql.InputObjectConfigFieldMap{
+			"_": &gql.InputObjectFieldConfig{
+				Type:        gql.Int,
+				Description: "Placeholder - empty object not permitted, but will have fields shortly",


question: Do we want a const string for this as it is used in a few places.

I don't think so, content here doesnt really matter

accident

shahzadlone

Leaving a LGTM, but feel like I wasn't qualified for some of it haha. Can do a second round if not in a rush once I wake up. Perhaps @jsimnz 's second opinion might be useful here?

AndrewSisley · 2022-07-07T14:34:42Z

Leaving a LGTM, but feel like I wasn't qualified for some of it haha

Ahh, its just code, nothing fancy. Will leave it hanging around for a couple more hours in case anyone else wants to chime in, but I have future work dependent on this and there is nothing here IMO that could have a significant impact on the health of the codebase

fredcarle · 2022-07-07T18:06:16Z

query/graphql/planner/top.go

+// plan graph. It has no source, and will only yield a single item
+// containing all of its children.
+type topLevelNode struct {
+	documentIterator


thought (non-blocking): I look forward to see this renamed to something more appropriate 😅

fredcarle

LGTM! I think I went over it 3 times and quite pleased with it. The only thing that I find a little weird is the isInRecurse with the defer switching from true to false. But I can't think of anything better at the moment so I think I'll let it go for now :)

fredcarle · 2022-07-07T18:25:48Z

query/graphql/planner/top.go

+			}
+			n.currentValue.Fields[n.childIndexes[i]] = docs
+		default:
+			_, err := child.Next()


question: why do you ignore hasChild here but not above? What if the child value is nil? Not saying it's wrong. But maybe a comment would help clarify.

Will add a comment - an explicit nil is desirable - it means whatever the user has requested is nil (current a nil is not possible anyway, as atm this can only be an aggregate, which never returns nil). Although having written that I think it would be better to check the bool and explicitly assign nil (even though it would technically be dead code atm).

tweak this

went with a comment after remembering more as to why this is like this

Had this also as a pending comment, but moving it here for consistency. I still strongly suggest to keep the bool check in place, regardless of the strongest mental gurantees we think are in place, we should never call Value() without guaranteeing from the previous Next() that the node indeed has a value.

And it costs literally nothing to have the bool check in place.

Sure will change - it will mean introducing dead code though (to handle !hasValue)

revise

I wouldn't characterize it as "dead" code. As its still enforces the expected guarantees required, so its more of a safety net.

jsimnz

Submitting my comments now, still investigating the recursive structure of topLevelNode. Generate looks good!

jsimnz · 2022-07-06T19:37:05Z

query/graphql/planner/sum.go

@@ -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,


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.

Glad you support this, as I had that in mind when writing that

jsimnz · 2022-07-07T17:56:01Z

query/graphql/planner/top.go

+	// This node's children may use this node as a source
+	// this property controls the recursive flow preventing
+	// infinate loops.
+	isInRecurse bool


suggestion: Technically this breaks the guarantees of the plan graph being a DAG (Directed Acyclical Graph).

I'm not sure I fully understand the reason for having something called "topLevelNode" that can exist not at the top of the graph.

suggestion: Technically this breaks the guarantees of the plan graph being a DAG (Directed Acyclical Graph).

For future reference: We spoke over discord about this

I'm not sure I fully understand the reason for having something called "topLevelNode" that can exist not at the top of the graph.

It does and can only exist at the top of the graph (ignoring recursive element). Making it always there is mentioned in the out-of-scope section of the PR description (item 2)

jsimnz · 2022-07-07T17:57:57Z

query/graphql/planner/top.go

+	if n.isInRecurse {
+		return
+	}
+	n.isInRecurse = true
+	defer func() {
+		n.isInRecurse = false
+	}()


suggestion: This feels funky to me, and not sure i've seen something like this used in the context of recursion, makes me feel like there is an issue with the control flow of the code. Also is a result of the DAG violation I mentioned above.

I noted the same concern above. But I can't think of anything clever at the moment to help us with it.

It would be possible to remove the recursion by adding a second new node to handle the select/join stuff, but IMO it is not worth it at the moment and is easy enough to do when expanding the feature (for explain, or multiple top-level stuff)

jsimnz · 2022-07-07T18:06:59Z

query/graphql/planner/top.go

+	if n.isdone {
+		return false, nil
+	}


suggestion: This would prevent the node from being re-used as is. Since the isDone var isn't reset during the Init call.

I thought about adding that in, but decided against it as it is untestable at the moment

It's testable, just not through integration tests. Which i guess inherently makes it a moot-ish point

IMO if a consumer cant hit a line of code it is dead and shouldn't really be tested

Similar to the other comment about !hasValue bool check. Its more about enforcing expected invariants that the planner / plangraph are supposed to have.

Some notable ones come to mind

plan graph is a DAG

Only call Value() after a succesful true call to Next()

Init() fully re-initializes the node in question (and sub nodes)

This particular comment relates the 3. I can certainly imagine there are other violations of these invariants throughout the code which will surface through additional testing, but atm this is identified here and now.

This is about ensuring that future consumer/callers of the plan graph can develop knowing certain invariants are maintained, so they don't have to go down a debugging rabbit hole only to learn a single random node doesn't properly re-initialize, or that walking the graph, expecting it to be acyclical, randomly OOMs itself because there is a circular reference somewhere.

This kind of mindset will save time/headbanging in the future. Obviously, the priority is to make sure the requirements right now are met, but not explicitly at the expense of future devs.

At the very least there are always two developers that need to interact with your code. You and Future You.

Future me should test any code he uses - if for some odd reason a piece of functionality inits a top level node twice it should 100% add tests that cover that. I find it much safer to assume that any code that isn't tested is broken, than to assume that untested code is correct.

Regarding this line, unless unit tested (which is a whole other topic) - there is no way to assert that n.isDone = false is correct (and useful - something that unit tests cant really assert very well).

RE (1) - there is nothing that asserts that this node forms part of a DAG is true or necessary. The same can be said of (2) - defra users dont give a monkey's about that.

(Generally) the only things I really care about in code is:

Does it provide provably correct behaviour to externals (externals can be internal devs in some cases)

Is it easy to change (this includes readability, cohesion, simplicity, etc)

As mentioned, the known alternative to the recursion is to introduce a second new node to handle the selects etc, this IMO would be less cohessive and more complex whilst providing no additional functionality in the present. It is not provable that it will be useful for future requirements (e.g. explain/multi-top-queries) as they have not been implemented yet, and it could end up being counter productive there costing additional time to replace with something that is provably useful.

jsimnz · 2022-07-07T20:34:21Z

query/graphql/planner/top.go

+			}
+			n.currentValue.Fields[n.childIndexes[i]] = docs
+		default:
+			_, err := child.Next()


Had this also as a pending comment, but moving it here for consistency. I still strongly suggest to keep the bool check in place, regardless of the strongest mental gurantees we think are in place, we should never call Value() without guaranteeing from the previous Next() that the node indeed has a value.

And it costs literally nothing to have the bool check in place.

Decouple them from host, cleaner for user, and allows reuse for top-level aggs

Minimal cost in re-getting, and makes it easier to call from other locations

Switch will also gain a new case shortly

Whilst they should have the same value at the moment, the disinction between the two becomes more important when introducing top-level aggregates

Will be called multiple times once top-level aggregates are introduced

AndrewSisley · 2022-07-11T15:36:08Z

Is a bug in the parser/query.go logic for aggregates that is breaking things when using a filter on a top-level-agg

Update: Was some legacy code hanging around, deleting it solved the problem

This has been incorrect for a while, and will cause problems for top-level aggregates

Any controversy appears to be very localised and this is blocking many of my remaining 0.3 tasks. Happy to continue the discussion post merge

* Rework count input objects Decouple them from host, cleaner for user, and allows reuse for top-level aggs * Remove sourceInfo param from Sum Minimal cost in re-getting, and makes it easier to call from other locations * Use switch instead of if for type check Switch will also gain a new case shortly * Remove unused type from createExpandedFieldAggregate * Use correct collection name Whilst they should have the same value at the moment, the disinction between the two becomes more important when introducing top-level aggregates * Extract out aggregate request logic to function Will be called multiple times once top-level aggregates are introduced * Remove legacy code This has been incorrect for a while, and will cause problems for top-level aggregates * Add support for top level aggregates

AndrewSisley added feature New feature or request area/query Related to the query component action/no-benchmark Skips the action that runs the benchmark. labels Jul 6, 2022

AndrewSisley added this to the DefraDB v0.3 milestone Jul 6, 2022

AndrewSisley requested a review from a team July 6, 2022 18:35

AndrewSisley self-assigned this Jul 6, 2022

AndrewSisley force-pushed the sisley/feat/I98-top-level-aggs branch 2 times, most recently from 76c2b69 to e896e05 Compare July 6, 2022 18:37

shahzadlone reviewed Jul 6, 2022

View reviewed changes

AndrewSisley requested a review from shahzadlone July 7, 2022 13:12

shahzadlone previously approved these changes Jul 7, 2022

View reviewed changes

AndrewSisley force-pushed the sisley/feat/I98-top-level-aggs branch from e896e05 to 1b73854 Compare July 7, 2022 14:20

AndrewSisley requested a review from shahzadlone July 7, 2022 14:25

shahzadlone approved these changes Jul 7, 2022

View reviewed changes

AndrewSisley force-pushed the sisley/feat/I98-top-level-aggs branch from 01bc421 to 85bac3a Compare July 7, 2022 14:32

fredcarle reviewed Jul 7, 2022

View reviewed changes

fredcarle approved these changes Jul 7, 2022

View reviewed changes

AndrewSisley force-pushed the sisley/feat/I98-top-level-aggs branch from 85bac3a to a393d66 Compare July 7, 2022 18:51

This was referenced Jul 7, 2022

Aggregate: Filter for inline arrays #392

Closed

Failure when adding a second schema set #489

Closed

jsimnz previously requested changes Jul 7, 2022

View reviewed changes

AndrewSisley requested a review from jsimnz July 8, 2022 13:43

AndrewSisley force-pushed the sisley/feat/I98-top-level-aggs branch from d402aa1 to 3c8d752 Compare July 8, 2022 13:46

AndrewSisley added 4 commits July 11, 2022 10:54

Rework count input objects

17572fa

Decouple them from host, cleaner for user, and allows reuse for top-level aggs

Remove sourceInfo param from Sum

447a96f

Minimal cost in re-getting, and makes it easier to call from other locations

Use switch instead of if for type check

b68dfeb

Switch will also gain a new case shortly

Remove unused type from createExpandedFieldAggregate

35035c0

AndrewSisley added 2 commits July 11, 2022 10:54

Use correct collection name

1bc8fd0

Whilst they should have the same value at the moment, the disinction between the two becomes more important when introducing top-level aggregates

Extract out aggregate request logic to function

4bc39dc

Will be called multiple times once top-level aggregates are introduced

AndrewSisley force-pushed the sisley/feat/I98-top-level-aggs branch from 93b4ba3 to 66ae0a4 Compare July 11, 2022 14:55

AndrewSisley force-pushed the sisley/feat/I98-top-level-aggs branch 2 times, most recently from bc2612f to a2f7807 Compare July 11, 2022 15:45

AndrewSisley added 2 commits July 11, 2022 11:49

Remove legacy code

4d75932

This has been incorrect for a while, and will cause problems for top-level aggregates

Add support for top level aggregates

7ed7bc1

AndrewSisley force-pushed the sisley/feat/I98-top-level-aggs branch from a2f7807 to 7ed7bc1 Compare July 11, 2022 15:50

AndrewSisley merged commit 46ec563 into develop Jul 11, 2022

AndrewSisley deleted the sisley/feat/I98-top-level-aggs branch July 11, 2022 16:04

	aggregateTarget.Type.(*gql.InputObject).AddFieldConfig("filter", expandedField)
	aggregateTarget.Type.(*gql.InputObject).AddFieldConfig(parserTypes.FilterClause, expandedField)

feat: Add support for top level aggregates #594

feat: Add support for top level aggregates #594

Conversation

AndrewSisley commented Jul 6, 2022 • edited Loading

Relevant issue(s)

Description

Tasks

How has this been tested?

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

shahzadlone Jul 6, 2022 • 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

shahzadlone left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewSisley Jul 7, 2022 • 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

AndrewSisley Jul 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewSisley Jul 7, 2022 • 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

AndrewSisley Jul 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shahzadlone left a comment

Choose a reason for hiding this comment

AndrewSisley commented Jul 7, 2022

Choose a reason for hiding this comment

fredcarle left a comment

Choose a reason for hiding this comment

fredcarle Jul 7, 2022 • edited Loading

Choose a reason for hiding this comment

AndrewSisley Jul 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewSisley Jul 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsimnz 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewSisley commented Jul 11, 2022 • edited Loading

AndrewSisley commented Jul 6, 2022 •

edited

Loading

shahzadlone Jul 6, 2022 •

edited

Loading

AndrewSisley Jul 7, 2022 •

edited

Loading

AndrewSisley Jul 7, 2022 •

edited

Loading

AndrewSisley Jul 7, 2022 •

edited

Loading

AndrewSisley Jul 7, 2022 •

edited

Loading

fredcarle Jul 7, 2022 •

edited

Loading

AndrewSisley Jul 7, 2022 •

edited

Loading

AndrewSisley Jul 7, 2022 •

edited

Loading

AndrewSisley commented Jul 11, 2022 •

edited

Loading