Fix union overfetching #279

kamilkisiela · 2025-07-16T14:27:52Z

by adding type conditions to the FetchNodes

summary of changes -> #279 (comment)

Closes #277

github-actions · 2025-07-16T14:29:04Z

Federation Audit Results

189 tests ±0 189 ✅ ±0 4s ⏱️ -1s
42 suites ±0 0 💤 ±0
42 files ±0 0 ❌ ±0

Results for commit dffbc05. ± Comparison against base commit 7e1c0f6.

♻️ This comment has been updated with latest results.

github-actions · 2025-07-16T14:29:05Z

GraphQL over HTTP Audit Results

61 tests ±0 61 ✅ ±0 0s ⏱️ ±0s
1 suites ±0 0 💤 ±0
1 files ±0 0 ❌ ±0

Results for commit 983ac7f. ± Comparison against base commit 7e1c0f6.

♻️ This comment has been updated with latest results.

github-actions · 2025-07-16T14:29:05Z

	Tests	Passed ✅	Time ⏱
federation-audit \| abstract-types.xml	18 ran	18 ✅	423ms
federation-audit \| child-type-mismatch.xml	4 ran	4 ✅	90ms
federation-audit \| circular-reference-interface.xml	2 ran	2 ✅	49ms
federation-audit \| complex-entity-call.xml	1 ran	1 ✅	80ms
federation-audit \| corrupted-supergraph-node-id.xml	10 ran	10 ✅	126ms
federation-audit \| enum-intersection.xml	5 ran	5 ✅	64ms
federation-audit \| fed1-external-extends-resolvable.xml	1 ran	1 ✅	24ms
federation-audit \| fed1-external-extends.xml	4 ran	4 ✅	58ms
federation-audit \| fed1-external-extension.xml	4 ran	4 ✅	58ms
federation-audit \| fed2-external-extends.xml	4 ran	4 ✅	63ms
federation-audit \| fed2-external-extension.xml	4 ran	4 ✅	63ms
federation-audit \| include-skip.xml	4 ran	4 ✅	88ms
federation-audit \| input-object-intersection.xml	3 ran	3 ✅	37ms
federation-audit \| interface-object-indirect-extension.xml	1 ran	1 ✅	52ms
federation-audit \| interface-object-with-requires.xml	7 ran	7 ✅	112ms
federation-audit \| keys-mashup.xml	1 ran	1 ✅	33ms
federation-audit \| mutations.xml	4 ran	4 ✅	79ms
federation-audit \| mysterious-external.xml	2 ran	2 ✅	52ms
federation-audit \| nested-provides.xml	2 ran	2 ✅	34ms
federation-audit \| node.xml	1 ran	1 ✅	26ms
federation-audit \| non-resolvable-interface-object.xml	7 ran	7 ✅	80ms
federation-audit \| null-keys.xml	1 ran	1 ✅	29ms
federation-audit \| override-type-interface.xml	4 ran	4 ✅	66ms
federation-audit \| override-with-requires.xml	4 ran	4 ✅	90ms
federation-audit \| parent-entity-call-complex.xml	1 ran	1 ✅	34ms
federation-audit \| parent-entity-call.xml	1 ran	1 ✅	26ms
federation-audit \| provides-on-interface.xml	2 ran	2 ✅	39ms
federation-audit \| provides-on-union.xml	2 ran	2 ✅	38ms
federation-audit \| requires-interface.xml	5 ran	5 ✅	74ms
federation-audit \| requires-requires.xml	5 ran	5 ✅	105ms
federation-audit \| requires-with-argument.xml	5 ran	5 ✅	103ms
federation-audit \| requires-with-fragments.xml	6 ran	6 ✅	97ms
federation-audit \| shared-root.xml	2 ran	2 ✅	42ms
federation-audit \| simple-entity-call.xml	1 ran	1 ✅	23ms
federation-audit \| simple-inaccessible.xml	4 ran	4 ✅	53ms
federation-audit \| simple-interface-object.xml	13 ran	13 ✅	173ms
federation-audit \| simple-override.xml	2 ran	2 ✅	36ms
federation-audit \| simple-requires-provides.xml	12 ran	12 ✅	233ms
federation-audit \| typename.xml	6 ran	6 ✅	90ms
federation-audit \| unavailable-override.xml	2 ran	2 ✅	36ms
federation-audit \| union-interface-distributed.xml	10 ran	10 ✅	101ms
federation-audit \| union-intersection.xml	12 ran	12 ✅	140ms

