Skip to content
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 ability to explain groupNode and it's attribute(s). #641

Merged
merged 4 commits into from
Jul 23, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions query/graphql/mapper/mapper.go
Original file line number Diff line number Diff line change
Expand Up @@ -780,18 +780,22 @@ func toGroupBy(source *parserTypes.GroupBy, mapping *core.DocumentMapping) *Grou
return nil
}

indexes := make([]int, len(source.Fields))
fields := make([]Field, len(source.Fields))
for i, fieldName := range source.Fields {
// If there are multiple properties of the same name we can just take the first as
// we have no other reasonable way of identifying which property they mean if multiple
// consumer specified requestables are available. Aggregate dependencies should not
// impact this as they are added after selects.
key := mapping.FirstIndexOfName(fieldName)
indexes[i] = key

fields[i] = Field{
Index: key,
Name: fieldName,
}
}

return &GroupBy{
FieldIndexes: indexes,
Fields: fields,
}
}

Expand Down
2 changes: 1 addition & 1 deletion query/graphql/mapper/targetable.go
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ type Limit struct {
// GroupBy represents a grouping instruction on a query.
type GroupBy struct {
// The indexes of fields by which documents should be grouped. Ordered.
FieldIndexes []int
Fields []Field
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion(blocking): I'm very hesitant to make a change like this without input from Andy, just for the sake of the explain system.

As I understand it, the goal here is to be able to efficiently get the corresponding FieldName when doing the GroupBy explain, but this change affects a lot of other places (as you know, since you had to update them all).

But, as I understand it, the mapper already has a utility to convert index into FieldName without needing to make a change like this.

eg: n.documentMapping.TryToFindNameFromIndex(index). Which you are already using for the order fields. Is it not possible to use this utility as well for the groupby fields? Which would mean you don't have to make this change?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair point John. I do think however that the codes reads a bit nicer with this change. For example, this:

for _, keyField := range keyFields {

reads better than this:

for _, keyField := range keyIndexes {

Copy link
Member

@jsimnz jsimnz Jul 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair point John. I do think however that the codes reads a bit nicer with this change. For example, this:

There's certainly more than a few rough edges w.r.t the mapper system, which are being tracked/tackled in #606.

The explain PRs should make an effort to not change core planner functionality, if it does need a refactor for something, it should be done in a seperate PR.

For this specific PR, as far as I can tall, the TryToFindNameFromIndex seems like it should be sufficient to circumvent this larger change.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't really change the functionality though. It merely adds information to a variable (from []int to []struct) and changes its name. I think of it as if it were already a struct, it would just be adding a struct field.

Copy link
Member Author

@shahzadlone shahzadlone Jul 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You make a fair point, however this is basically just adding an additional field (is nicer IMO). It's nice to have a guarantee of field index always having a corresponding name.

I wrote the TryToFindNameFromIndex function for finding indexes of ordering elements in orderNode. Using that function wouldn't guarantee that the field name exists (even though it should), and obviously is not as nice doing lookup if we don't have to.

One other thing I could do to reduce the changes (however would still prefer this approach better), is I could still pass in only the list of indexes into the functions whose signatures were changed to mapper.Field[] from int[].

LMK what you think, at the end of the day this is a very safe change as it's just tagging an additional field, and the previous field stays there as it was before. I would be concerned if I had removed a field haha

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd still prefer to try to minimize changes beyond utility funcs for explain PRs.

Although technically this is a small change ([]int to []struct) as Fred pointed out, it does sprawl all over the implementation.

Its OK for now, but lets try to minimize this stuff in the future.

}

type SortDirection string
Expand Down
27 changes: 16 additions & 11 deletions query/graphql/planner/arbitrary_join.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import (
"strings"

"github.com/sourcenetwork/defradb/core"
"github.com/sourcenetwork/defradb/query/graphql/mapper"
)

// A data-source that may yield child items, parent items, or both depending on configuration
Expand Down Expand Up @@ -111,7 +112,7 @@ func (n *dataSource) Source() planNode {
}

func (source *dataSource) mergeParent(
keyIndexes []int,
keyFields []mapper.Field,
destination *orderedMap,
childIndexes []int,
) (bool, error) {
Expand All @@ -132,15 +133,15 @@ func (source *dataSource) mergeParent(
}

value := source.parentSource.Value()
key := generateKey(value, keyIndexes)
key := generateKey(value, keyFields)

destination.mergeParent(key, childIndexes, value)

return true, nil
}

func (source *dataSource) appendChild(
keyIndexes []int,
keyFields []mapper.Field,
valuesByKey *orderedMap,
mapping *core.DocumentMapping,
) (bool, error) {
Expand All @@ -165,14 +166,18 @@ func (source *dataSource) appendChild(
// the same order - we need to treat it as a new item, regenerating the key and potentially caching
// it without yet receiving the parent-level details
value := source.childSource.Value()
key := generateKey(value, keyIndexes)
key := generateKey(value, keyFields)

valuesByKey.appendChild(key, source.childIndex, value, mapping)

return true, nil
}

func join(sources []*dataSource, keyIndexes []int, mapping *core.DocumentMapping) (*orderedMap, error) {
func join(
sources []*dataSource,
keyFields []mapper.Field,
mapping *core.DocumentMapping,
) (*orderedMap, error) {
result := orderedMap{
values: []core.Doc{},
indexesByKey: map[string]int{},
Expand All @@ -190,14 +195,14 @@ func join(sources []*dataSource, keyIndexes []int, mapping *core.DocumentMapping

for hasNextParent || hasNextChild {
if hasNextParent {
hasNextParent, err = source.mergeParent(keyIndexes, &result, childIndexes)
hasNextParent, err = source.mergeParent(keyFields, &result, childIndexes)
if err != nil {
return nil, err
}
}

if hasNextChild {
hasNextChild, err = source.appendChild(keyIndexes, &result, mapping)
hasNextChild, err = source.appendChild(keyFields, &result, mapping)
if err != nil {
return nil, err
}
Expand All @@ -208,11 +213,11 @@ func join(sources []*dataSource, keyIndexes []int, mapping *core.DocumentMapping
return &result, nil
}

func generateKey(doc core.Doc, keyIndexes []int) string {
func generateKey(doc core.Doc, keyFields []mapper.Field) string {
keyBuilder := strings.Builder{}
for _, keyField := range keyIndexes {
keyBuilder.WriteString(fmt.Sprint(keyField))
keyBuilder.WriteString(fmt.Sprintf("_%v_", doc.Fields[keyField]))
for _, keyField := range keyFields {
keyBuilder.WriteString(fmt.Sprint(keyField.Index))
keyBuilder.WriteString(fmt.Sprintf("_%v_", doc.Fields[keyField.Index]))
}
return keyBuilder.String()
}
Expand Down
1 change: 1 addition & 0 deletions query/graphql/planner/explain.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ var (
_ explainablePlanNode = (*createNode)(nil)
_ explainablePlanNode = (*dagScanNode)(nil)
_ explainablePlanNode = (*deleteNode)(nil)
_ explainablePlanNode = (*groupNode)(nil)
_ explainablePlanNode = (*hardLimitNode)(nil)
_ explainablePlanNode = (*orderNode)(nil)
_ explainablePlanNode = (*renderLimitNode)(nil)
Expand Down
125 changes: 117 additions & 8 deletions query/graphql/planner/group.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@
package planner

import (
"fmt"

"github.com/sourcenetwork/defradb/core"
"github.com/sourcenetwork/defradb/query/graphql/mapper"

Expand All @@ -29,7 +31,7 @@ type groupNode struct {

// The fields to group by - this must be an ordered collection and
// will include any parent group-by fields (if any)
groupByFieldIndexes []int
groupByFields []mapper.Field

// The data sources that this node will draw data from.
dataSources []*dataSource
Expand Down Expand Up @@ -61,17 +63,17 @@ func (p *Planner) GroupBy(n *mapper.GroupBy, parsed *mapper.Select, childSelects
if childSelect.GroupBy != nil {
// group by fields have to be propagated downwards to ensure correct sub-grouping, otherwise child
// groups will only group on the fields they explicitly reference
childSelect.GroupBy.FieldIndexes = append(childSelect.GroupBy.FieldIndexes, n.FieldIndexes...)
childSelect.GroupBy.Fields = append(childSelect.GroupBy.Fields, n.Fields...)
}
dataSources = append(dataSources, newDataSource(childSelect.Index))
}

groupNodeObj := groupNode{
p: p,
childSelects: childSelects,
groupByFieldIndexes: n.FieldIndexes,
dataSources: dataSources,
docMapper: docMapper{&parsed.DocumentMapping},
p: p,
childSelects: childSelects,
groupByFields: n.Fields,
dataSources: dataSources,
docMapper: docMapper{&parsed.DocumentMapping},
}
return &groupNodeObj, nil
}
Expand Down Expand Up @@ -127,7 +129,7 @@ func (n *groupNode) Source() planNode { return n.dataSources[0].Source() }

func (n *groupNode) Next() (bool, error) {
if n.values == nil {
values, err := join(n.dataSources, n.groupByFieldIndexes, n.documentMapping)
values, err := join(n.dataSources, n.groupByFields, n.documentMapping)
if err != nil {
return false, err
}
Expand Down Expand Up @@ -169,3 +171,110 @@ func (n *groupNode) Next() (bool, error) {

return false, nil
}

// Explain method returns a map containing all attributes of this node that
// are to be explained, subscribes / opts-in this node to be an explainablePlanNode.
func (n *groupNode) Explain() (map[string]interface{}, error) {
explainerMap := map[string]interface{}{}

// Get the parent level groupBy attribute(s).
groupByFields := []string{}
for _, field := range n.groupByFields {
groupByFields = append(
groupByFields,
field.Name,
)
}
explainerMap["groupByFields"] = groupByFields

// Get the inner group (child) selection attribute(s).
if len(n.childSelects) == 0 {
explainerMap["childSelects"] = nil
} else {
childSelects := make([]map[string]interface{}, 0, len(n.childSelects))
for _, child := range n.childSelects {
if child == nil {
continue
}

childExplainGraph := map[string]interface{}{}

childExplainGraph[collectionNameLabel] = child.CollectionName

c := child.Targetable

// Get targetable attribute(s) of this child.

if c.DocKeys.HasValue {
childExplainGraph["docKeys"] = c.DocKeys.Value
} else {
childExplainGraph["docKeys"] = nil
}

if c.Filter == nil || c.Filter.ExternalConditions == nil {
childExplainGraph[filterLabel] = nil
} else {
childExplainGraph[filterLabel] = c.Filter.ExternalConditions
}

if c.Limit != nil {
childExplainGraph[limitLabel] = map[string]interface{}{
limitLabel: c.Limit.Limit,
offsetLabel: c.Limit.Offset,
}
} else {
childExplainGraph[limitLabel] = nil
}

if c.OrderBy != nil {
innerOrderings := []map[string]interface{}{}
for _, condition := range c.OrderBy.Conditions {
orderFieldNames := []string{}

for _, orderFieldIndex := range condition.FieldIndexes {
// Try to find the name of this index.
fieldName, found := n.documentMapping.TryToFindNameFromIndex(orderFieldIndex)
if !found {
return nil, fmt.Errorf(
"No order field name (for grouping) was found for index =%d",
orderFieldIndex,
)
}

orderFieldNames = append(orderFieldNames, fieldName)
}
// Put it all together for this order element.
innerOrderings = append(innerOrderings,
map[string]interface{}{
"fields": orderFieldNames,
"direction": string(condition.Direction),
},
)
}

childExplainGraph["orderBy"] = innerOrderings
} else {
childExplainGraph["orderBy"] = nil
}

if c.GroupBy != nil {
innerGroupByFields := []string{}
for _, fieldOfChildGroupBy := range c.GroupBy.Fields {
innerGroupByFields = append(
innerGroupByFields,
fieldOfChildGroupBy.Name,
)
}
childExplainGraph["groupBy"] = innerGroupByFields
} else {
childExplainGraph["groupBy"] = nil
}

childSelects = append(childSelects, childExplainGraph)
}

explainerMap["childSelects"] = childSelects
}

return explainerMap, nil
}
Loading