feat: Min and max numerical aggregates #3078

nasdf · 2024-09-27T00:08:57Z

Relevant issue(s)

Resolves #2978

Description

This PR adds support for two new query aggregates min and max.

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?

Added integration tests

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

MacOS

codecov · 2024-09-27T01:17:55Z

Codecov Report

Attention: Patch coverage is 93.67681% with 27 lines in your changes missing coverage. Please review.

Project coverage is 79.85%. Comparing base (c986312) to head (3cff29d).
Report is 1 commits behind head on develop.

Files with missing lines	Patch %	Lines
internal/planner/max.go	92.57%	8 Missing and 3 partials ⚠️
internal/planner/min.go	92.57%	8 Missing and 3 partials ⚠️
internal/request/graphql/schema/generate.go	93.65%	2 Missing and 2 partials ⚠️
internal/planner/sum.go	95.83%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #3078      +/-   ##
===========================================
+ Coverage    79.67%   79.85%   +0.18%     
===========================================
  Files          348      351       +3     
  Lines        27014    27358     +344     
===========================================
+ Hits         21522    21845     +323     
- Misses        3962     3975      +13     
- Partials      1530     1538       +8

Flag	Coverage Δ
all-tests	`79.85% <93.68%> (+0.18%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
internal/planner/aggregate.go	`100.00% <100.00%> (ø)`
internal/planner/explain.go	`60.87% <ø> (ø)`
internal/planner/select.go	`85.36% <100.00%> (+0.25%)`	⬆️
internal/planner/top.go	`70.63% <100.00%> (+0.96%)`	⬆️
internal/planner/sum.go	`87.72% <95.83%> (-1.94%)`	⬇️
internal/request/graphql/schema/generate.go	`88.15% <93.65%> (+0.04%)`	⬆️
internal/planner/max.go	`92.57% <92.57%> (ø)`
internal/planner/min.go	`92.57% <92.57%> (ø)`

... and 14 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c986312...3cff29d. Read the comment docs.

fredcarle · 2024-09-27T02:07:59Z

tests/integration/query/simple/with_group_average_max_test.go

+}
+
+// Note: this test should follow a different code path to `_avg` on it's own
+// utilising the existing `_min` node instead of adding a new one.  This test cannot


typo: _max

AndrewSisley

I want too review this but will wait until the tests are passing, please re-request my review when they are and I'll get right on this :)

nasdf · 2024-09-27T17:51:56Z

I want too review this but will wait until the tests are passing, please re-request my review when they are and I'll get right on this :)

Should be all fixed now. There was an issue with casting max value floats to ints which I think may exist in the sum aggregate where I copied the code from.

AndrewSisley

Looks good to me, just some minor stuff to sort out first IMO, and the below question.

question: It does not lok like you have added max/min to non-numeric types such as DateTime, String, Boolean, etc, and the gql related code does not look like it will facilitate the introduction of that support - was this an oversight or do you have a plan on how to introduce it later? I am concerned that doing so will be a breaking change to the GQL syntax due to the return type.

AndrewSisley · 2024-09-27T17:54:59Z

internal/planner/aggregate.go

+	return a.Value() < b.Value()
+}
+
+func reverse[T any](original func(T, T) bool) func(T, T) bool {


suggestion: inverse might be a better name? I'm not sure though. Maybe a line of documentation would be handy as reverse suggests this is order related, but the implementation and signature does not suggest it is limited to that.

AndrewSisley · 2024-09-27T18:00:31Z

internal/planner/max.go

+		if err != nil {
+			return nil, err
+		}
+		// If one source property is a float, the result will be a float - no need to check the rest


suggestion: This comment looks copy pasted and doesn't quite make sense for min/max If one source property is a float, the result will be a float is not really correct, although this is a valid enough way to determine return-type. I suggest tweaking the comment slightly for min/max.

I'm not seeing which part is incorrect.

If you had:

[]any{ float(3.5), int(9999), }

max would be an int. Summing/averaging the set would always result in a float, but min/maxing the set would not. The comment makes sense for other aggregates, but not for min/max.

Okay I see what you mean. I've refactored the logic so it actually returns the correct type now.

Correct me if I'm wrong but I don't think we have any operations that would allow us to test mixed type aggregates.

Correct me if I'm wrong but I don't think we have any operations that would allow us to test mixed type aggregates.

No you are correct, I'm only commenting on the comment, the code/tests are all good IMO :)

I've refactored the logic so it actually returns the correct type now.

Nice, thanks!

AndrewSisley · 2024-09-27T18:02:35Z

internal/planner/max.go

+func (n *maxNode) Kind() string           { return "maxNode" }
+func (n *maxNode) Init() error            { return n.plan.Init() }
+func (n *maxNode) Start() error           { return n.plan.Start() }
+func (n *maxNode) Spans(spans core.Spans) { n.plan.Spans(spans) }


nitpick: This should probably be covered by tests, I think having a docID param on the aggregate should hit this, otherwise it could be deleted/broken and it wouldn't break the build.

added a test that should cover it

AndrewSisley · 2024-09-27T18:05:19Z

internal/planner/max.go

+	max := -math.MaxFloat64
+	for _, source := range n.aggregateMapping {
+		child := n.currentValue.Fields[source.Index]
+		collectionMax := -math.MaxFloat64


todo: The code in this for loop looks like it will return -math.MaxFloat64 if the target is empty/nil, have you tested this, and is it the behaviour we want (nil might be better, otherwise an empty set will have the same result as an item with a literal -math.MaxFloat64?)?

refactored to return nil on empty values

AndrewSisley · 2024-09-27T18:08:58Z

internal/planner/max.go

+	max := -math.MaxFloat64
+	for _, source := range n.aggregateMapping {
+		child := n.currentValue.Fields[source.Index]
+		collectionMax := -math.MaxFloat64


suggstion: You seem to alternate between using ``-math.MaxFloat64and-math.MinFloat64`, this makes spotting its significance/other-refs harder and adds a easily avoided little confusion - I suggest picking one and using it throughout.

