Improve performance of query hashing by using a precomputed schema hash #6622

IvanGoncharov · 2025-01-22T13:07:46Z

This PR reverts the query hashing back to a simpler algorithm, which is faster and has more predictable CPU and memory costs.

Previous vs. New Algorithm

Both the old and new algorithms base their hash on the query, operation name, and supergraph schema.
This hash is critical in various parts of the Router (e.g., caching query plans and entities).

Key Change

The new algorithm always hashes the entire schema (as an SDL string).
The old algorithm tried to hash only the parts of the schema referenced by the query. While it allowed the hash to stay unchanged for irrelevant schema modifications, it introduced significant complexity and risk of collisions (partially fixed in Fix the query hashing algorithm #6205).
Those fixes increased CPU and memory usage (improved somewhat in Avoid re-computing implementers_map in QueryHashVisitor (1.59) #6569), revealing scalability issues for large queries and schemas.

Benefits of the New Approach

Simple, predictable, and less prone to collisions (limited by SHA256).
Since each query depends on the same supergraph schema, the algorithm computes the schema hash only once and reuses it.

Implementation Details

A new SchemaHash type has been introduced and the existing QueryHash type refined.
QueryHash is now created via SchemaHash::operation_hash with a query string and optional operation name.
This ensures the hash always includes the full schema, plus the full query and operation name.

Future Plan

This PR still allows some legacy shortcuts in QueryHash creation for now, but they will be removed in subsequent PRs.

Checklist

Complete the checklist (and note appropriate exceptions) before the PR is marked ready-for-review.

Exceptions

Note any exceptions here

Notes

It may be appropriate to bring upcoming changes to the attention of other (impacted) groups. Please endeavour to do this before seeking PR approval. The mechanism for doing this will vary considerably, so use your judgement as to how and when to do this. ↩
Configuration is an important part of many changes. Where applicable please try to document configuration examples. ↩
Tick whichever testing boxes are applicable. If you are adding Manual Tests, please document the manual testing (extensively) in the Exceptions. ↩

The `QueryHash` type was reused here, but it's not hashing a query at all.

This is just to keep it compiling right now, we can remove it at the end (it's probably right to do so)

Adding it to all the transitive usage sites is a lot of work. Renamed it for clarity.

It would be better to make everything not-pub, but let's do that separately.

… cache

It's vital that we populate `configuration.apollo` before we try to populate `configuration.apollo.schema_id`.

Co-authored-by: Renée <[email protected]>

IvanGoncharov · 2025-01-23T00:51:54Z

apollo-router/tests/set_context.rs

@@ -313,6 +313,8 @@ fn setup_from_mocks(
    configuration: serde_json::Value,
    mocks: &[(&'static str, &'static str)],
 ) -> TestHarness<'static> {
+    let _ = rustls::crypto::aws_lc_rs::default_provider().install_default();
+


@goto-bus-stop This looks like an unintentional change introduced by the merge.
Is it correct?

Tests failed without it. We have to do this in several places after the hyper 1.0 upgrade on dev, but it's a bit weird that these tests were passing on dev before :/

It's because tonic changed. We need to call this.

IvanGoncharov · 2025-01-23T00:52:37Z

apollo-router/tests/type_conditions.rs

@@ -354,6 +354,8 @@ fn setup_from_mocks(
    configuration: serde_json::Value,
    mocks: &[(&'static str, &'static str)],
 ) -> TestHarness<'static> {
+    let _ = rustls::crypto::aws_lc_rs::default_provider().install_default();
+


@goto-bus-stop looks like the same happened here

SimonSapin · 2025-01-23T12:36:30Z

apollo-router/src/spec/schema.rs

+// FIXME: It seems bad that you can create an empty hash easily and use it in security-critical
+// places. This impl should be deleted outright and we should update usage sites.
+// If the query hash is truly not required to contain data in those usage sites, we should use
+// something like an Option instead.


This seems potentially quite bad. File an issue so we don’t forget it?

What's the impact of fixing this now? Adding something that is a security risk doesn't sound good.

@BrynCooke Actual call to default happening here:
https://github.com/apollographql/router/pull/6622/files#diff-7108edf02483a7de6d8c37b938df0634afc78590242b4fc94709c9dd2e626fb4R108
Previously, it was just less noticeable, and my change and @goto-bus-stop comments just make it stand out more.
Fixing it will significantly increase scope of this PR but @SimonSapin is right, we need to open an issue about it.
I will do that right now

apollo-router/src/query_planner/fetch.rs

apollo-router/src/query_planner/tests.rs

IvanGoncharov · 2025-01-23T15:45:02Z

@Mergifyio backport 1.59.2

mergify · 2025-01-23T15:45:10Z

backport 1.59.2

✅ Backports have been created

#6631 Improve performance of query hashing by using a precomputed schema hash (backport #6622) has been created for branch 1.59.2 but encountered conflicts

…sh (#6622) Co-authored-by: Simon Sapin <[email protected]> Co-authored-by: Renée Kooi <[email protected]> (cherry picked from commit 8c81327) # Conflicts: # apollo-router/src/query_planner/bridge_query_planner.rs # apollo-router/src/query_planner/caching_query_planner.rs # apollo-router/src/query_planner/fetch.rs # apollo-router/src/query_planner/snapshots/apollo_router__query_planner__bridge_query_planner__tests__plan_root.snap # apollo-router/src/spec/query/change.rs # apollo-router/src/spec/schema.rs # apollo-router/tests/integration/redis.rs # apollo-router/tests/snapshots/type_conditions___test_type_conditions_disabled.snap # apollo-router/tests/snapshots/type_conditions___test_type_conditions_enabled.snap # apollo-router/tests/snapshots/type_conditions___test_type_conditions_enabled_generate_query_fragments.snap # apollo-router/tests/snapshots/type_conditions___test_type_conditions_enabled_list_of_list.snap # apollo-router/tests/snapshots/type_conditions___test_type_conditions_enabled_list_of_list_of_list.snap # apollo-router/tests/snapshots/type_conditions___test_type_conditions_enabled_shouldnt_make_article_fetch.snap

goto-bus-stop · 2025-01-23T15:53:27Z

@Mergifyio backport 1.x

mergify · 2025-01-23T15:53:37Z

backport 1.x

✅ Backports have been created

#6633 Improve performance of query hashing by using a precomputed schema hash (backport #6622) has been created for branch 1.x but encountered conflicts

…sh (#6622) Co-authored-by: Simon Sapin <[email protected]> Co-authored-by: Renée Kooi <[email protected]> (cherry picked from commit 8c81327) # Conflicts: # apollo-router/src/query_planner/fetch.rs # apollo-router/src/spec/schema.rs

BrynCooke · 2025-01-23T15:56:56Z

@Mergifyio refresh

mergify · 2025-01-23T15:57:02Z

refresh

✅ Pull request refreshed

…sh (backport #6622) (#6631) Co-authored-by: Ivan Goncharov <[email protected]> Co-authored-by: Renée Kooi <[email protected]>

…sh (backport #6622) (#6633) Co-authored-by: Ivan Goncharov <[email protected]> Co-authored-by: Renée Kooi <[email protected]>

SimonSapin and others added 30 commits January 20, 2025 14:06

Avoid re-computing implementers_map in QueryHashVisitor (#6569)

be40574

WIP: Introduce QueryHash and SchemaHash types

c3699e2

Keep QueryHash::default but with scary comments

612db49

Back to using Arc<String> as the representation for schema ID

7ac072b

Rename SchemaHash -> SchemaId

8d3c02e

Introduce ConfigModeHash type

a600f7b

The `QueryHash` type was reused here, but it's not hashing a query at all.

Update API usage

43f001f

Reintroduce schema_id field to CachingQueryKey

68a3738

This is just to keep it compiling right now, we can remove it at the end (it's probably right to do so)

Update two schema aware hash callsites (this may not be all)

ee1b489

Document QueryHash

d33e40d

next step

a0b9dea

fix error in entity.rs

866c807

Fix import of QueryHash inside batching code

ffe1b6c

Merge remote-tracking branch 'origin/1.59.2' into i1g/query_hash_cleanup

f046cd8

Pass query text to extract_query_information as necessary

8d30e7e

Remove cfg(test) from Query::empty

50d1829

Adding it to all the transitive usage sites is a lot of work. Renamed it for clarity.

Fix test assertion

c6aa3af

Comment how Request::query_hash works

879e71a

Fix lifetime in BridgeQueryPlanner::call

594a746

Mark ConfigModeHash pub(crate) to appease linter

68aa912

It would be better to make everything not-pub, but let's do that separately.

it's building !!!

9473d13

Update test

c83dd83

remove "Schema aware query hashing algorithm"

8fe5095

Update cache keys in snapshots

662bb83

Update some more snapshots

941dad3

Reintroduce operation parsing for query plans loaded from distributed…

e2afd1a

… cache

Hash the raw query hash bytes

1997eb1

Fix inject_schema_id

5adc7fc

It's vital that we populate `configuration.apollo` before we try to populate `configuration.apollo.schema_id`.

Make clippy happy!

967217a

Snapshot updates for setContext and type conditioned fetching

4882b2a

IvanGoncharov changed the title ~~Improve performance of query hashing by using precomputed schema hash~~ Improve performance of query hashing by using a precomputed schema hash Jan 23, 2025

add changelog

a4404f3

IvanGoncharov marked this pull request as ready for review January 23, 2025 00:43

IvanGoncharov requested a review from a team as a code owner January 23, 2025 00:43

IvanGoncharov and others added 2 commits January 23, 2025 02:43

Merge branch 'dev' into i1g/query_hash_cleanup

13095ac

Update apollo-router/src/router_factory.rs

23dedd1

Co-authored-by: Renée <[email protected]>

IvanGoncharov commented Jan 23, 2025

View reviewed changes

SimonSapin reviewed Jan 23, 2025

View reviewed changes

goto-bus-stop added 2 commits January 23, 2025 14:47

Re-add test

67815a5

Un-double-Arc SchemaHash in fetch.rs

f4e8b7f

SimonSapin approved these changes Jan 23, 2025

View reviewed changes

BrynCooke approved these changes Jan 23, 2025

View reviewed changes

IvanGoncharov merged commit 8c81327 into dev Jan 23, 2025
16 checks passed

IvanGoncharov deleted the i1g/query_hash_cleanup branch January 23, 2025 15:43

mergify bot mentioned this pull request Jan 23, 2025

Improve performance of query hashing by using a precomputed schema hash (backport #6622) #6631

Merged

6 tasks

BrynCooke added backport-1.x Backport this PR to 1.x and removed backport-1.x Backport this PR to 1.x labels Jan 23, 2025

mergify bot mentioned this pull request Jan 23, 2025

Improve performance of query hashing by using a precomputed schema hash (backport #6622) #6633

Merged

6 tasks

abernix pushed a commit that referenced this pull request Jan 23, 2025

Improve performance of query hashing by using a precomputed schema ha…

7161c2e

…sh (backport #6622) (#6631) Co-authored-by: Ivan Goncharov <[email protected]> Co-authored-by: Renée Kooi <[email protected]>

abernix pushed a commit that referenced this pull request Jan 24, 2025

Improve performance of query hashing by using a precomputed schema ha…

0baa02b

…sh (backport #6622) (#6633) Co-authored-by: Ivan Goncharov <[email protected]> Co-authored-by: Renée Kooi <[email protected]>

abernix mentioned this pull request Jan 28, 2025

prep release: v1.59.2 #6672

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve performance of query hashing by using a precomputed schema hash #6622

Improve performance of query hashing by using a precomputed schema hash #6622

IvanGoncharov commented Jan 22, 2025 •

edited by jira bot

Loading

IvanGoncharov Jan 23, 2025

goto-bus-stop Jan 23, 2025

BrynCooke Jan 23, 2025

IvanGoncharov Jan 23, 2025

SimonSapin Jan 23, 2025

BrynCooke Jan 23, 2025

IvanGoncharov Jan 23, 2025

IvanGoncharov commented Jan 23, 2025

mergify bot commented Jan 23, 2025 •

edited

Loading

goto-bus-stop commented Jan 23, 2025

mergify bot commented Jan 23, 2025 •

edited

Loading

BrynCooke commented Jan 23, 2025

mergify bot commented Jan 23, 2025

Improve performance of query hashing by using a precomputed schema hash #6622

Improve performance of query hashing by using a precomputed schema hash #6622

Conversation

IvanGoncharov commented Jan 22, 2025 • edited by jira bot Loading

Previous vs. New Algorithm

Key Change

Benefits of the New Approach

Implementation Details

Future Plan

Footnotes

IvanGoncharov Jan 23, 2025

Choose a reason for hiding this comment

goto-bus-stop Jan 23, 2025

Choose a reason for hiding this comment

BrynCooke Jan 23, 2025

Choose a reason for hiding this comment

IvanGoncharov Jan 23, 2025

Choose a reason for hiding this comment

SimonSapin Jan 23, 2025

Choose a reason for hiding this comment

BrynCooke Jan 23, 2025

Choose a reason for hiding this comment

IvanGoncharov Jan 23, 2025

Choose a reason for hiding this comment

IvanGoncharov commented Jan 23, 2025

mergify bot commented Jan 23, 2025 • edited Loading

✅ Backports have been created

goto-bus-stop commented Jan 23, 2025

mergify bot commented Jan 23, 2025 • edited Loading

✅ Backports have been created

BrynCooke commented Jan 23, 2025

mergify bot commented Jan 23, 2025

✅ Pull request refreshed

IvanGoncharov commented Jan 22, 2025 •

edited by jira bot

Loading

mergify bot commented Jan 23, 2025 •

edited

Loading

mergify bot commented Jan 23, 2025 •

edited

Loading