fix: Group by field dependency without selection by jsimnz · Pull Request #4966 · sourcenetwork/defradb

jsimnz · 2026-06-20T07:50:11Z

Relevant issue(s)

Resolves #4954

Description

Added an explicit step to mapper to resolve group by dependencies instead of relying on implicit selection set field resolvers.

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?

Manual, added integration tests to cover various cases

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

Linux NixOS

coderabbitai · 2026-06-20T07:52:00Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: a92c3714-5d68-4c59-91a1-c3e3a2ce40c8

📥 Commits

Reviewing files that changed from the base of the PR and between a7a15b7 and 7b1a6af.

📒 Files selected for processing (2)

tests/integration/query/one_to_many/with_group_unselected_field_test.go
tests/integration/query/simple/with_group_unselected_field_test.go

🚧 Files skipped from review as they are similar to previous changes (1)

tests/integration/query/simple/with_group_unselected_field_test.go

📜 Recent review details

⏰ Context from checks skipped due to timeout. (35)

GitHub Check: Test macos job
GitHub Check: Build dependencies job
GitHub Check: Validate containerfile job
GitHub Check: Test Limited Resource job
GitHub Check: Check mocks job
GitHub Check: Build and test Linux C shared library
GitHub Check: Build, install, and test Debian package
GitHub Check: Start binary job
GitHub Check: Check wizard health job
GitHub Check: Test NPX/JS build job
GitHub Check: Test coverage job (c, file, collection-save)
GitHub Check: Test coverage job (cli, file, collection-save)
GitHub Check: Test coverage job (cli, file, collection-named)
GitHub Check: Test coverage job (cli, file, gql)
GitHub Check: Test coverage job (go, memory, gql)
GitHub Check: Test coverage job (go, file, gql)
GitHub Check: Test coverage job (http, file, collection-save)
GitHub Check: Test coverage job (go, memory, collection-save)
GitHub Check: Test coverage job (http, file, gql)
GitHub Check: Test coverage document acp job (c, source-hub)
GitHub Check: Test coverage leveldb job
GitHub Check: Test coverage JS job
GitHub Check: Test coverage secondary index job
GitHub Check: Test coverage telemetry job
GitHub Check: Check vulnerabilities job
GitHub Check: Test coverage document acp job (http, source-hub)
GitHub Check: Test coverage lens job (wazero)
GitHub Check: Test coverage document acp job (go, source-hub)
GitHub Check: Test coverage signed docs job
GitHub Check: Test coverage encryption job
GitHub Check: Test coverage view job
GitHub Check: Check cli documentation job
GitHub Check: Check http documentation job
GitHub Check: Lint GoLang job
GitHub Check: Lint GoLang job

🧰 Additional context used

🧠 Learnings (3)

📚 Learning: 2026-03-27T08:24:16.083Z

Learnt from: islamaliev
Repo: sourcenetwork/defradb PR: 4661
File: http/middleware.go:93-93
Timestamp: 2026-03-27T08:24:16.083Z
Learning: When handling `client.ErrNotAuthorizedToPerformOperation` in the defradb repository, treat it as an unauthenticated/missing-identity condition (not insufficient permissions). Map it to HTTP 401 Unauthorized rather than 403 Forbidden; this mapping is intentional and consistent with prior behavior (pre-PR `#4661`).

Applied to files:

tests/integration/query/one_to_many/with_group_unselected_field_test.go

📚 Learning: 2026-06-18T16:47:47.339Z

Learnt from: islamaliev
Repo: sourcenetwork/defradb PR: 4942
File: internal/db/db.go:227-231
Timestamp: 2026-06-18T16:47:47.339Z
Learning: When reviewing this repository’s Go code, do not flag `sync.WaitGroup.Go()` (e.g., `wg.Go(fn)`) as a compilation error. This method exists starting in Go 1.25; it should be treated as valid as long as the repo’s configured/minimum Go toolchain version is 1.25+ (e.g., per `go.mod` and/or CI). If the project is building with an older Go version, then it may be appropriate to flag it.

