Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(federation): query plan correctness checker #6498

Merged
merged 29 commits into from
Feb 9, 2025
Merged

feat(federation): query plan correctness checker #6498

merged 29 commits into from
Feb 9, 2025

Conversation

duckki
Copy link
Contributor

@duckki duckki commented Dec 21, 2024 •
edited
Loading

This PR implements the first version of the query plan correctness checker.
It implemented the completeness part of the plan. I'll create a separate PR for the soundness part.

Status

  • ResponseShape computation from operations
  • ResponseShape computation from query plans
  • ResponseShape comparison
  • Handle all PathElements.
  • Handle Defer/Subscription nodes
  • Subgraph constraints handling
  • Corpus run

@svc-apollo-docs
Copy link
Collaborator

svc-apollo-docs commented Dec 21, 2024 •
edited
Loading

✅ Docs preview has no changes

The preview was not built because there were no changes.

Build ID: a73096d5747ac8585d82eb71

@github-actions GitHub Actions
Copy link
Contributor

@duckki, please consider creating a changeset entry in /.changesets/. These instructions describe the process and tooling.

@router-perf
Copy link

router-perf bot commented Dec 21, 2024

CI performance tests

  • connectors-const - Connectors stress test that runs with a constant number of users
  • const - Basic stress test that runs with a constant number of users
  • demand-control-instrumented - A copy of the step test, but with demand control monitoring and metrics enabled
  • demand-control-uninstrumented - A copy of the step test, but with demand control monitoring enabled
  • enhanced-signature - Enhanced signature enabled
  • events - Stress test for events with a lot of users and deduplication ENABLED
  • events_big_cap_high_rate - Stress test for events with a lot of users, deduplication enabled and high rate event with a big queue capacity
  • events_big_cap_high_rate_callback - Stress test for events with a lot of users, deduplication enabled and high rate event with a big queue capacity using callback mode
  • events_callback - Stress test for events with a lot of users and deduplication ENABLED in callback mode
  • events_without_dedup - Stress test for events with a lot of users and deduplication DISABLED
  • events_without_dedup_callback - Stress test for events with a lot of users and deduplication DISABLED using callback mode
  • extended-reference-mode - Extended reference mode enabled
  • large-request - Stress test with a 1 MB request payload
  • no-tracing - Basic stress test, no tracing
  • reload - Reload test over a long period of time at a constant rate of users
  • step-jemalloc-tuning - Clone of the basic stress test for jemalloc tuning
  • step-local-metrics - Field stats that are generated from the router rather than FTV1
  • step-with-prometheus - A copy of the step test with the Prometheus metrics exporter enabled
  • step - Basic stress test that steps up the number of users over time
  • xlarge-request - Stress test with 10 MB request payload
  • xxlarge-request - Stress test with 100 MB request payload

@duckki duckki force-pushed the duckki/correctness branch from a910d24 to bcb23f4 Compare December 22, 2024 05:34
@duckki duckki changed the title WIP/feat(federation): started query plan correctness checker WIP/feat(federation): query plan correctness checker Jan 8, 2025
@duckki duckki force-pushed the duckki/correctness branch 3 times, most recently from 2632b38 to 38bb635 Compare January 11, 2025 23:54
@goto-bus-stop goto-bus-stop mentioned this pull request Jan 17, 2025
Closed
duckki added 14 commits January 27, 2025 14:47
@duckki
feat(federation): started query plan correctness checker
18f1a29 
- implemented ResponseShape computation from an operation
@duckki
implemented basic correctness check of query plans
5eab255 
- comparison of two response shapes
- query plans analysis, deriving plan's overall response shape
@duckki
refactor: handle interpret_query_plan failures as mismatch error
85aae0f 
- instead of federation internal error
@duckki
debug: added more fmt::Display implementations for response shape-rel…
7c41d20 
…ated data structures
@duckki
debug: improved response shape display to suppress showing type condi…
b1e0f27 
…tions unnecessarily
@duckki
fix: merge subscription's rest node from the state after the primary
ed0d303
@duckki
fix: support type-conditioned fetching
7a9b39b
@duckki
fix: infer subgraph constraints
f33f9ae
@duckki
refactor: use the set-intersection notation (∩) for type conditions
acab67e
@duckki
refactor: moved the correctness module to apollo-federation/src/
23ec404
@duckki
added compare_operations function
b8217a6
@duckki
fix: check if entity type definitions are resolvable
fdb9a02
@duckki
refactor: cleaning up
fb9e5d2
@duckki
added an extra sanity check where multiple typename conditions are pr…
294c280 
…ovided
@duckki duckki force-pushed the duckki/correctness branch from e1bd353 to 294c280 Compare January 29, 2025 03:54
@duckki duckki changed the title WIP/feat(federation): query plan correctness checker feat(federation): query plan correctness checker Jan 31, 2025
@duckki
Copy link
Contributor Author

duckki commented Jan 31, 2025

I have finished the first version (the completeness checking part) and rebased it on more recent dev branch commit.
This is now ready for review.

@duckki duckki added the backport-1.x Backport this PR to 1.x label Jan 31, 2025
@duckki duckki requested a review from lrlna January 31, 2025 19:01
TylerBloom
TylerBloom approved these changes Feb 6, 2025
View reviewed changes
Copy link
Contributor

@TylerBloom TylerBloom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The core looks very good and inline with the design docs. I have some general cleanup comments