gemini-code-assist

Code Review

This pull request fixes an overfetching issue with union types by preserving type cast information in the query plan paths. I've left a few comments, including a critical fix for a compilation error due to a type mismatch when converting MergePath to PlanPathSegment.

lib/query-planner/src/planner/plan_nodes.rs

lib/query-planner/src/ast/merge_path.rs

lib/query-planner/src/planner/plan_nodes.rs

github-actions · 2025-07-17T09:58:39Z

✅ `k6-benchmark` results

     ✓ response code was 200
     ✓ no graphql errors
     ✓ valid response structure

     █ setup

     checks.........................: 100.00% ✓ 14895      ✗ 0   
     data_received..................: 438 MB  14 MB/s
     data_sent......................: 5.8 MB  193 kB/s
     http_req_blocked...............: avg=37.88µs  min=1.32µs  med=2.69µs   max=5.93ms   p(90)=4.25µs   p(95)=4.99µs  
     http_req_connecting............: avg=34.08µs  min=0s      med=0s       max=5.87ms   p(90)=0s       p(95)=0s      
     http_req_duration..............: avg=297.65ms min=5.15ms  med=292.61ms max=647.41ms p(90)=378.61ms p(95)=416.92ms
       { expected_response:true }...: avg=297.65ms min=5.15ms  med=292.61ms max=647.41ms p(90)=378.61ms p(95)=416.92ms
     http_req_failed................: 0.00%   ✓ 0          ✗ 4985
     http_req_receiving.............: avg=281.23µs min=38.13µs med=104.94µs max=39.18ms  p(90)=354.95µs p(95)=1.29ms  
     http_req_sending...............: avg=45.78µs  min=8.65µs  med=16.99µs  max=14.59ms  p(90)=27.81µs  p(95)=44.64µs 
     http_req_tls_handshaking.......: avg=0s       min=0s      med=0s       max=0s       p(90)=0s       p(95)=0s      
     http_req_waiting...............: avg=297.32ms min=5.1ms   med=292.46ms max=644.24ms p(90)=377.84ms p(95)=416.83ms
     http_reqs......................: 4985    164.273653/s
     iteration_duration.............: avg=303.43ms min=63.35ms med=293.89ms max=959.25ms p(90)=381.42ms p(95)=425.7ms 
     iterations.....................: 4965    163.614581/s
     vus............................: 50      min=50       max=50
     vus_max........................: 50      min=50       max=50

kamilkisiela · 2025-07-17T10:00:08Z

/gemini review

kamilkisiela · 2025-07-17T10:00:11Z

/gemini summary

gemini-code-assist

Code Review

The code changes introduce type conditions to the FetchNodes, effectively fixing union overfetching. The introduction of FlattenNodePath and FlattenNodePathSegment enhances code clarity and correctness. The comprehensive tests for the new traverse_and_collect logic are commendable.

lib/query-plan-executor/src/lib.rs

lib/query-planner/src/planner/plan_nodes.rs

gemini-code-assist · 2025-07-17T10:02:27Z

Summary of Changes

This pull request introduces a significant improvement to how union types are handled in query plan execution, specifically addressing the issue of overfetching. By implementing type conditions directly within the FlattenNode's path traversal, the system can now intelligently filter data based on GraphQL's __typename field, leading to more efficient data processing and reduced unnecessary data fetching for union members.

Highlights