Applied to files:

tests/integration/query/one_to_many/with_group_unselected_field_test.go

📚 Learning: 2026-06-18T16:47:47.339Z

Learnt from: islamaliev
Repo: sourcenetwork/defradb PR: 4942
File: internal/db/db.go:227-231
Timestamp: 2026-06-18T16:47:47.339Z
Learning: In sourcenetwork/defradb code reviews, do not raise findings that are likely compiler/type-check errors (e.g., code that would fail `go test`/`go build`) because CI pipelines will catch compilation failures. Only flag compilation-related problems if there is a clear, verifiable issue beyond what CI will already surface (e.g., a mismatched build tag or missing generated artifact that CI would not cover).

Applied to files:

tests/integration/query/one_to_many/with_group_unselected_field_test.go

🔇 Additional comments (3)

tests/integration/query/one_to_many/with_group_unselected_field_test.go (3)

72-106: LGTM!

147-181: LGTM!

108-145: The hard-coded author IDs in the test results match the values in the booksByThreeAuthors dataset. No action needed.

📝 Walkthrough

Walkthrough

toSelect in the mapper now delegates group-by field processing to a new resolveGroupByDependencies helper. This helper remaps relation/object group-by fields to their internal FK IDs, ensures the synthetic GROUP field is mapped, and appends any group-by fields not already in the selection set as hidden dependencies. Integration tests cover the regression for scalar and relation group-by cases.

Changes

GroupBy hidden dependency fix

Layer / File(s)	Summary
`resolveGroupByDependencies` implementation `internal/planner/mapper/mapper.go`	`toSelect` replaces inlined group-by logic with a call to the new `resolveGroupByDependencies` helper, which remaps relation/object fields to FK IDs, maps the synthetic `GROUP` field, and appends unselected group-by fields as hidden dependencies so group-key computation works even when those fields are not rendered.
Scalar groupBy regression tests `tests/integration/query/simple/with_group_unselected_field_test.go`	Adds integration tests for numeric (`Age`) and string (`Email`) group-by where the grouped field is omitted from the selection set, asserting separate `GROUP` arrays per distinct value.
One-to-many relation groupBy regression tests `tests/integration/query/one_to_many/with_group_unselected_field_test.go`	Adds integration tests for relation group-by covering grouping by FK field `_authorID` without rendering it, grouping by relation object `author` while selecting or omitting the FK field.

Sequence Diagrams

sequenceDiagram
  participant toSelect
  participant resolveGroupByDependencies
  participant mapping
  participant fields

  toSelect->>resolveGroupByDependencies: selectRequest, fields, mapping
  resolveGroupByDependencies->>mapping: remap object/relation field to FK ID
  resolveGroupByDependencies->>mapping: ensure GROUP field is mapped
  resolveGroupByDependencies->>fields: append missing groupBy fields as hidden deps
  resolveGroupByDependencies-->>toSelect: updated fields or error

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~15 minutes

Assessment against linked issues

Objective	Addressed	Explanation
Grouping should produce correct per-group splits when the grouped-by field is not included in the return selection set [`#4954`]	✅

Possibly related PRs

sourcenetwork/defradb#4409: Both PRs address planner/grouping internals to ensure groupBy key fields are available for group computation even when not directly rendered—this PR via resolveGroupByDependencies helper in mapper, related PR via indexed _group/join mapping fixes.

Suggested labels

bug

Suggested reviewers

fredcarle
shahzadlone
AndrewSisley

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

codecov · 2026-06-20T07:58:22Z

Codecov Report

❌ Patch coverage is 94.59459% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 76.29%. Comparing base (d10c71f) to head (7b1a6af).
⚠️ Report is 1 commits behind head on develop.

Files with missing lines	Patch %	Lines
internal/planner/mapper/mapper.go	94.59%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #4966      +/-   ##
===========================================
+ Coverage    76.21%   76.29%   +0.08%     
===========================================
  Files          606      606              
  Lines        46810    46831      +21     