apollo-federation/src/lib.rs Show resolved Hide resolved
apollo-federation/src/correctness/mod.rs Outdated Show resolved Hide resolved
apollo-federation/src/correctness/mod.rs Outdated Show resolved Hide resolved
apollo-federation/src/correctness/query_plan_analysis.rs Outdated Show resolved Hide resolved
apollo-federation/src/correctness/response_shape_compare.rs Outdated Show resolved Hide resolved
apollo-federation/src/correctness/response_shape_compare.rs Outdated Show resolved Hide resolved
apollo-federation/src/correctness/response_shape_compare.rs Show resolved Hide resolved
apollo-federation/src/correctness/subgraph_constraint.rs Outdated Show resolved Hide resolved
apollo-federation/src/correctness/query_plan_analysis.rs Show resolved Hide resolved
@duckki
Copy link
Contributor Author

duckki commented Feb 8, 2025

Thanks, @TylerBloom . The refactored code looks so much better!

@duckki duckki enabled auto-merge (squash) February 8, 2025 23:53
@duckki duckki merged commit 1724f0f into dev Feb 9, 2025
14 checks passed
@duckki duckki deleted the duckki/correctness branch February 9, 2025 00:09
@duckki
Copy link
Contributor Author

duckki commented Feb 11, 2025

@mergify queue

@mergify Mergify
Copy link
Contributor

mergify bot commented Feb 11, 2025

queue

🟠 Waiting for conditions to match

  • -closed [📌 queue requirement]
  • -conflict [📌 queue requirement]
  • -draft [📌 queue requirement]
  • any of: [📌 queue -> configuration change requirements]
    • -mergify-configuration-changed
    • check-success = Configuration changed
  • any of: [📌 queue requirement]
    • check-neutral = Mergify Merge Protections
    • check-skipped = Mergify Merge Protections
    • check-success = Mergify Merge Protections
  • any of: [🔀 queue conditions]
    • all of: [📌 queue conditions of queue default]
      • #approved-reviews-by >= 1 [🛡 GitHub repository ruleset rule]
      • #changes-requested-reviews-by = 0 [🛡 GitHub repository ruleset rule]
      • any of: [🛡 GitHub repository ruleset rule]
        • check-success = CLA
        • check-neutral = CLA
        • check-skipped = CLA
      • any of: [🛡 GitHub repository ruleset rule]
        • check-success = ci/circleci: check_compliance-amd_linux_build
        • check-neutral = ci/circleci: check_compliance-amd_linux_build
        • check-skipped = ci/circleci: check_compliance-amd_linux_build
      • any of: [🛡 GitHub repository ruleset rule]
        • check-success = ci/circleci: check_helm-amd_linux_helm
        • check-neutral = ci/circleci: check_helm-amd_linux_helm
        • check-skipped = ci/circleci: check_helm-amd_linux_helm
      • any of: [🛡 GitHub repository ruleset rule]
        • check-success = ci/circleci: test-amd_linux_test
        • check-neutral = ci/circleci: test-amd_linux_test
        • check-skipped = ci/circleci: test-amd_linux_test
      • any of: [🛡 GitHub repository ruleset rule]
        • check-success = ci/circleci: test-arm_linux_test
        • check-neutral = ci/circleci: test-arm_linux_test
        • check-skipped = ci/circleci: test-arm_linux_test
      • any of: [🛡 GitHub repository ruleset rule]
        • check-success = ci/circleci: test-macos_test
        • check-neutral = ci/circleci: test-macos_test
        • check-skipped = ci/circleci: test-macos_test
      • any of: [🛡 GitHub repository ruleset rule]
        • check-success = ci/circleci: test-windows_test
        • check-neutral = ci/circleci: test-windows_test
        • check-skipped = ci/circleci: test-windows_test
      • any of: [🛡 GitHub repository ruleset rule]
        • check-neutral = Mergify Merge Protections
        • check-skipped = Mergify Merge Protections
        • check-success = Mergify Merge Protections

@duckki
Copy link
Contributor Author

duckki commented Feb 11, 2025

@mergify backport 1.x

@mergify Mergify
Copy link
Contributor

mergify bot commented Feb 11, 2025 •
edited
Loading

backport 1.x

✅ Backports have been created

mergify bot pushed a commit that referenced this pull request Feb 11, 2025
@duckki @mergify
feat(federation): query plan correctness checker (#6498)
541769a 
- This is the first version that implements the completeness part.

(cherry picked from commit 1724f0f)
@mergify mergify bot mentioned this pull request Feb 11, 2025
Merged
7 tasks
duckki added a commit that referenced this pull request Feb 11, 2025
@mergify @duckki
feat(federation): query plan correctness checker (backport #6498) (#6767
0015c17 
)

Co-authored-by: Duckki Oe <duckki.oe@apollographql.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TylerBloom TylerBloom TylerBloom approved these changes

@dariuszkuc dariuszkuc Awaiting requested review from dariuszkuc dariuszkuc is a code owner

@sachindshinde sachindshinde Awaiting requested review from sachindshinde sachindshinde is a code owner

@goto-bus-stop goto-bus-stop Awaiting requested review from goto-bus-stop goto-bus-stop is a code owner

@SimonSapin SimonSapin Awaiting requested review from SimonSapin SimonSapin is a code owner

@lrlna lrlna Awaiting requested review from lrlna lrlna is a code owner

Assignees
No one assigned
Labels
backport-1.x Backport this PR to 1.x
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@duckki @svc-apollo-docs @lrlna @TylerBloom