Core Logic Enhancement: Union Overfetching Fix: The primary goal of this PR is to prevent overfetching for union types. This is achieved by enhancing the traverse_and_collect function in query-plan-executor to incorporate type conditions. It now uses a new FlattenNodePathSegment::Cast variant to filter data based on the __typename field during traversal, ensuring that only relevant data for a specific union member is processed.
New Path Representation for Flatten Nodes: Introduced FlattenNodePath and FlattenNodePathSegment (with Field, List, and Cast variants) in query-planner to provide a more structured and expressive way to represent traversal paths within the query plan. The FlattenNode now uses this new path type, allowing for explicit type casting conditions to be embedded directly into the execution path.
Improved Path Traversal Utility: The traverse_and_collect utility function in query-plan-executor was significantly refactored to consume the new FlattenNodePathSegment enum. This refactor includes logic to handle field access, list iteration, and crucially, type-based filtering using the __typename field for Cast segments, preventing unnecessary data processing.
Updated Path Serialization and Display: The MergePath and FlattenNodePath display and serialization logic were updated to correctly include and represent Cast segments (e.g., |[TypeName]). This ensures that the query plan's string representation and its internal JSON serialization accurately reflect the new type conditions.
Comprehensive Testing and New Fixture: New unit tests for the traverse_and_collect function (lib/query-plan-executor/src/tests/traverse_and_collect.rs) were added to validate the new path traversal and type casting logic. A new supergraph fixture (lib/query-planner/fixture/tests/union-overfetching.supergraph.graphql) and a dedicated test case (union_overfetching_test) were introduced to specifically verify the union overfetching fix.

Changelog