refactored, this is no longer applicable

AndrewSisley · 2024-09-27T18:11:00Z

internal/planner/max.go

+	plan planNode
+
+	isFloat           bool
+	virtualFieldIndex int


suggestion: I'm sorry about the bother, because you are copying my undocumented work in the other aggregates, but I think it would be worth a line of documentation on virtualFieldIndex on these nodes, its not very clear.

fredcarle

Nice work! I like what you did with the aggregate generics and thanks for adding all those tests. Just need to resole Andy's comments before merge.

nasdf · 2024-09-30T16:12:40Z

Looks good to me, just some minor stuff to sort out first IMO, and the below question.

question: It does not lok like you have added max/min to non-numeric types such as DateTime, String, Boolean, etc, and the gql related code does not look like it will facilitate the introduction of that support - was this an oversight or do you have a plan on how to introduce it later? I am concerned that doing so will be a breaking change to the GQL syntax due to the return type.

I'd prefer to add support for non-numerical types in a new PR. I think it will require a good amount of changes to the aggregate types and I'm not worried about a breaking GQL change if it's needed.

internal/planner/max.go

internal/planner/min.go

tests/integration/explain.go

tests/integration/explain/debug/top_with_max_test.go

tests/integration/explain/debug/with_max_join_test.go

AndrewSisley · 2024-09-30T17:33:16Z

I think it will require a good amount of changes to the aggregate types

Which aggregate types? The new types added in this PR? Or some existing stuff to?

I am concerned that you will just end up re-writing a lot the new stuff added in this PR to handle (IMO) a pretty core sub-feature (DateTime).

Note: I am alright with you doing that in another PR is everyone else is, especially given this has already been reviewed now (and so a lot of the extra time has already been spent), but my preference would be to get the long-term syntax (and internal code structure) correct first.

AndrewSisley · 2024-09-30T18:19:24Z

internal/planner/max.go

+	var max *float64
+	for _, source := range n.aggregateMapping {
+		child := n.currentValue.Fields[source.Index]
+		var collectionMax *float64


todo: This is incorrect for large integers, you cannot convert a very big int to a float and then back again and expect it to have the same value.

suggestion: Add failing tests before fixing this (if you can, you might struggle to set an Int to Int64MaxVal atm via our public clients, as GQL is technically only 32bit Ints)

e.g. the below will result in a negative number:

a := math.MaxInt64 - 1 println(fmt.Sprint(a)) b := float64(a) c := int64(b) panic(fmt.Sprint(c))

refactored to use big.Float

did you try to test this?

I've added tests for int max value

nasdf · 2024-09-30T22:31:23Z

Which aggregate types? The new types added in this PR? Or some existing stuff to?

I haven't thought through all of it yet, but because the aggregates are expected to return a float and can then be used as input into other aggregates, there will be issues with an aggregate that can return a union type of String, DateTime, etc.

I am concerned that you will just end up re-writing a lot the new stuff added in this PR to handle (IMO) a pretty core sub-feature (DateTime).

The planner code is very generic. I'm not too worried about this.

shahzadlone

LGTM thanks for doing the suggestions

AndrewSisley · 2024-10-01T14:11:13Z

there will be issues with an aggregate that can return a union type of String, DateTime, etc.

Yes, that is why I am concerned you are painting the syntax into a corner and we will need to make an avoidable breaking change in the (possibly near) future in order to correct this problem. The breaking change will require us to change our tests, and users to change their code.

I do not think this is a good thing to do, but will not block the PR over it. Please resolve the cast problem/test request before merge though.

.

nasdf · 2024-10-01T15:51:46Z

there will be issues with an aggregate that can return a union type of String, DateTime, etc.

Yes, that is why I am concerned you are painting the syntax into a corner and we will need to make an avoidable breaking change in the (possibly near) future in order to correct this problem. The breaking change will require us to change our tests, and users to change their code.

I do not think this is a good thing to do, but will not block the PR over it. Please resolve the cast problem/test request before merge though.

The GQL type system won't allow for an aggregate that returns a union to be used as input in another aggregate.

I think the best approach will be to add new aggregates: _minDateTime, _maxString, etc.

This shouldn't result in any breaking changes to the existing aggregates.

AndrewSisley · 2024-10-01T15:54:15Z

I think the best approach will be to add new aggregates: _minDateTime, _maxString, etc.

This shouldn't result in any breaking changes to the existing aggregates.

Only if we dont consider it weird that we have _min and not _minFloat, if that is the long term syntax we want I would prefer that all the types be consistent.

AndrewSisley

Talked over the long term plan in discord, is fully compatible with this PR - thanks for all of this Keenan, and for the discussion :)

