refactor: Rework sum and count nodes to make use of generics #757

AndrewSisley · 2022-08-23T21:41:22Z

Relevant issue(s)

Resolves #633

Description

Rework sum and count node type switches to make use of generics. Switches changed in this PR will be expanded with new cases shortly and I'd rather not write them twice :)

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

Debian Linux

codecov · 2022-08-23T21:43:34Z

Codecov Report

Merging #757 (e862f65) into develop (d59592f) will increase coverage by 0.04%.
The diff coverage is 76.92%.

@@             Coverage Diff             @@
##           develop     #757      +/-   ##
===========================================
+ Coverage    58.02%   58.07%   +0.04%     
===========================================
  Files          144      144              
  Lines        16593    16578      -15     
===========================================
- Hits          9628     9627       -1     
+ Misses        6052     6043       -9     
+ Partials       913      908       -5

Impacted Files	Coverage Δ
query/graphql/planner/count.go	`91.35% <72.72%> (+7.51%)`	⬆️
query/graphql/planner/sum.go	`80.95% <80.00%> (+3.37%)`	⬆️

shahzadlone · 2022-08-23T22:04:23Z

query/graphql/planner/count.go

@@ -99,62 +99,28 @@ func (n *countNode) Next() (bool, error) {
 		// v.Len will panic if v is not one of these types, we don't want it to panic
 		case reflect.Array, reflect.Chan, reflect.Map, reflect.Slice, reflect.String:
 			if source.Filter != nil {
+				var arrayCount int
+				var err error
 				switch array := property.(type) {


question: Wondering if there is a possibility to replace all the cases with:

case []core.Doc, []bool, []int64, []float64, []string: arrayCount, err = countItems(array, source.Filter) }

Type inference issue?

Yeah, some langs let you do that but I couldn't find a way in Go (including in the reflect package in case they'd added some generic funcs) - would have been nice

If not possible then don't worry about it. Still makes things a lot nicer than before.

shahzadlone · 2022-08-23T22:32:06Z

query/graphql/planner/count.go

@@ -165,4 +131,18 @@ func (n *countNode) Next() (bool, error) {
 	return true, nil
 }

+func countItems[T any](collection []T, filter *mapper.Filter) (int, error) {


suggestion: Define the type constraint rather than using any.

Suggested change

func countItems[T any](collection []T, filter *mapper.Filter) (int, error) {

type Countable interface {

core.Doc | ~bool | ~int64 | ~float64 | ~string

}

func countItems[T Countable](collection []T, filter *mapper.Filter) (int, error) {

Why? This function will work with any type of T

Is that the desired behavior? thought we specifically wanted to only handle the cases defined in the switches and any other element type is to be treated like any error or default case.

Yes it is desired. Why would you block future cases and couple the function to specific types?

Well it's normal to want to limit some functionality for a specific unsupported type, I was wondering if perhaps the reason we had explicit cases was maybe that we wanted to limit count/sum only to these cases. However, I understand now that we need the explicit cases because we need the actual type information instead of interface{} (through type switch).

As this is desired (and assuming also desired for the sum items), this is resolved.

jsimnz

Quick thought regarding type constraints to simplify sum

jsimnz · 2022-08-23T22:20:25Z

query/graphql/planner/sum.go

@@ -257,4 +242,19 @@ func (n *sumNode) Next() (bool, error) {
 	return true, nil
 }

+func sumItems[T any](collection []T, filter *mapper.Filter, toFloat func(T) float64) (float64, error) {


suggestion: Instead of using T any we can limit the type constraint to "numbers"

eg

import "golang.org/x/exp/constraints" type Number interface { constraints.Integer | constraint.Float }

Then you wouldn't have to do toFloat, then you can convert the final results sum into a float64 return float64(sum) and since sum is guranteed to be a number, the compiler will be happy with that conversion. So you can drop the argument dependancy entirely

toFloat also takes child documents, not just numbers - what you are suggesting is not possible without reverting 1 out of the 3 current cases

Lol, I wanted to scream when I read 'golang.org/x/exp/constraints' - Golang really really loves doing stuff that is known at compile time at runtime... Why would you want to box values of the generic type?? (I assume at least given that it is an interface)

Its not really box values of generic types, theres a new special syntax for type constraints that re-uses the interface semantics

ie

type Number interface { ~int }

is special type constraint attached to the named interface Number.

As for sub documents, wouldn't it make sense to do the subdoc stuff outside of sumItems? Since the toFloat function for []code.Doc does child-access stuff, and also uses index contexts outside of its scope. Seems simipler to seperate the two, and not have the toFloat func.

theres a new special syntax for type constraints that re-uses the interface semantics

Ah okay, and I checked - it doesn't seem to behave like a pointer either, bit odd to use interface for that though - looks like you can't use them as interfaces either (compiler blocks it)

As for sub documents, wouldn't it make sense to do the subdoc stuff outside of sumItems?

I didn't think so when I wrote it, I would rather have everything go through the same logic and keep as much as possible out of the switch. Would also prefer to leave the function unconstrained - reduces the risk of having to change it in the future

:) Now I'm hopeful for unions lol (if not rust-style enums)

jsimnz · 2022-08-23T22:23:54Z

query/graphql/planner/count.go

@@ -165,4 +131,18 @@ func (n *countNode) Next() (bool, error) {
 	return true, nil
 }

+func countItems[T any](collection []T, filter *mapper.Filter) (int, error) {


extreme nitpick: change parameter name from collection as its used a lot in a specific context (ie collections in the DB) :)

lol, fair enough, will do

rename param (and for sum)

shahzadlone

LGTM.

coversations appear resolved and I need this code in to progress on the more important task - any comments you may add here can be applied to that branch

…etwork#757) * Refactor count node to make use of generics * Refactor sum node to make use of generics

AndrewSisley added area/query Related to the query component refactor This issue specific to or requires *notable* refactoring of existing codebases and components action/no-benchmark Skips the action that runs the benchmark. labels Aug 23, 2022

AndrewSisley added this to the DefraDB v0.3.1 milestone Aug 23, 2022

AndrewSisley requested a review from a team August 23, 2022 21:41

AndrewSisley self-assigned this Aug 23, 2022

AndrewSisley changed the title ~~refactor: Rework sum and count node type switches to make use of generics~~ refactor: Rework sum and count nodes to make use of generics Aug 23, 2022

shahzadlone reviewed Aug 23, 2022

View reviewed changes

AndrewSisley requested a review from shahzadlone August 23, 2022 22:29

shahzadlone reviewed Aug 23, 2022

View reviewed changes

AndrewSisley requested a review from shahzadlone August 23, 2022 22:36

jsimnz previously requested changes Aug 23, 2022

View reviewed changes

AndrewSisley requested a review from jsimnz August 23, 2022 22:49

shahzadlone approved these changes Aug 23, 2022

View reviewed changes

AndrewSisley added 3 commits August 24, 2022 11:39

Refactor count node to make use of generics

03af7f8

Refactor sum node to make use of generics

3b33af4

FIXUP - PR requests

e862f65

AndrewSisley force-pushed the sisley/refactor/I633-sum-count-generic-refactor branch from 4a5714c to e862f65 Compare August 24, 2022 15:39

AndrewSisley merged commit aa92a70 into develop Aug 24, 2022

AndrewSisley deleted the sisley/refactor/I633-sum-count-generic-refactor branch August 24, 2022 15:51

shahzadlone pushed a commit to shahzadlone/defradb that referenced this pull request Feb 23, 2024

refactor: Rework sum and count nodes to make use of generics (sourcen…

ab43f74

…etwork#757) * Refactor count node to make use of generics * Refactor sum node to make use of generics

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: Rework sum and count nodes to make use of generics #757

refactor: Rework sum and count nodes to make use of generics #757

AndrewSisley commented Aug 23, 2022

codecov bot commented Aug 23, 2022 •

edited

Loading

shahzadlone Aug 23, 2022

AndrewSisley Aug 23, 2022

shahzadlone Aug 23, 2022

shahzadlone Aug 23, 2022

AndrewSisley Aug 23, 2022

shahzadlone Aug 23, 2022

AndrewSisley Aug 23, 2022

shahzadlone Aug 23, 2022

jsimnz left a comment

jsimnz Aug 23, 2022

AndrewSisley Aug 23, 2022

AndrewSisley Aug 23, 2022

jsimnz Aug 23, 2022

jsimnz Aug 23, 2022

AndrewSisley Aug 23, 2022

AndrewSisley Aug 23, 2022

jsimnz Aug 23, 2022

AndrewSisley Aug 23, 2022 •

edited

Loading

shahzadlone left a comment

-func countItems[T any](collection []T, filter *mapper.Filter) (int, error) {
+type Countable interface {
+	core.Doc | ~bool | ~int64 | ~float64 | ~string
+}
+func countItems[T Countable](collection []T, filter *mapper.Filter) (int, error) {

refactor: Rework sum and count nodes to make use of generics #757

refactor: Rework sum and count nodes to make use of generics #757

Conversation

AndrewSisley commented Aug 23, 2022

Relevant issue(s)

Description

codecov bot commented Aug 23, 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

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

AndrewSisley Aug 23, 2022 • edited Loading

Choose a reason for hiding this comment

shahzadlone left a comment

Choose a reason for hiding this comment

codecov bot commented Aug 23, 2022 •

edited

Loading

AndrewSisley Aug 23, 2022 •

edited

Loading