add max value tests. refactor min max return type handling

sourcenetwork · Oct 1, 2024 · 623e275 · 623e275
1 parent 9a10cb1
commit 623e275
Show file tree

Hide file tree

Showing 4 changed files with 94 additions and 40 deletions.
diff --git a/internal/planner/max.go b/internal/planner/max.go
@@ -24,10 +24,10 @@ type maxNode struct {
 	documentIterator
 	docMapper
 
-	p    *Planner
-	plan planNode
+	p      *Planner
+	plan   planNode
+	parent *mapper.Select
 
-	isFloat bool
 	// virtualFieldIndex is the index of the field
 	// that contains the result of the aggregate.
 	virtualFieldIndex int
@@ -45,22 +45,9 @@ func (p *Planner) Max(
 	field *mapper.Aggregate,
 	parent *mapper.Select,
 ) (*maxNode, error) {
-	isFloat := false
-	for _, target := range field.AggregateTargets {
-		isTargetFloat, err := p.isValueFloat(parent, &target)
-		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
-		if isTargetFloat {
-			isFloat = true
-			break
-		}
-	}
-
 	return &maxNode{
 		p:                 p,
-		isFloat:           isFloat,
+		parent:            parent,
 		aggregateMapping:  field.AggregateTargets,
 		virtualFieldIndex: field.Index,
 		docMapper:         docMapper{field.DocumentMapping},
@@ -142,6 +129,8 @@ func (n *maxNode) Next() (bool, error) {
 	n.currentValue = n.plan.Value()
 
 	var max *big.Float
+	isFloat := false
+
 	for _, source := range n.aggregateMapping {
 		child := n.currentValue.Fields[source.Index]
 		var collectionMax *big.Float
@@ -242,14 +231,20 @@ func (n *maxNode) Next() (bool, error) {
 		if err != nil {
 			return false, err
 		}
-		if max == nil || collectionMax == nil || collectionMax.Cmp(max) > 0 {
-			max = collectionMax
+		if collectionMax == nil || (max != nil && collectionMax.Cmp(max) <= 0) {
+			continue
+		}
+		isTargetFloat, err := n.p.isValueFloat(n.parent, &source)
+		if err != nil {
+			return false, err
 		}
+		isFloat = isTargetFloat
+		max = collectionMax
 	}
 
 	if max == nil {
 		n.currentValue.Fields[n.virtualFieldIndex] = nil
-	} else if n.isFloat {
+	} else if isFloat {
 		res, _ := max.Float64()
 		n.currentValue.Fields[n.virtualFieldIndex] = res
 	} else {

diff --git a/internal/planner/min.go b/internal/planner/min.go
@@ -24,10 +24,10 @@ type minNode struct {
 	documentIterator
 	docMapper
 
-	p    *Planner
-	plan planNode
+	p      *Planner
+	plan   planNode
+	parent *mapper.Select
 
-	isFloat bool
 	// virtualFieldIndex is the index of the field
 	// that contains the result of the aggregate.
 	virtualFieldIndex int
@@ -45,22 +45,9 @@ func (p *Planner) Min(
 	field *mapper.Aggregate,
 	parent *mapper.Select,
 ) (*minNode, error) {
-	isFloat := false
-	for _, target := range field.AggregateTargets {
-		isTargetFloat, err := p.isValueFloat(parent, &target)
-		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
-		if isTargetFloat {
-			isFloat = true
-			break
-		}
-	}
-
 	return &minNode{
 		p:                 p,
-		isFloat:           isFloat,
+		parent:            parent,
 		aggregateMapping:  field.AggregateTargets,
 		virtualFieldIndex: field.Index,
 		docMapper:         docMapper{field.DocumentMapping},
@@ -142,6 +129,8 @@ func (n *minNode) Next() (bool, error) {
 	n.currentValue = n.plan.Value()
 
 	var min *big.Float
+	isFloat := false
+
 	for _, source := range n.aggregateMapping {
 		child := n.currentValue.Fields[source.Index]
 		var collectionMin *big.Float
@@ -242,14 +231,20 @@ func (n *minNode) Next() (bool, error) {
 		if err != nil {
 			return false, err
 		}
-		if min == nil || collectionMin == nil || collectionMin.Cmp(min) < 0 {
-			min = collectionMin
+		if collectionMin == nil || (min != nil && collectionMin.Cmp(min) >= 0) {
+			continue
+		}
+		isTargetFloat, err := n.p.isValueFloat(n.parent, &source)
+		if err != nil {
+			return false, err
 		}
+		isFloat = isTargetFloat
+		min = collectionMin
 	}
 
 	if min == nil {
 		n.currentValue.Fields[n.virtualFieldIndex] = nil
-	} else if n.isFloat {
+	} else if isFloat {
 		res, _ := min.Float64()
 		n.currentValue.Fields[n.virtualFieldIndex] = res
 	} else {

diff --git a/tests/integration/query/simple/with_max_test.go b/tests/integration/query/simple/with_max_test.go
@@ -11,9 +11,12 @@
 package simple
 
 import (
+	"math"
 	"testing"
 
 	testUtils "github.com/sourcenetwork/defradb/tests/integration"
+
+	"github.com/sourcenetwork/immutable"
 )
 
 func TestQuerySimple_WithMaxOnUndefinedObject_ReturnsError(t *testing.T) {
@@ -95,3 +98,32 @@ func TestQuerySimple_WithMax_Succeeds(t *testing.T) {
 
 	executeTestCase(t, test)
 }
+
+func TestQuerySimple_WithMaxAndMaxValueInt_Succeeds(t *testing.T) {
+	test := testUtils.TestCase{
+		SupportedMutationTypes: immutable.Some([]testUtils.MutationType{
+			// GraphQL does not support 64 bit int
+			testUtils.CollectionSaveMutationType,
+			testUtils.CollectionNamedMutationType,
+		}),
+		Description: "Simple query max and max value int",
+		Actions: []any{
+			testUtils.CreateDoc{
+				DocMap: map[string]any{
+					"Name": "John",
+					"Age":  int64(math.MaxInt64),
+				},
+			},
+			testUtils.Request{
+				Request: `query {
+					_max(Users: {field: Age})
+				}`,
+				Results: map[string]any{
+					"_max": int64(math.MaxInt64),
+				},
+			},
+		},
+	}
+
+	executeTestCase(t, test)
+}
diff --git a/tests/integration/query/simple/with_min_test.go b/tests/integration/query/simple/with_min_test.go
@@ -11,9 +11,12 @@
 package simple
 
 import (
+	"math"
 	"testing"
 
 	testUtils "github.com/sourcenetwork/defradb/tests/integration"
+
+	"github.com/sourcenetwork/immutable"
 )
 
 func TestQuerySimple_WithMinOnUndefinedObject_ReturnsError(t *testing.T) {
@@ -95,3 +98,32 @@ func TestQuerySimple_WithMin_Succeeds(t *testing.T) {
 
 	executeTestCase(t, test)
 }
+
+func TestQuerySimple_WithMinAndMaxValueInt_Succeeds(t *testing.T) {
+	test := testUtils.TestCase{
+		SupportedMutationTypes: immutable.Some([]testUtils.MutationType{
+			// GraphQL does not support 64 bit int
+			testUtils.CollectionSaveMutationType,
+			testUtils.CollectionNamedMutationType,
+		}),
+		Description: "Simple query min and max value int",
+		Actions: []any{
+			testUtils.CreateDoc{
+				DocMap: map[string]any{
+					"Name": "John",
+					"Age":  int64(math.MaxInt64),
+				},
+			},
+			testUtils.Request{
+				Request: `query {
+					_max(Users: {field: Age})
+				}`,
+				Results: map[string]any{
+					"_max": int64(math.MaxInt64),
+				},
+			},
+		},
+	}
+
+	executeTestCase(t, test)
+}