===========================================
+ Hits         35675    35727      +52     
+ Misses        8322     8304      -18     
+ Partials      2813     2800      -13

Flag	Coverage Δ
all-tests	`76.29% <94.59%> (+0.08%)`	⬆️

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

Files with missing lines	Coverage Δ
internal/planner/mapper/mapper.go	`90.85% <94.59%> (-0.01%)`	⬇️

... and 12 files with indirect coverage changes

Continue to review full report in Codecov by Harness.

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

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

ChrisBQu · 2026-06-22T12:54:07Z

+	testUtils "github.com/sourcenetwork/defradb/tests/integration"
+)
+
+// This is a regression test for https://github.com/sourcenetwork/defradb/issues/4954.


question: Why the verbosity on this particular test? And why the link to a specific github issue? I am asking because I just commented on a third-party contributor PR, stating that we typically don't do this -- which, historically, is true.

Are we moving into a different direction with introducing regression tests, and how we document them? If so, I'm not against it. I just think we should seek to be consistent.

I would like an answer, but what that answer is does not affect my approving this PR. Gonna give it the LGTM.

todo: Please remove the link to the issue :)

fredcarle

LGTM. Just one very minor nitpick todo.

shahzadlone

LGTM

ribiza · 2026-07-15T15:12:00Z

In the interest of not losing valuable work: is there anything else that needs to be done here, or can we merge? @jsimnz

jsimnz added this to the DefraDB v1.0 milestone Jun 20, 2026

jsimnz self-assigned this Jun 20, 2026

ChrisBQu approved these changes Jun 22, 2026

View reviewed changes

fredcarle approved these changes Jun 22, 2026

View reviewed changes

jsimnz added 3 commits June 22, 2026 11:41

added group by field dependency resolver

8874c5f

add tests

8fb7e97

removed test comments

a7a15b7

jsimnz force-pushed the jsimnz/fix/groupby-field-result-ignore branch from 5054276 to a7a15b7 Compare June 22, 2026 18:42

formatted test func names

7b1a6af

edjroz mentioned this pull request Jun 24, 2026

Double groupBy query returns null values for relationship #4952

Open

shahzadlone approved these changes Jul 2, 2026

View reviewed changes

Navigation Menu

Uh oh!

fix: Group by field dependency without selection - #4966

fix: Group by field dependency without selection#4966
jsimnz wants to merge 4 commits into
developfrom
jsimnz/fix/groupby-field-result-ignore

jsimnz commented Jun 20, 2026

Uh oh!

coderabbitai Bot commented Jun 20, 2026 •

edited

Loading

Walkthrough

Changes

Sequence Diagrams

Estimated code review effort

Assessment against linked issues

Possibly related PRs

Suggested labels

Suggested reviewers

Uh oh!

codecov Bot commented Jun 20, 2026 •

edited

Loading

Uh oh!

ChrisBQu Jun 22, 2026

Uh oh!

fredcarle Jun 22, 2026

Uh oh!

fredcarle left a comment

Uh oh!

shahzadlone left a comment

Uh oh!

ribiza commented Jul 15, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Uh oh!

Conversation

jsimnz commented Jun 20, 2026

Relevant issue(s)

Description

Tasks

How has this been tested?

Uh oh!

coderabbitai Bot commented Jun 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Sequence Diagrams

Estimated code review effort

Assessment against linked issues

Possibly related PRs

Suggested labels

Suggested reviewers

Uh oh!

codecov Bot commented Jun 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ChrisBQu Jun 22, 2026

Choose a reason for hiding this comment

Uh oh!

fredcarle Jun 22, 2026

Choose a reason for hiding this comment

Uh oh!

fredcarle left a comment

Choose a reason for hiding this comment

Uh oh!

shahzadlone left a comment

Choose a reason for hiding this comment

Uh oh!

ribiza commented Jul 15, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

coderabbitai Bot commented Jun 20, 2026 •

edited

Loading

codecov Bot commented Jun 20, 2026 •

edited

Loading