islamaliev · 2024-10-17T11:17:36Z

bug bash result: all looks good.

nasdf added 3 commits September 25, 2024 16:10

implement min max aggregates

35f7cef

rename to min max

621e2c3

add tests

d3ada97

nasdf added feature New feature or request area/query Related to the query component labels Sep 27, 2024

nasdf added this to the DefraDB v0.14 milestone Sep 27, 2024

nasdf self-assigned this Sep 27, 2024

nasdf requested a review from a team September 27, 2024 00:10

fix introspection tests

27f2e21

fredcarle reviewed Sep 27, 2024

View reviewed changes

AndrewSisley requested changes Sep 27, 2024

View reviewed changes

nasdf added 2 commits September 27, 2024 09:17

fix max value types in test results

63f062c

cast min max values correctly

58a5093

nasdf force-pushed the nasdf/feat/min-max-aggregate branch from db85fa3 to 58a5093 Compare September 27, 2024 17:22

nasdf requested a review from AndrewSisley September 27, 2024 17:52

AndrewSisley previously requested changes Sep 27, 2024

View reviewed changes

add inline array tests

87ecfd4

fredcarle approved these changes Sep 27, 2024

View reviewed changes

shahzadlone reviewed Sep 30, 2024

View reviewed changes

pr fixes

e779d9f

nasdf changed the title ~~feat: Min and max aggregates~~ feat: Min and max numerical aggregates Sep 30, 2024

AndrewSisley reviewed Sep 30, 2024

View reviewed changes

nasdf added 2 commits September 30, 2024 12:43

use big float

caa0862

add more explain tests

9a10cb1

nasdf requested a review from shahzadlone September 30, 2024 22:32

nasdf requested a review from AndrewSisley September 30, 2024 22:32

shahzadlone approved these changes Sep 30, 2024

View reviewed changes

add max value tests. refactor min max return type handling

4f8b79c

nasdf force-pushed the nasdf/feat/min-max-aggregate branch from 623e275 to 4f8b79c Compare October 1, 2024 15:59

Merge branch 'develop' into nasdf/feat/min-max-aggregate

3cff29d

AndrewSisley approved these changes Oct 2, 2024

View reviewed changes

nasdf merged commit e59f6d9 into sourcenetwork:develop Oct 2, 2024
43 checks passed

feat: Min and max numerical aggregates #3078

feat: Min and max numerical aggregates #3078

Conversation

nasdf commented Sep 27, 2024

Relevant issue(s)

Description

Tasks

How has this been tested?

codecov bot commented Sep 27, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewSisley left a comment

Choose a reason for hiding this comment

nasdf commented Sep 27, 2024 • edited Loading

AndrewSisley 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

AndrewSisley Sep 30, 2024 • 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

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

fredcarle left a comment

Choose a reason for hiding this comment

nasdf commented Sep 30, 2024

AndrewSisley commented Sep 30, 2024

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

nasdf commented Sep 30, 2024 • edited Loading

shahzadlone left a comment

Choose a reason for hiding this comment

AndrewSisley commented Oct 1, 2024

nasdf commented Oct 1, 2024

AndrewSisley commented Oct 1, 2024

AndrewSisley left a comment

Choose a reason for hiding this comment

islamaliev commented Oct 17, 2024

codecov bot commented Sep 27, 2024 •

edited

Loading

nasdf commented Sep 27, 2024 •

edited

Loading

AndrewSisley Sep 30, 2024 •

edited

Loading

nasdf commented Sep 30, 2024 •

edited

Loading