.cargo/config.toml
- Added a new test_qpe command for running tests specifically for the query-plan-executor package.
lib/query-plan-executor/benches/executor_benches.rs
- Updated benchmark path definitions to use the new FlattenNodePathSegment enum, aligning with the core logic changes.
lib/query-plan-executor/src/lib.rs
- Refactored traverse_and_collect function to accept &[FlattenNodePathSegment] and implement type-aware data filtering for Cast segments.
- Introduced contains_typename helper function for __typename checks.
- Modified ExecutionStep and ParallelNode implementations to utilize the new FlattenNodePath type, removing intermediate string conversions.
lib/query-plan-executor/src/tests/mod.rs
- Included the new traverse_and_collect test module.
lib/query-plan-executor/src/tests/traverse_and_collect.rs
- Added a new file containing extensive unit tests for the traverse_and_collect function, covering various scenarios including field access, list traversal, and type casting.
lib/query-planner/fixture/tests/union-overfetching.supergraph.graphql
- Added a new supergraph schema fixture to enable testing of union overfetching scenarios.
lib/query-planner/src/ast/merge_path.rs
- Added Serialize and Deserialize traits to Condition and Segment enums.
- Updated the Display implementation for Segment::Cast to use a new |[TypeName] format.
- Modified path string conversion methods to include Cast segments, which were previously filtered out.
lib/query-planner/src/planner/plan_nodes.rs
- Changed FlattenNode.path type from Vec<String> to the new FlattenNodePath struct.
- Defined new enums FlattenNodePathSegment (Field, Cast, List) and FlattenNodePath.
- Implemented From<&MergePath> for FlattenNodePath to convert MergePath segments into the new FlattenNodePath structure, including Cast segments.
- Updated PrettyDisplay for FlattenNode to use the new FlattenNodePath's display implementation.
lib/query-planner/src/tests/*.rs
- Numerous test snapshots across various test files (e.g., alias.rs, arguments.rs, interface.rs, union.rs) were updated to reflect the new JSON serialization format for FlattenNode paths, now including explicit Field, List, and Cast segments.

Activity

Automated Federation Audit Results were posted, showing 177 tests passed, 12 removed, and 1 error.
Automated GraphQL over HTTP Audit Results were posted, showing 61 tests passed.
Detailed Federation Audit results per XML file were provided by github-actions[bot].
Automated k6-benchmark results were posted, indicating all checks passed and providing performance metrics.
Author kamilkisiela requested a /gemini review and /gemini summary.
A critical review comment from gemini-code-assist[bot] identified a type mismatch in FlattenNodePath conversion, suggesting as_ref().clone() for Arc<String> dereferencing.
A medium priority review comment from gemini-code-assist[bot] suggested removing commented-out lines in merge_path.rs for maintainability.
A medium priority review comment from gemini-code-assist[bot] recommended making a match statement more concise in plan_nodes.rs using if let with an or-pattern.
A medium priority review comment from gemini-code-assist[bot] advised improving flat_map logic in traverse_and_collect by separating filtering and mapping steps.
A medium priority review comment from gemini-code-assist[bot] suggested adding a Display implementation for FlattenNode for better debugging and logging.

lib/query-plan-executor/src/lib.rs

by adding type conditions to the FetchNodes

lib/query-planner/src/tests/arguments.rs

ardatan · 2025-07-17T12:48:11Z

lib/query-planner/src/tests/requires_provides.rs

-              "product"
+              {
+                "Field": "product"
+              }


Can't we also get rid of InlineFragment in the require selection set then move that type condition to here?
So instead of having ... on Product in requires selections, we can have { Cast: Product } in the flatten node's path directly, no?

This comment was marked as outdated.

Sign in to view

gemini-code-assist bot reviewed Jul 16, 2025

View reviewed changes

lib/query-planner/src/planner/plan_nodes.rs Outdated Show resolved Hide resolved

lib/query-planner/src/ast/merge_path.rs Show resolved Hide resolved

lib/query-planner/src/planner/plan_nodes.rs Outdated Show resolved Hide resolved

gemini-code-assist bot reviewed Jul 17, 2025

View reviewed changes

lib/query-plan-executor/src/lib.rs Outdated Show resolved Hide resolved

lib/query-planner/src/planner/plan_nodes.rs Show resolved Hide resolved

kamilkisiela commented Jul 17, 2025

View reviewed changes

lib/query-plan-executor/src/lib.rs Outdated Show resolved Hide resolved

Fix union overfetching

cd1934a

by adding type conditions to the FetchNodes

kamilkisiela force-pushed the kamil-conditions branch from f0fcf45 to cd1934a Compare July 17, 2025 11:42

kamilkisiela marked this pull request as ready for review July 17, 2025 11:42

dotansimha reviewed Jul 17, 2025

View reviewed changes

lib/query-planner/src/tests/arguments.rs Show resolved Hide resolved

dotansimha approved these changes Jul 17, 2025

View reviewed changes

Use "@" to represent List

b927509

ardatan reviewed Jul 17, 2025

View reviewed changes

kamilkisiela merged commit 1940cbb into main Jul 18, 2025
11 of 12 checks passed

kamilkisiela deleted the kamil-conditions branch July 18, 2025 08:52

dotansimha mentioned this pull request Jul 20, 2025

refactor(qp): introduce Single/Multi state for FetchGraph and steps #280

Draft

11 tasks

Uh oh!

Fix union overfetching #279

Fix union overfetching #279

Uh oh!

Conversation

kamilkisiela commented Jul 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as outdated.

Uh oh!

github-actions bot commented Jul 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Federation Audit Results

Uh oh!

github-actions bot commented Jul 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

GraphQL over HTTP Audit Results

Uh oh!

github-actions bot commented Jul 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Jul 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ k6-benchmark results

Uh oh!

kamilkisiela commented Jul 17, 2025

Uh oh!

kamilkisiela commented Jul 17, 2025

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

gemini-code-assist bot commented Jul 17, 2025

Summary of Changes

Highlights

Uh oh!

Uh oh!

Uh oh!

ardatan Jul 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kamilkisiela commented Jul 16, 2025 •

edited

Loading

github-actions bot commented Jul 16, 2025 •

edited

Loading

github-actions bot commented Jul 16, 2025 •

edited

Loading

github-actions bot commented Jul 16, 2025 •

edited

Loading

github-actions bot commented Jul 17, 2025 •

edited

Loading

✅ `k6-benchmark` results

ardatan Jul 17, 2025 •

edited

Loading