From 8874c5fde1aa8c8d5dc6f1f94e3eea7eb9d04bba Mon Sep 17 00:00:00 2001 From: John-Alan Simmons Date: Sat, 20 Jun 2026 00:26:44 -0700 Subject: [PATCH 1/4] added group by field dependency resolver --- internal/planner/mapper/mapper.go | 103 ++++++++++++++++++++++-------- 1 file changed, 78 insertions(+), 25 deletions(-) diff --git a/internal/planner/mapper/mapper.go b/internal/planner/mapper/mapper.go index 54decbd7cc..80247a7bf2 100644 --- a/internal/planner/mapper/mapper.go +++ b/internal/planner/mapper/mapper.go @@ -210,31 +210,12 @@ func toSelect( } } - // Resolve groupBy mappings i.e. alias remapping and handle missed inner group. - if selectRequest.GroupBy.HasValue() { - groupByFields := selectRequest.GroupBy.Value().Fields - // Remap all alias field names to use their internal field name mappings. - for index, groupByField := range groupByFields { - fieldDesc, ok := definition.GetFieldByName(groupByField) - if ok && fieldDesc.Kind.IsObject() { - if fieldDesc.Kind.IsArray() { - return nil, NewErrInvalidFieldToGroupBy(groupByField) - } - groupByFields[index] = request.ToFieldID(groupByField) - } - } - - selectRequest.GroupBy = immutable.Some( - request.GroupBy{ - Fields: groupByFields, - }, - ) - - // If there is a groupBy, and no inner group has been requested, we need to map the property here - if _, isGroupFieldMapped := mapping.IndexesByName[request.GroupFieldName]; !isGroupFieldMapped { - index := mapping.GetNextIndex() - mapping.Add(index, request.GroupFieldName) - } + // Resolve groupBy dependencies: alias/relation remapping, map the missed inner + // group field, and ensure groupBy fields are fetched even when not part of the + // selection set. + fields, err = resolveGroupByDependencies(definition, selectRequest, mapping, fields) + if err != nil { + return nil, err } targetable, err := toTargetable(thisIndex, selectRequest, mapping) @@ -391,6 +372,78 @@ func resolveChildOrder( return nil, ErrMissingSelect } +// resolveGroupByDependencies remaps the groupBy fields to their internal field names, +// maps the synthetic GROUP field if no inner group was requested, and ensures that +// every groupBy field is fetched even when it is not part of the selection set. +// +// This mirrors how [resolveOrderDependencies] adds order fields that were missed due +// to not being rendered. +func resolveGroupByDependencies( + definition client.CollectionVersion, + selectRequest *request.Select, + mapping *core.DocumentMapping, + fields []Requestable, +) ([]Requestable, error) { + if !selectRequest.GroupBy.HasValue() { + return fields, nil + } + + groupByFields := selectRequest.GroupBy.Value().Fields + // Remap all object (relation) field names to use their internal foreign-key field + // id, as that is the scalar value actually stored on - and fetched from - the document. + for index, groupByField := range groupByFields { + fieldDesc, ok := definition.GetFieldByName(groupByField) + if ok && fieldDesc.Kind.IsObject() { + if fieldDesc.Kind.IsArray() { + return nil, NewErrInvalidFieldToGroupBy(groupByField) + } + groupByFields[index] = request.ToFieldID(groupByField) + } + } + + selectRequest.GroupBy = immutable.Some( + request.GroupBy{ + Fields: groupByFields, + }, + ) + + // If there is a groupBy, and no inner group has been requested, we need to map the property here + if _, isGroupFieldMapped := mapping.IndexesByName[request.GroupFieldName]; !isGroupFieldMapped { + index := mapping.GetNextIndex() + mapping.Add(index, request.GroupFieldName) + } + + // Ensure every groupBy field is fetched, even when it is not part of the selection + // set. The field is added as a hidden dependency (it is given no render key) so that + // its value is available for group-key computation without being returned in the response. + for _, groupByField := range groupByFields { + alreadyRequested := false + for _, existingField := range fields { + if existingField.GetName() == groupByField { + alreadyRequested = true + break + } + } + if alreadyRequested { + continue + } + + fieldIndexes := mapping.IndexesByName[groupByField] + if len(fieldIndexes) == 0 { + // Should be unreachable for a valid groupBy field as all base fields are + // mapped by getTopLevelInfo, but guard against an out-of-range panic. + continue + } + + fields = append(fields, &Field{ + Index: fieldIndexes[0], + Name: groupByField, + }) + } + + return fields, nil +} + // resolveAggregates figures out which fields the given aggregates are targeting // and converts the aggregateRequest into an Aggregate, appending it onto the given // fields slice. From 8fb7e97a57477346706475a99194904ff8a2b579 Mon Sep 17 00:00:00 2001 From: John-Alan Simmons Date: Sat, 20 Jun 2026 00:28:08 -0700 Subject: [PATCH 2/4] add tests --- .../with_group_unselected_field_test.go | 188 ++++++++++++++++++ .../with_group_unselected_field_test.go | 141 +++++++++++++ 2 files changed, 329 insertions(+) create mode 100644 tests/integration/query/one_to_many/with_group_unselected_field_test.go create mode 100644 tests/integration/query/simple/with_group_unselected_field_test.go diff --git a/tests/integration/query/one_to_many/with_group_unselected_field_test.go b/tests/integration/query/one_to_many/with_group_unselected_field_test.go new file mode 100644 index 0000000000..3734cb31b2 --- /dev/null +++ b/tests/integration/query/one_to_many/with_group_unselected_field_test.go @@ -0,0 +1,188 @@ +// Copyright 2026 Democratized Data Foundation +// +// This file is part of the DefraDB test suite. +// +// The DefraDB test suite is licensed under either: +// +// (1) GNU Affero General Public License v3 +// (2) Business Source License 1.1 +// +// See tests/LICENSE for details. + +package one_to_many + +import ( + "testing" + + "github.com/sourcenetwork/defradb/tests/action" + testUtils "github.com/sourcenetwork/defradb/tests/integration" +) + +var booksByThreeAuthors = []any{ + &action.AddDoc{ + CollectionID: 0, + Doc: `{ + "name": "Painted House", + "rating": 4.9, + "_authorID": "bae-9d52c335-c8e3-5782-8daa-e359c106e0ab" + }`, + }, + &action.AddDoc{ + CollectionID: 0, + Doc: `{ + "name": "A Time for Mercy", + "rating": 4.5, + "_authorID": "bae-9d52c335-c8e3-5782-8daa-e359c106e0ab" + }`, + }, + &action.AddDoc{ + CollectionID: 0, + Doc: `{ + "name": "Candide", + "rating": 4.95, + "_authorID": "bae-b9c6cd5a-a931-5984-994d-7c435baa9f32" + }`, + }, + &action.AddDoc{ + CollectionID: 0, + Doc: `{ + "name": "Zadig", + "rating": 4.91, + "_authorID": "bae-b9c6cd5a-a931-5984-994d-7c435baa9f32" + }`, + }, + &action.AddDoc{ + CollectionID: 1, + Doc: `{ + "name": "John Grisham", + "age": 65, + "verified": true + }`, + }, + &action.AddDoc{ + CollectionID: 1, + Doc: `{ + "name": "Voltaire", + "age": 327, + "verified": true + }`, + }, +} + +// Regression test for https://github.com/sourcenetwork/defradb/issues/4954 for the related-id +// field. Grouping by the foreign-key id must still occur even when the id is not rendered. +func TestQueryOneToManyWithGroupByRelatedIDWithoutRenderedGroupField(t *testing.T) { + test := testUtils.TestCase{ + Actions: append( + append([]any{}, booksByThreeAuthors...), + &action.Request{ + Request: `query { + Book(groupBy: [_authorID]) { + GROUP { + name + } + } + }`, + Results: map[string]any{ + "Book": []map[string]any{ + { + "GROUP": []map[string]any{ + {"name": "Painted House"}, + {"name": "A Time for Mercy"}, + }, + }, + { + "GROUP": []map[string]any{ + {"name": "Candide"}, + {"name": "Zadig"}, + }, + }, + }, + }, + NonOrderedResults: true, + }, + ), + } + + executeTestCase(t, test) +} + +// Grouping by the relation object field name while also rendering the foreign-key id. This +// exercises the path where the fetched group-by dependency and an explicitly selected field +// resolve to the same underlying field, ensuring the id is rendered exactly once per group. +func TestQueryOneToManyWithGroupByRelationObjectRenderingRelatedID(t *testing.T) { + test := testUtils.TestCase{ + Actions: append( + append([]any{}, booksByThreeAuthors...), + &action.Request{ + Request: `query { + Book(groupBy: [author]) { + _authorID + GROUP { + name + } + } + }`, + Results: map[string]any{ + "Book": []map[string]any{ + { + "_authorID": "bae-9d52c335-c8e3-5782-8daa-e359c106e0ab", + "GROUP": []map[string]any{ + {"name": "Painted House"}, + {"name": "A Time for Mercy"}, + }, + }, + { + "_authorID": "bae-b9c6cd5a-a931-5984-994d-7c435baa9f32", + "GROUP": []map[string]any{ + {"name": "Candide"}, + {"name": "Zadig"}, + }, + }, + }, + }, + NonOrderedResults: true, + }, + ), + } + + executeTestCase(t, test) +} + +// As above but grouping by the relation object field name (which is internally remapped to its +// foreign-key id) rather than the id field directly. +func TestQueryOneToManyWithGroupByRelationObjectWithoutRenderedGroupField(t *testing.T) { + test := testUtils.TestCase{ + Actions: append( + append([]any{}, booksByThreeAuthors...), + &action.Request{ + Request: `query { + Book(groupBy: [author]) { + GROUP { + name + } + } + }`, + Results: map[string]any{ + "Book": []map[string]any{ + { + "GROUP": []map[string]any{ + {"name": "Painted House"}, + {"name": "A Time for Mercy"}, + }, + }, + { + "GROUP": []map[string]any{ + {"name": "Candide"}, + {"name": "Zadig"}, + }, + }, + }, + }, + NonOrderedResults: true, + }, + ), + } + + executeTestCase(t, test) +} diff --git a/tests/integration/query/simple/with_group_unselected_field_test.go b/tests/integration/query/simple/with_group_unselected_field_test.go new file mode 100644 index 0000000000..98df4442c8 --- /dev/null +++ b/tests/integration/query/simple/with_group_unselected_field_test.go @@ -0,0 +1,141 @@ +// Copyright 2026 Democratized Data Foundation +// +// This file is part of the DefraDB test suite. +// +// The DefraDB test suite is licensed under either: +// +// (1) GNU Affero General Public License v3 +// (2) Business Source License 1.1 +// +// See tests/LICENSE for details. + +package simple + +import ( + "testing" + + "github.com/sourcenetwork/defradb/tests/action" + testUtils "github.com/sourcenetwork/defradb/tests/integration" +) + +// This is a regression test for https://github.com/sourcenetwork/defradb/issues/4954. +// +// Grouping must still occur on the grouped-by field even when that field is not part of +// the parent's selection set. Previously the field was never fetched, so every document +// produced an identical (nil) group key and all documents collapsed into a single group. +func TestQuerySimpleWithGroupByNumberWithoutRenderedGroupField(t *testing.T) { + test := testUtils.TestCase{ + Actions: []any{ + &action.AddDoc{ + Doc: `{ + "Name": "John", + "Age": 32 + }`, + }, + &action.AddDoc{ + Doc: `{ + "Name": "Bob", + "Age": 32 + }`, + }, + &action.AddDoc{ + Doc: `{ + "Name": "Carlo", + "Age": 55 + }`, + }, + &action.AddDoc{ + Doc: `{ + "Name": "Alice", + "Age": 19 + }`, + }, + &action.Request{ + Request: `query { + Users(groupBy: [Age]) { + GROUP { + Name + } + } + }`, + Results: map[string]any{ + "Users": []map[string]any{ + { + "GROUP": []map[string]any{ + {"Name": "John"}, + {"Name": "Bob"}, + }, + }, + { + "GROUP": []map[string]any{ + {"Name": "Carlo"}, + }, + }, + { + "GROUP": []map[string]any{ + {"Name": "Alice"}, + }, + }, + }, + }, + NonOrderedResults: true, + }, + }, + } + + executeTestCase(t, test) +} + +// Companion to the above using a String grouped-by field (matching the genre field in the +// original issue report), ensuring the fix is not specific to a single field kind. +func TestQuerySimpleWithGroupByStringWithoutRenderedGroupField(t *testing.T) { + test := testUtils.TestCase{ + Actions: []any{ + &action.AddDoc{ + Doc: `{ + "Name": "John", + "Email": "fiction@example.com" + }`, + }, + &action.AddDoc{ + Doc: `{ + "Name": "Bob", + "Email": "fiction@example.com" + }`, + }, + &action.AddDoc{ + Doc: `{ + "Name": "Carlo", + "Email": "nonfiction@example.com" + }`, + }, + &action.Request{ + Request: `query { + Users(groupBy: [Email]) { + GROUP { + Name + } + } + }`, + Results: map[string]any{ + "Users": []map[string]any{ + { + "GROUP": []map[string]any{ + {"Name": "John"}, + {"Name": "Bob"}, + }, + }, + { + "GROUP": []map[string]any{ + {"Name": "Carlo"}, + }, + }, + }, + }, + NonOrderedResults: true, + }, + }, + } + + executeTestCase(t, test) +} From a7a15b753eed9aaed189ec84fae5757126ae4dbe Mon Sep 17 00:00:00 2001 From: John-Alan Simmons Date: Mon, 22 Jun 2026 11:41:57 -0700 Subject: [PATCH 3/4] removed test comments --- .../query/one_to_many/with_group_unselected_field_test.go | 5 ----- .../query/simple/with_group_unselected_field_test.go | 7 ------- 2 files changed, 12 deletions(-) diff --git a/tests/integration/query/one_to_many/with_group_unselected_field_test.go b/tests/integration/query/one_to_many/with_group_unselected_field_test.go index 3734cb31b2..14e626850d 100644 --- a/tests/integration/query/one_to_many/with_group_unselected_field_test.go +++ b/tests/integration/query/one_to_many/with_group_unselected_field_test.go @@ -69,8 +69,6 @@ var booksByThreeAuthors = []any{ }, } -// Regression test for https://github.com/sourcenetwork/defradb/issues/4954 for the related-id -// field. Grouping by the foreign-key id must still occur even when the id is not rendered. func TestQueryOneToManyWithGroupByRelatedIDWithoutRenderedGroupField(t *testing.T) { test := testUtils.TestCase{ Actions: append( @@ -107,9 +105,6 @@ func TestQueryOneToManyWithGroupByRelatedIDWithoutRenderedGroupField(t *testing. executeTestCase(t, test) } -// Grouping by the relation object field name while also rendering the foreign-key id. This -// exercises the path where the fetched group-by dependency and an explicitly selected field -// resolve to the same underlying field, ensuring the id is rendered exactly once per group. func TestQueryOneToManyWithGroupByRelationObjectRenderingRelatedID(t *testing.T) { test := testUtils.TestCase{ Actions: append( diff --git a/tests/integration/query/simple/with_group_unselected_field_test.go b/tests/integration/query/simple/with_group_unselected_field_test.go index 98df4442c8..4ab9044e21 100644 --- a/tests/integration/query/simple/with_group_unselected_field_test.go +++ b/tests/integration/query/simple/with_group_unselected_field_test.go @@ -18,11 +18,6 @@ import ( testUtils "github.com/sourcenetwork/defradb/tests/integration" ) -// This is a regression test for https://github.com/sourcenetwork/defradb/issues/4954. -// -// Grouping must still occur on the grouped-by field even when that field is not part of -// the parent's selection set. Previously the field was never fetched, so every document -// produced an identical (nil) group key and all documents collapsed into a single group. func TestQuerySimpleWithGroupByNumberWithoutRenderedGroupField(t *testing.T) { test := testUtils.TestCase{ Actions: []any{ @@ -86,8 +81,6 @@ func TestQuerySimpleWithGroupByNumberWithoutRenderedGroupField(t *testing.T) { executeTestCase(t, test) } -// Companion to the above using a String grouped-by field (matching the genre field in the -// original issue report), ensuring the fix is not specific to a single field kind. func TestQuerySimpleWithGroupByStringWithoutRenderedGroupField(t *testing.T) { test := testUtils.TestCase{ Actions: []any{ From 7b1a6af69c909f91accabf9cb0caba1d02eb1570 Mon Sep 17 00:00:00 2001 From: John-Alan Simmons Date: Mon, 22 Jun 2026 12:36:20 -0700 Subject: [PATCH 4/4] formatted test func names --- .../query/one_to_many/with_group_unselected_field_test.go | 8 +++----- .../query/simple/with_group_unselected_field_test.go | 4 ++-- 2 files changed, 5 insertions(+), 7 deletions(-) diff --git a/tests/integration/query/one_to_many/with_group_unselected_field_test.go b/tests/integration/query/one_to_many/with_group_unselected_field_test.go index 14e626850d..e74c2487ae 100644 --- a/tests/integration/query/one_to_many/with_group_unselected_field_test.go +++ b/tests/integration/query/one_to_many/with_group_unselected_field_test.go @@ -69,7 +69,7 @@ var booksByThreeAuthors = []any{ }, } -func TestQueryOneToManyWithGroupByRelatedIDWithoutRenderedGroupField(t *testing.T) { +func TestQueryOneToMany_WithGroupByRelatedIDWithoutRenderedGroupField(t *testing.T) { test := testUtils.TestCase{ Actions: append( append([]any{}, booksByThreeAuthors...), @@ -105,7 +105,7 @@ func TestQueryOneToManyWithGroupByRelatedIDWithoutRenderedGroupField(t *testing. executeTestCase(t, test) } -func TestQueryOneToManyWithGroupByRelationObjectRenderingRelatedID(t *testing.T) { +func TestQueryOneToMany_WithGroupByRelationObjectRenderingRelatedID(t *testing.T) { test := testUtils.TestCase{ Actions: append( append([]any{}, booksByThreeAuthors...), @@ -144,9 +144,7 @@ func TestQueryOneToManyWithGroupByRelationObjectRenderingRelatedID(t *testing.T) executeTestCase(t, test) } -// As above but grouping by the relation object field name (which is internally remapped to its -// foreign-key id) rather than the id field directly. -func TestQueryOneToManyWithGroupByRelationObjectWithoutRenderedGroupField(t *testing.T) { +func TestQueryOneToMany_WithGroupByRelationObjectWithoutRenderedGroupField(t *testing.T) { test := testUtils.TestCase{ Actions: append( append([]any{}, booksByThreeAuthors...), diff --git a/tests/integration/query/simple/with_group_unselected_field_test.go b/tests/integration/query/simple/with_group_unselected_field_test.go index 4ab9044e21..ce423d8c3e 100644 --- a/tests/integration/query/simple/with_group_unselected_field_test.go +++ b/tests/integration/query/simple/with_group_unselected_field_test.go @@ -18,7 +18,7 @@ import ( testUtils "github.com/sourcenetwork/defradb/tests/integration" ) -func TestQuerySimpleWithGroupByNumberWithoutRenderedGroupField(t *testing.T) { +func TestQuerySimple_WithGroupByNumberWithoutRenderedGroupField(t *testing.T) { test := testUtils.TestCase{ Actions: []any{ &action.AddDoc{ @@ -81,7 +81,7 @@ func TestQuerySimpleWithGroupByNumberWithoutRenderedGroupField(t *testing.T) { executeTestCase(t, test) } -func TestQuerySimpleWithGroupByStringWithoutRenderedGroupField(t *testing.T) { +func TestQuerySimple_WithGroupByStringWithoutRenderedGroupField(t *testing.T) { test := testUtils.TestCase{ Actions: []any{ &action.AddDoc{