feat: Add sort support for numeric aggregates #786

AndrewSisley · 2022-09-09T19:39:32Z

Relevant issue(s)

Resolves #381

Description

Adds sort support for numeric aggregates. Does not add it for count, as the sort order cannot impact the count result. Also adds a Sort enumerable type.

Will suffer from the same issue as #588 when ordering via a related item (unless matching join is rendered), I'm noting this in that issue so that they can be fixed together.

How has this been tested?

(replace) Describe the tests performed to verify the changes. Provide instructions to reproduce them.

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

Debian Linux

codecov · 2022-09-09T20:02:12Z

Codecov Report

Merging #786 (926c38e) into develop (0ad47d5) will increase coverage by 0.25%.
The diff coverage is 88.04%.

@@             Coverage Diff             @@
##           develop     #786      +/-   ##
===========================================
+ Coverage    59.28%   59.53%   +0.25%     
===========================================
  Files          153      154       +1     
  Lines        17035    17181     +146     
===========================================
+ Hits         10099    10229     +130     
- Misses        6019     6030      +11     
- Partials       917      922       +5

Impacted Files	Coverage Δ
query/graphql/schema/generate.go	`82.58% <70.00%> (+0.14%)`	⬆️
query/graphql/mapper/mapper.go	`85.21% <83.75%> (-0.33%)`	⬇️
core/enumerable/sort.go	`92.50% <92.50%> (ø)`
query/graphql/planner/sum.go	`87.33% <100.00%> (+1.76%)`	⬆️

shahzadlone · 2022-09-12T10:42:38Z

core/enumerable/sort.go

+type enumerableSort[T any] struct {
+	source   Enumerable[T]
+	less     func(T, T) bool
+	capacity int


question: Do you actually mean capacity here or length? The Sort function would take capacity as the of len(source), and then Next() would reserve the capacity for the result to that length (using the capacity, which is understandable).

suggestion: Removal, if len(source) is sufficient.

The length of source is unknown during enumeration (due to filters, limits etc) - it is a capacity (a fairly generous capacity potentially).

Is capacity not always == len(source) ?

You can't do len(source). Enumerable[T] is an interface.

An interface, and the result of it's enumeration would be unknown until it has been enumerated - would need to use the Count function instead

core/enumerable/sort.go

query/graphql/mapper/mapper.go

shahzadlone · 2022-09-12T11:33:59Z

query/graphql/mapper/mapper.go

+	// The order in which items should be aggregated. Affects results when used with
+	// limit. Optional.
+	order *parserTypes.OrderBy


thought: Outside the scope of this PR, but maybe we can organize these types in one place. Right now Limit parser.Filter and parserTypes.OrderBy are in 3 different places.

partially agreed :) is a bit odd order and filter are in different places, but they are different from limit as they have to be parsed and we use the same parsing logic as the parser here. I have a vague memory of thinking it possible to remove that, but it has since left my head (may be noted in a comment somewhere)

shahzadlone · 2022-09-12T11:45:12Z

query/graphql/mapper/mapper.go

+	if other == nil {
+		return o == nil
+	}
+
+	if len(o.Conditions) != len(other.Conditions) {
+		return false
+	}
+
+	for i, conditionA := range o.Conditions {
+		conditionB := other.Conditions[i]
+		if conditionA.Direction != conditionB.Direction {
+			return false
+		}
+
+		if len(conditionA.FieldIndexes) != len(conditionB.FieldIndexes) {
+			return false
+		}
+
+		for j, fieldIndexA := range conditionA.FieldIndexes {
+			fieldIndexB := conditionB.FieldIndexes[j]
+			if fieldIndexA != fieldIndexB {
+				return false
+			}
+		}
+	}
+
+	return true


suggestion: Perhaps some coverage for these lines as there is no test coverage at the moment.

Cheers, I'll add a test or two

Add tests for order matching

line 1005 is dead at the moment due to #588

