Skip to content

Commit

Permalink
fix: Avoid populating order.ordering with empties. (#618)
Browse files Browse the repository at this point in the history
- Resolves: #589 

- Description: Hunch mentioned in last week's meeting was correct, it was indeed a `make` where it would populate the array with empties and then append being called on top of it. Also removed the function `IsEmpty()`, we used to check for empty elements previously which was introduced in #481.
  • Loading branch information
shahzadlone authored Jul 12, 2022
1 parent 42b3209 commit eca5324
Show file tree
Hide file tree
Showing 4 changed files with 10 additions and 22 deletions.
16 changes: 8 additions & 8 deletions query/graphql/mapper/mapper.go
Original file line number Diff line number Diff line change
Expand Up @@ -798,27 +798,27 @@ func toOrderBy(source *parserTypes.OrderBy, mapping *core.DocumentMapping) *Orde
}

conditions := make([]OrderCondition, len(source.Conditions))
for _, condition := range source.Conditions {
for conditionIndex, condition := range source.Conditions {
fields := strings.Split(condition.Field, ".")
fieldIndexes := make([]int, len(fields))
currentMapping := mapping
for i, field := range fields {
for fieldIndex, field := range 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.
fieldIndex := currentMapping.FirstIndexOfName(field)
fieldIndexes[i] = fieldIndex
if i != len(fields)-1 {
firstFieldIndex := currentMapping.FirstIndexOfName(field)
fieldIndexes[fieldIndex] = firstFieldIndex
if fieldIndex != len(fields)-1 {
// no need to do this for the last (and will panic)
currentMapping = &currentMapping.ChildMappings[fieldIndex]
currentMapping = &currentMapping.ChildMappings[firstFieldIndex]
}
}

conditions = append(conditions, OrderCondition{
conditions[conditionIndex] = OrderCondition{
FieldIndexes: fieldIndexes,
Direction: SortDirection(condition.Direction),
})
}
}

return &OrderBy{
Expand Down
7 changes: 0 additions & 7 deletions query/graphql/mapper/targetable.go
Original file line number Diff line number Diff line change
Expand Up @@ -106,13 +106,6 @@ type OrderCondition struct {
Direction SortDirection
}

func (oc OrderCondition) IsEmpty() bool {
if oc.Direction == "" && len(oc.FieldIndexes) == 0 {
return true
}
return false
}

type OrderBy struct {
Conditions []OrderCondition
}
Expand Down
5 changes: 0 additions & 5 deletions query/graphql/planner/order.go
Original file line number Diff line number Diff line change
Expand Up @@ -103,11 +103,6 @@ func (n *orderNode) Explain() (map[string]interface{}, error) {
orderings := []map[string]interface{}{}

for _, element := range n.ordering {
// Skip all empty elements.
if element.IsEmpty() {
continue
}

// Build the list containing the corresponding names of all the indexes.
fieldNames := []string{}
for _, fieldIndex := range element.FieldIndexes {
Expand Down
4 changes: 2 additions & 2 deletions query/graphql/planner/select.go
Original file line number Diff line number Diff line change
Expand Up @@ -417,7 +417,7 @@ func (p *Planner) Select(parsed *mapper.Select) (planNode, error) {
docMapper: docMapper{&parsed.DocumentMapping},
}
limit := parsed.Limit
order := parsed.OrderBy
orderBy := parsed.OrderBy
groupBy := parsed.GroupBy

aggregates, err := s.initSource()
Expand All @@ -435,7 +435,7 @@ func (p *Planner) Select(parsed *mapper.Select) (planNode, error) {
return nil, err
}

orderPlan, err := p.OrderBy(parsed, order)
orderPlan, err := p.OrderBy(parsed, orderBy)
if err != nil {
return nil, err
}
Expand Down

0 comments on commit eca5324

Please sign in to comment.