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

fix(dual-query-planner): updated semantic diff to handle fragment name differences #6158

Merged
merged 3 commits into from
Oct 22, 2024
Merged

fix(dual-query-planner): updated semantic diff to handle fragment name differences #6158

merged 3 commits into from
Oct 22, 2024

Conversation

duckki
Copy link
Contributor

@duckki duckki commented Oct 16, 2024

Fragment definitions and their names could be slightly reordered/switched. This PR updates semantic diff to handle those differences.

  • same_ast_document creates a fragment mapping from the JS fragment name to matching Rust fragment name based on their definition body, not their places or names.
  • Fragment spreads in selection sets are compared after mapping JS fragment names to Rust fragment names.

Checklist

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

  • Changes are compatible[^1]
  • Documentation[^2] completed
  • Performance impact assessed and acceptable
  • Tests added and passing[^3]
    • Unit Tests
    • Integration Tests
    • Manual Tests

@duckki duckki requested review from a team as code owners October 16, 2024 01:54
@svc-apollo-docs
Copy link
Collaborator

svc-apollo-docs commented Oct 16, 2024 •
edited
Loading

✅ Docs Preview Ready

No new or changed pages found.

@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 Oct 16, 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 marked this pull request as draft October 16, 2024 04:05
@duckki duckki mentioned this pull request Oct 18, 2024
Merged
6 tasks
@duckki
Copy link
Contributor Author

duckki commented Oct 18, 2024 •
edited
Loading

#6173 should be merged before this PR to avoid false mismatches.

@duckki duckki force-pushed the duckki/router-811 branch from 3e92696 to 67bae6f Compare October 21, 2024 18:45
@duckki duckki marked this pull request as ready for review October 21, 2024 18:49
TylerBloom
TylerBloom approved these changes Oct 21, 2024
View reviewed changes
apollo-router/src/query_planner/dual_query_planner.rs
// Thus, we can build the fragment name mapping in a single pass and compare
// fragment definitions using the mapping at the same time, since earlier fragments
// will never reference later fragments.
x_frags.iter().try_fold((), |_, x_frag| {
Copy link
Contributor

Choose a reason for hiding this comment

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

Not important, but I think you're looking for .try_for_each rather than .try_fold using a unit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, I wanted to something like try_all (rust-lang/rfcs#3233) or FallibleIterator (but it's private). We should make FallibleIterator more accessible :)

@duckki duckki enabled auto-merge (squash) October 22, 2024 02:45
@duckki duckki merged commit ed9f504 into dev Oct 22, 2024
15 checks passed
@duckki duckki deleted the duckki/router-811 branch October 22, 2024 03:06
LongLiveCHIEF pushed a commit to StateFarmIns/router that referenced this pull request Nov 21, 2024
@duckki @LongLiveCHIEF
fix(dual-query-planner): updated semantic diff to handle fragment nam…
f3d6fd1 
…e differences (apollographql#6158)

- `same_ast_document` now creates a fragment mapping from the JS fragment name to matching Rust fragment name based on their definition body, not their places or names.
- Fragment spreads in selection sets are compared after mapping JS fragment names to Rust fragment names.
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

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

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