other than the 1005 block are rest being hit now? (idk why codecov doesn't show them as hitting)

I'm context switching too much, and this is performance-only code (no tests will fail). I missed a test case for sure, but there is an existing that should cover more than this - will look more into this

Why is rendered test not hitting most of this

Rendered or average test with orderby different field

^Might be related to Shahzad's current ticket, leave this for a few hours and see how he gets on fixing it as it might be quick

^Might be related to Shahzad's current ticket, leave this for a few hours and see how he gets on fixing it as it might be quick

I was able to do the fix. 1st and 2nd commit in https://github.com/sourcenetwork/defradb/pull/774/commits, but unsure if that helps this.

shahzadlone · 2022-09-12T11:54:08Z

query/graphql/schema/generate.go

+		if err != nil {
+			return nil, err
+		}


question: So all or nothing kind of deal. Even if one errors out we just return error. Do you know how big g.typeDefs generally is? wondering if is waste of time or worthwhile to perhaps reserve generatedQueryFields to it's size.

I am confused by what you mean here, we need to generate these and we don't want things to partially succeed and leave users with a broken database.

Sorry should have been clearer. We either return all generated fields or none if an error occurs (the way it should be). But. what I was asking was do you see any benefit to reserving generatedQueryFields to the capacity of g.typeDefs ?

Ah, got you. I don't think worrying about that is worth the bother - this is run on schema update, not query so performance (of that scale) doesnt really matter

shahzadlone · 2022-09-12T12:00:52Z

tests/integration/query/inline_array/with_sum_limit_offset_order_test.go

+func TestQueryInlineNillableIntegerArrayWithSumWithOffsetWithLimitWithOrderAsc(t *testing.T) {
+	test := testUtils.QueryTestCase{
+		Description: "Simple inline array, ordered offsetted limited sum of integer array",
+		Query: `query {
+					users {
+						Name
+						_sum(TestScores: {offset: 1, limit: 3, order: ASC})
+					}
+				}`,
+		Docs: map[int][]string{
+			0: {
+				`{
+					"Name": "Shahzad",
+					"TestScores": [null, 2, 5, 1, 0, 7]
+				}`,
+			},
+		},
+		Results: []map[string]interface{}{
+			{
+				"Name": "Shahzad",
+				// 0 + 1 + 2
+				"_sum": int64(3),
+			},
+		},
+	}
+


suggestion: Perhaps another test where {offset: 0, limit: 3, order: ASC} and "TestScores": [null, 0, 7]

Early morning me is struggling to see why. Why?

I might move null from the zero index though, to make sure sort is done before offset

test tweak

I wanted to see what happens if null is there and limit needs to fill one more number (i.e. limit > total-non-nil-numbers.

Yes sir good idea on moving null to another index.

question: when you do sort how do you handle nulls (I might have seen this somewhere on this PR but slipped my mind). Like are they less than every other number, greater than or just ignored.

I wanted to see what happens if null is there and limit needs to fill one more number

IMO that is not an order related test, and the limit tests already cover limits exceeding the length

when you do sort how do you handle nulls

Is handled in the less function, in sum.go

shahzadlone

LGTM! (assuming you will be adding the appropriate tests).

fredcarle

LGTM with a single thought. Anything else was brought up by Shahzad.

query/graphql/mapper/mapper.go

* Add sort enumerable * Move resolution of object input types to before resolution of aggregates * Add order support for inline array numeric aggs * Add order support for relation numeric aggs

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

AndrewSisley added this to the DefraDB v0.3.1 milestone Sep 9, 2022

AndrewSisley requested a review from a team September 9, 2022 19:39

AndrewSisley self-assigned this Sep 9, 2022

AndrewSisley mentioned this pull request Sep 9, 2022

Request where parent is ordered by child with no selection of child causes a runtime error #588

Closed

AndrewSisley force-pushed the sisley/feat/I381-agg-sort branch from 9c11a37 to f26a33c Compare September 9, 2022 19:57

shahzadlone reviewed Sep 12, 2022

View reviewed changes

core/enumerable/sort.go Show resolved Hide resolved

shahzadlone reviewed Sep 12, 2022

View reviewed changes

query/graphql/mapper/mapper.go Show resolved Hide resolved

shahzadlone reviewed Sep 12, 2022

View reviewed changes

AndrewSisley force-pushed the sisley/feat/I381-agg-sort branch 2 times, most recently from 0b2fd91 to 60e818b Compare September 12, 2022 14:26

AndrewSisley requested a review from shahzadlone September 12, 2022 14:26

shahzadlone approved these changes Sep 12, 2022

View reviewed changes

fredcarle approved these changes Sep 13, 2022

View reviewed changes

query/graphql/mapper/mapper.go Show resolved Hide resolved

AndrewSisley added 4 commits September 15, 2022 15:15

Add sort enumerable

30deea9

Move resolution of object input types to before resolution of aggregates

c6c8821

Add order support for inline array numeric aggs

4f61926

Add order support for relation numeric aggs

6f21897

AndrewSisley force-pushed the sisley/feat/I381-agg-sort branch from 60e818b to 6f21897 Compare September 15, 2022 19:15

FIXUP

926c38e

AndrewSisley merged commit 57a9394 into develop Sep 15, 2022

AndrewSisley deleted the sisley/feat/I381-agg-sort branch September 15, 2022 19:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add sort support for numeric aggregates #786

feat: Add sort support for numeric aggregates #786

AndrewSisley commented Sep 9, 2022

codecov bot commented Sep 9, 2022 •

edited

Loading

shahzadlone Sep 12, 2022

AndrewSisley Sep 12, 2022

shahzadlone Sep 12, 2022

fredcarle Sep 13, 2022

AndrewSisley Sep 13, 2022

shahzadlone Sep 12, 2022

AndrewSisley Sep 12, 2022

shahzadlone Sep 12, 2022

AndrewSisley Sep 12, 2022 •

edited

Loading

AndrewSisley Sep 12, 2022

shahzadlone Sep 12, 2022

AndrewSisley Sep 12, 2022 •

edited

Loading

AndrewSisley Sep 12, 2022

shahzadlone Sep 13, 2022 •

edited

Loading

AndrewSisley Sep 15, 2022

shahzadlone Sep 12, 2022

AndrewSisley Sep 12, 2022

shahzadlone Sep 12, 2022

AndrewSisley Sep 12, 2022

shahzadlone Sep 12, 2022

AndrewSisley Sep 12, 2022 •

edited

Loading

shahzadlone Sep 12, 2022

AndrewSisley Sep 12, 2022

shahzadlone left a comment •

edited

Loading

fredcarle left a comment

feat: Add sort support for numeric aggregates #786

feat: Add sort support for numeric aggregates #786

Conversation

AndrewSisley commented Sep 9, 2022

Relevant issue(s)

Description

How has this been tested?

codecov bot commented Sep 9, 2022 • edited Loading

Codecov Report

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 Sep 12, 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 Sep 12, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shahzadlone Sep 13, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewSisley Sep 12, 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 • edited Loading

Choose a reason for hiding this comment

fredcarle left a comment

Choose a reason for hiding this comment

codecov bot commented Sep 9, 2022 •

edited

Loading

AndrewSisley Sep 12, 2022 •

edited

Loading

AndrewSisley Sep 12, 2022 •

edited

Loading

shahzadlone Sep 13, 2022 •

edited

Loading

AndrewSisley Sep 12, 2022 •

edited

Loading

shahzadlone left a comment •

edited

Loading